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

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

[Sergio Durigan Junior]

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..f468c788b6 100644
--- 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
+	 here.  This is also needed to silence -Werror-stringop
+	 warnings.  */
+      gdb_assert (length > 0);
+
       result = (char *) xmalloc (length);
       result[0] = '\0';
       for (i = 0; i < argc; ++i)
-- 
2.21.0

Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

[Pedro Alves]

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.)


> --- 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.

Thanks,
Pedro Alves

Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

[Sergio Durigan Junior]

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,

-- 
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

Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

[Sergio Durigan Junior]

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

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

[Pedro Alves]

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

Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

[Sergio Durigan Junior]

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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/

Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments

[Sergio Durigan Junior]

On Wednesday, January 29 2020, I wrote:

> 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.

Here's the patch I ended up pushing.

c47f70e2ce7b347aadbde873aae6c2df92c42180

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 c47f70e2ce7b347aadbde873aae6c2df92c42180 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 29 Jan 2020 12:53:55 -0500
Subject: [PATCH] Fix -Werror-stringop error on
 infcmd.c:construct_inferior_arguments

While testing a GCC 10 build of our git HEAD, Sergio 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 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.

Tested by rebuilding.

gdb/ChangeLog:
2020-01-29  Pedro Alves  <palves@redhat.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>

	* infcmd.c (construct_inferior_arguments): Assert that
	'argc' is greater than 0.

Change-Id: Ide8407cbedcb4921de1843a6a15bbcb7676c7d26
---
 gdb/ChangeLog | 6 ++++++
 gdb/infcmd.c  | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ee2ba1c38d..535249196a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2020-01-29  Pedro Alves  <palves@redhat.com>
+	    Sergio Durigan Junior  <sergiodj@redhat.com>
+
+	* infcmd.c (construct_inferior_arguments): Assert that
+	'argc' is greater than 0.
+
 2020-01-29  Luis Machado  <luis.machado@linaro.org>
 
 	* aarch64-tdep.c (BRK_INSN_MASK): Define to 0xffe0001f.
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index b44adca88d..62890bde2a 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 (argc > 0);
+
   if (startup_with_shell)
     {
 #ifdef __MINGW32__
-- 
2.21.0