From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 55276 invoked by alias); 29 Jan 2020 20:24:55 -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 55140 invoked by uid 89); 29 Jan 2020 20:24:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.4 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,TRACKER_ID autolearn=ham version=3.3.1 spammy= X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (205.139.110.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Jan 2020 20:24:44 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1580329479; 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=GOOCMR8qddFp5Fv8vwY83ybXDdO45yxc9q60GAEgUxo=; b=aJUTKhJRroJokTfDSEZU++ugtKAAh9XGEhBgysrvSrfdv0oKEAofjzdinLJ45uHbHM2iLD wMZTG+metTw3c8jWa2j7a1DrzNoz3ywjBfYptk3Lh2m94jdd1/8i+rHR0VV9BjC/GlRCpc y+u+SYA7NtPAsALQ/xBsX5xv7A/fkJw= 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-290-54sHiGAdNKOYGYIvyNTt6w-1; Wed, 29 Jan 2020 15:24:32 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CCD42DB63 for ; Wed, 29 Jan 2020 20:24:31 +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 9DFB65DA7B; Wed, 29 Jan 2020 20:24:29 +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> <87lfpqrqcg.fsf@redhat.com> <9d7548f2-68e5-4b58-2fc2-9745b50e1dd4@redhat.com> <87ftfym2yl.fsf@redhat.com> Date: Wed, 29 Jan 2020 23:25:00 -0000 In-Reply-To: <87ftfym2yl.fsf@redhat.com> (Sergio Durigan Junior's message of "Wed, 29 Jan 2020 15:18:10 -0500") Message-ID: <87blqmm2o2.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/msg00960.txt.bz2 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 gc= c see >>>> that the loops always run at least once. >>>=20 >>> Yes, this should work. >>>=20 >>> 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 **arg= v) >> { >> char *result; >>=20=20=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); > > 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, --=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 c47f70e2ce7b347aadbde873aae6c2df92c42180 Mon Sep 17 00:00:00 2001 From: Pedro Alves 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, 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 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 Sergio Durigan Junior * 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 + Sergio Durigan Junior + + * infcmd.c (construct_inferior_arguments): Assert that + 'argc' is greater than 0. + 2020-01-29 Luis Machado =20 * 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; =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 (argc > 0); + if (startup_with_shell) { #ifdef __MINGW32__ --=20 2.21.0