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

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

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