Re: [PATCH RESEND] gdb: remove static buffer in command_line_input

[Pedro Alves <pedro@palves.net>]

On 2022-12-16 1:38 p.m., Simon Marchi wrote:
> 
> 
> On 12/16/22 08:21, Pedro Alves wrote:
>> On 2022-12-15 7:06 p.m., Simon Marchi via Gdb-patches wrote:
>>>    else
>>>      {
>>>        /* Copy whole line including terminating null, and we're
>>>  	 done.  */
>>> -      buffer_grow (cmd_line_buffer, rl, len + 1);
>>> -      cmd = cmd_line_buffer->buffer;
>>> +      cmd_line_buffer.append (rl, len + 1);
>>> +      return true;
>>
>> std::string is guaranteed to be null terminated, but, it can also hold
>> \0 characters in the middle of the string.  Isn't that
>>
>>   cmd_line_buffer.append (rl, len + 1);
>>
>> ending up with an embedded \0 at the end of the string, with cmd_line_buffer.size() accounting for it?
> 
> Argh, yes!  I rewrote this patch from scratch multiple times until I got
> something satisfactory, and I know I changed that in a previous
> iteration, because I remember having that thought.
> 
> Here's a patch to fix it:
> 
> 
> From 8757d2ec9edf2ae0b77039ca0ecc9ca32fd87256 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Fri, 16 Dec 2022 08:34:56 -0500
> Subject: [PATCH] gdb: don't copy final null character in
>  command_line_append_input_line
> 
> The modified std::string::append call currently copies the null
> character from the source string, leaving the std::string with a null
> character in its content, part of its size, in addition to the null
> character managed by the std::string itself  We don't want to copy that

Missing period after "itself".

> null character, so remove that "+ 1".
> 
> Change-Id: I412fe9bd6b08e7ce7604ef5d8038b62f1a784c9b
> ---
>  gdb/event-top.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/event-top.c b/gdb/event-top.c
> index fa86a89e4a19..fa9e1f3f6a73 100644
> --- a/gdb/event-top.c
> +++ b/gdb/event-top.c
> @@ -640,7 +640,7 @@ command_line_append_input_line (std::string &cmd_line_buffer, const char *rl)
>      {
>        /* Copy whole line including terminating null, and we're
>  	 done.  */
> -      cmd_line_buffer.append (rl, len + 1);
> +      cmd_line_buffer.append (rl, len);

I'd update the comment -- the "including terminating null" part seems
misleading now, just drop that part?

Otherwise LTGM.

>        return true;
>      }
>  }
> 
> base-commit: e60a615dde5d6674a6488b74afe807a775551407
>