Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

  On Wednesday, January 29 2020, Pedro Alves wrote:

> On 1/29/20 5:59 PM, Sergio Durigan Junior wrote:
>> While testing a GCC 10 build of our git HEAD, I noticed an error
>> triggered by -Werror-stringop on
>> infcmd.c:construct_inferior_arguments.  One of the things the function
>> does is calculate the length of the string that will hold the
>> inferior's arguments.  GCC warns us that 'length' can be 0, which can
>> lead to undesired behaviour:
>> 
>> ../../gdb/infcmd.c: In function 'char* construct_inferior_arguments(int, char**)':
>> ../../gdb/infcmd.c:369:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>>   369 |       result[0] = '\0';
>>       |       ~~~~~~~~~~^~~~~~
>> ../../gdb/infcmd.c:368:33: note: at offset 0 to an object with size 0 allocated by 'xmalloc' here
>>   368 |       result = (char *) xmalloc (length);
>>       |                         ~~~~~~~~^~~~~~~~
>> 
>
>> The solution here is to explicit check that 'length' is greater than
>> 0.  Since we're dealing with 'argc', I think it's pretty much
>> guaranteed that it's going to be at least 1.
>
> It's a certainly -- there's only one caller to construct_inferior_arguments,
> which does:
>
> const char *
> get_inferior_args (void)
> {
>   if (current_inferior ()->argc != 0)
>     {      
>       char *n;
>
>       n = construct_inferior_arguments (current_inferior ()->argc,
> 					current_inferior ()->argv);
>
>
> (construct_inferior_arguments could be made static, btw.)

Thanks.

>> --- a/gdb/infcmd.c
>> +++ b/gdb/infcmd.c
>> @@ -365,6 +365,11 @@ construct_inferior_arguments (int argc, char **argv)
>>  	  length += strlen (argv[i]) + 1;
>>  	}
>>  
>> +      /* argc should always be at least 1, but we double check this
>
> Uppercase ARGC.  But putting
>
>  gdb_assert (argc > 0);
>
> at the top of the function instead as I originally suggested also works
> for me (tried current gcc master), which seems a bit better to me, as it
> covers both branches at once.  Did it not work for you?  This makes gcc see
> that the loops always run at least once.

Yes, this should work.

Updated patch below.

Thanks,
  

On Wednesday, January 29 2020, I wrote:

> On Wednesday, January 29 2020, Pedro Alves wrote:
>
>> On 1/29/20 5:59 PM, Sergio Durigan Junior wrote:
>>> While testing a GCC 10 build of our git HEAD, I noticed an error
>>> triggered by -Werror-stringop on
>>> infcmd.c:construct_inferior_arguments.  One of the things the function
>>> does is calculate the length of the string that will hold the
>>> inferior's arguments.  GCC warns us that 'length' can be 0, which can
>>> lead to undesired behaviour:
>>> 
>>> ../../gdb/infcmd.c: In function 'char* construct_inferior_arguments(int, char**)':
>>> ../../gdb/infcmd.c:369:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>>>   369 |       result[0] = '\0';
>>>       |       ~~~~~~~~~~^~~~~~
>>> ../../gdb/infcmd.c:368:33: note: at offset 0 to an object with size 0 allocated by 'xmalloc' here
>>>   368 |       result = (char *) xmalloc (length);
>>>       |                         ~~~~~~~~^~~~~~~~
>>> 
>>
>>> The solution here is to explicit check that 'length' is greater than
>>> 0.  Since we're dealing with 'argc', I think it's pretty much
>>> guaranteed that it's going to be at least 1.
>>
>> It's a certainly -- there's only one caller to construct_inferior_arguments,
>> which does:
>>
>> const char *
>> get_inferior_args (void)
>> {
>>   if (current_inferior ()->argc != 0)
>>     {      
>>       char *n;
>>
>>       n = construct_inferior_arguments (current_inferior ()->argc,
>> 					current_inferior ()->argv);
>>
>>
>> (construct_inferior_arguments could be made static, btw.)
>
> Thanks.
>
>>> --- a/gdb/infcmd.c
>>> +++ b/gdb/infcmd.c
>>> @@ -365,6 +365,11 @@ construct_inferior_arguments (int argc, char **argv)
>>>  	  length += strlen (argv[i]) + 1;
>>>  	}
>>>  
>>> +      /* argc should always be at least 1, but we double check this
>>
>> Uppercase ARGC.  But putting
>>
>>  gdb_assert (argc > 0);
>>
>> at the top of the function instead as I originally suggested also works
>> for me (tried current gcc master), which seems a bit better to me, as it
>> covers both branches at once.  Did it not work for you?  This makes gcc see
>> that the loops always run at least once.
>
> Yes, this should work.
>
> Updated patch below.

Ops, I forgot to s/argc/ARGC/.  I did that locally.

> Thanks,
>
> -- 
> Sergio
> GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
> Please send encrypted e-mail if possible
> http://sergiodj.net/
>
> From 6c746d24ccf13a56b9164fb241a4091223f1f706 Mon Sep 17 00:00:00 2001
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Wed, 29 Jan 2020 12:53:55 -0500
> Subject: [PATCH] Assert that 'length' > 0 on
>  infcmd.c:construct_inferior_arguments
>
> While testing a GCC 10 build of our git HEAD, I noticed an error
> triggered by -Werror-stringop on
> infcmd.c:construct_inferior_arguments.  One of the things the function
> does is calculate the length of the string that will hold the
> inferior's arguments.  GCC warns us that 'length' can be 0, which can
> lead to undesired behaviour:
>
> ../../gdb/infcmd.c: In function 'char* construct_inferior_arguments(int, char**)':
> ../../gdb/infcmd.c:369:17: error: writing 1 byte into a region of size 0 [-Werror=stringop-overflow=]
>   369 |       result[0] = '\0';
>       |       ~~~~~~~~~~^~~~~~
> ../../gdb/infcmd.c:368:33: note: at offset 0 to an object with size 0 allocated by 'xmalloc' here
>   368 |       result = (char *) xmalloc (length);
>       |                         ~~~~~~~~^~~~~~~~
>
> The solution here is to explicit check that 'length' is greater than
> 0.  Since we're dealing with 'argc', I think it's pretty much
> guaranteed that it's going to be at least 1.
>
> Tested by rebuilding.
>
> OK?
>
> gdb/ChangeLog:
> 2020-01-29  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* infcmd.c (construct_inferior_arguments): Assert that
> 	'length' is greater than 0.
>
> Change-Id: Ide8407cbedcb4921de1843a6a15bbcb7676c7d26
> ---
>  gdb/infcmd.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index b44adca88d..ee3d9a7d98 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -268,6 +268,11 @@ construct_inferior_arguments (int argc, char **argv)
>  {
>    char *result;
>  
> +  /* argc should always be at least 1, but we double check this
> +     here.  This is also needed to silence -Werror-stringop
> +     warnings.  */
> +  gdb_assert (length > 0);
> +
>    if (startup_with_shell)
>      {
>  #ifdef __MINGW32__
> -- 
> 2.21.0
  

On 1/29/20 7:54 PM, Sergio Durigan Junior wrote:
> On Wednesday, January 29 2020, Pedro Alves wrote:

>> Uppercase ARGC.  But putting
>>
>>  gdb_assert (argc > 0);
>>
>> at the top of the function instead as I originally suggested also works
>> for me (tried current gcc master), which seems a bit better to me, as it
>> covers both branches at once.  Did it not work for you?  This makes gcc see
>> that the loops always run at least once.
> 
> Yes, this should work.
> 
> Updated patch below.

The version you sent won't build for sure -- you're checking
length instead of argc:

 @@ -268,6 +268,11 @@ construct_inferior_arguments (int argc, char **argv)
  {
    char *result;
  
 +  /* argc should always be at least 1, but we double check this
 +     here.  This is also needed to silence -Werror-stringop
 +     warnings.  */
 +  gdb_assert (length > 0);


This is OK with that fixed, but please update the commit log, here:

 "The solution here is to explicit check that 'length' is greater than
 0.  Since we're dealing with 'argc', I think it's pretty much
 guaranteed that it's going to be at least 1."

I'd go with:

 The solution here is to assert that 'argc' is greater than
 0 on entry, which makes GCC understand that the loops always
 run at least once, and thus 'length' is always > 0.

... and also update the commit's subject.  I'd use

 Fix -Werror-stringop error on infcmd.c:construct_inferior_arguments

instead, to talk in terms of what you're fixing instead of how
you're fixing it.

Thanks,
Pedro Alves
  

On Wednesday, January 29 2020, Pedro Alves wrote:

> On 1/29/20 7:54 PM, Sergio Durigan Junior wrote:
>> On Wednesday, January 29 2020, Pedro Alves wrote:
>
>>> Uppercase ARGC.  But putting
>>>
>>>  gdb_assert (argc > 0);
>>>
>>> at the top of the function instead as I originally suggested also works
>>> for me (tried current gcc master), which seems a bit better to me, as it
>>> covers both branches at once.  Did it not work for you?  This makes gcc see
>>> that the loops always run at least once.
>> 
>> Yes, this should work.
>> 
>> Updated patch below.
>
> The version you sent won't build for sure -- you're checking
> length instead of argc:
>
>  @@ -268,6 +268,11 @@ construct_inferior_arguments (int argc, char **argv)
>   {
>     char *result;
>   
>  +  /* argc should always be at least 1, but we double check this
>  +     here.  This is also needed to silence -Werror-stringop
>  +     warnings.  */
>  +  gdb_assert (length > 0);

Congrats, this is the new challenge called "How many interactions does
it take to get a simple patch accepted?"

Sorry about this brain fart.  I should slow down.

> This is OK with that fixed, but please update the commit log, here:
>
>  "The solution here is to explicit check that 'length' is greater than
>  0.  Since we're dealing with 'argc', I think it's pretty much
>  guaranteed that it's going to be at least 1."
>
> I'd go with:
>
>  The solution here is to assert that 'argc' is greater than
>  0 on entry, which makes GCC understand that the loops always
>  run at least once, and thus 'length' is always > 0.
>
> ... and also update the commit's subject.  I'd use
>
>  Fix -Werror-stringop error on infcmd.c:construct_inferior_arguments
>
> instead, to talk in terms of what you're fixing instead of how
> you're fixing it.

OK, I'll do those things *calmly* and put your name as the author of the
patch.

Thanks,