Re: [PATCH] gdb: fix missing null-character when using value_cstring

[Andrew Burgess <aburgess@redhat.com>]

Pedro Alves <pedro@palves.net> writes:

> On 2023-04-06 2:20 p.m., Andrew Burgess via Gdb-patches wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>> On 4/3/23 17:49, Andrew Burgess via Gdb-patches wrote:
>
>>>> Alternatively, if we change the requirements of value_cstring so that
>>>> we require the passed length includes the trailing null, then we can
>>>> assert that this is indeed the case within value_cstring.  Any
>>>> incorrect uses in the future will be quickly spotted.
>>>>
>>>> So that's what I did, c_string_operation::evaluate is left unchanged,
>>>> but every other use of value_cstring is adjusted with a '+ 1' so that
>>>> we include the null within the length being passed.
>>>
>>> That sounds counterintuitive to me.  With an API of style pointer +
>>> length, I don't expect the length to include the null terminator.  It
>>> also unnecessarily forces the caller to have a null-terminated version
>>> of the string, which may not always be the case (you might want to call
>>> value_cstring on a subset of an existing string).
>>>
>>> I think that:
>>>
>>> struct value *
>>> value_cstring (const char *ptr, ssize_t len, struct type *char_type)
>>>
>>> should take a length excluding the null terminator, but a null
>>> terminator in the result (its job is to build a C string, and a C string
>>> requires a null terminator at the end).
>> 
>> This is why writing comments is so important :)
>> 
>> I read it as "build a value* from this C-string", which is why I figured
>> we can assume there will be a '\0' at the end.
>> 
>> Anyway, I don't really mind either way, just so long as we can get
>> something that works!  I'll flip this around to the way you suggest and
>> repost.
>> 
>
> Hmm, this rang a bell.  See this patch and following discussion:
>
>  https://inbox.sourceware.org/gdb-patches/20210713153142.3957666-1-simon.marchi@efficios.com/
>

Thanks for pointing that out.

I guess from the age of that patch it's not something you're planning to
get back to anytime soon.  I assume you'd have no objections if I
rebased your patch and merged it with my tests and reposted it?

Thanks,
Andrew