From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42476 invoked by alias); 29 Jan 2020 19:54:19 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 42467 invoked by uid 89); 29 Jan 2020 19:54:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=encrypted X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (207.211.31.81) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Jan 2020 19:54:17 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580327655; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=rS15flu00H9MrQOUBk9N04EniQRvzT50+nW8ZCaSqfU=; b=Pv+GSJ6iLEfDvlI1Eu4zEsN05/8HtGUjF1uoKxzFkH8YLgU57mYGyDkWBygvUtAf1M9fl9 mtYlD5KEnnY8pheEmfv5ksED79bTtNeMjic6WGZUogRli9E55UjbLfQc0qmLugmp/8R/W4 wbpjcBlVxflDxBWZDgdr71IREaG46Kk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-141-0Za_JUr2MGCBNUGvHkTxcw-1; Wed, 29 Jan 2020 14:54:09 -0500 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3AEC11006476 for ; Wed, 29 Jan 2020 19:54:08 +0000 (UTC) Received: from localhost (unused-10-15-17-196.yyz.redhat.com [10.15.17.196]) by smtp.corp.redhat.com (Postfix) with ESMTP id D9D0F619E2; Wed, 29 Jan 2020 19:54:07 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches Subject: Re: [PATCH] Assert that 'length' > 0 on infcmd.c:construct_inferior_arguments References: <20200129175943.1035-1-sergiodj@redhat.com> <640bcf24-b8c3-3c96-0ea5-2efdc29f039a@redhat.com> Date: Wed, 29 Jan 2020 19:58:00 -0000 In-Reply-To: <640bcf24-b8c3-3c96-0ea5-2efdc29f039a@redhat.com> (Pedro Alves's message of "Wed, 29 Jan 2020 18:44:48 +0000") Message-ID: <87lfpqrqcg.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00953.txt.bz2 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: >>=20 >> ../../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=3Dstringop-overflow=3D] >> 369 | result[0] =3D '\0'; >> | ~~~~~~~~~~^~~~~~ >> ../../gdb/infcmd.c:368:33: note: at offset 0 to an object with size 0 al= located by 'xmalloc' here >> 368 | result =3D (char *) xmalloc (length); >> | ~~~~~~~~^~~~~~~~ >>=20 > >> 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_argumen= ts, > which does: > > const char * > get_inferior_args (void) > { > if (current_inferior ()->argc !=3D 0) > {=20=20=20=20=20=20 > char *n; > > n =3D 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 +=3D strlen (argv[i]) + 1; >> } >>=20=20 >> + /* 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 s= ee > that the loops always run at least once. Yes, this should work. Updated patch below. Thanks, --=20 Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ =46rom 6c746d24ccf13a56b9164fb241a4091223f1f706 Mon Sep 17 00:00:00 2001 From: Sergio Durigan Junior 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, ch= ar**)': ../../gdb/infcmd.c:369:17: error: writing 1 byte into a region of size 0 [-= Werror=3Dstringop-overflow=3D] 369 | result[0] =3D '\0'; | ~~~~~~~~~~^~~~~~ ../../gdb/infcmd.c:368:33: note: at offset 0 to an object with size 0 alloc= ated by 'xmalloc' here 368 | result =3D (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 * 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; =20 + /* 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__ --=20 2.21.0