This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments
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/