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

[Sergio Durigan Junior <sergiodj@redhat.com>]

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