Message ID | 20200302024616.1049417-1-sergiodj@redhat.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 81938 invoked by alias); 2 Mar 2020 02:46:28 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 81930 invoked by uid 89); 2 Mar 2020 02:46:28 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-16.7 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=2020-03-02, liberty, teststring 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; Mon, 02 Mar 2020 02:46:27 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583117185; 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=i2+v37PmFs5ZKmpcoHtNG7zYhylPH5axwOw9NDdXw+I=; b=QhD8E3gsj6RbDF18rva8ct13d8V04fHriDvhV6+6uSaZQWsfCC6nXHDn8R10jBEUh1qGLV lLS2Q+gbzfK6/PS3HzrHRqpoo3PQDxJLd17pRToFS6ROmdNm/HBtFAFLo5IQc7M85v4TeL /A32weoepBO1kGWSITgW8yUI12Z27nA= 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-494-gTjxd03zPJmKAYEhVL3kiA-1; Sun, 01 Mar 2020 21:46:23 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 0921A18C35A0; Mon, 2 Mar 2020 02:46:23 +0000 (UTC) Received: from psique.yyz.redhat.com (unused-10-15-17-54.yyz.redhat.com [10.15.17.54]) by smtp.corp.redhat.com (Postfix) with ESMTP id F27F492D20; Mon, 2 Mar 2020 02:46:19 +0000 (UTC) From: Sergio Durigan Junior <sergiodj@redhat.com> To: GDB Patches <gdb-patches@sourceware.org> Cc: Philippe Waroquiers <philippe.waroquiers@skynet.be>, Pedro Alves <palves@redhat.com>, Sergio Durigan Junior <sergiodj@redhat.com> Subject: [PATCH] Fix printf of a convenience variable holding an inferior address Date: Sun, 1 Mar 2020 21:46:16 -0500 Message-Id: <20200302024616.1049417-1-sergiodj@redhat.com> In-Reply-To: <20190610211622.15237-1-philippe.waroquiers@skynet.be> References: <20190610211622.15237-1-philippe.waroquiers@skynet.be> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes |
Commit Message
Sergio Durigan Junior
March 2, 2020, 2:46 a.m. UTC
Back at: commit 1f6f6e21fa86dc3411a6498608f32e9eb24b7851 Author: Philippe Waroquiers <philippe.waroquiers@skynet.be> Date: Mon Jun 10 21:41:51 2019 +0200 Ensure GDB printf command can print convenience var strings without a target. GDB was extended in order to allow the printing of convenience variables that are strings without a target. However, this introduced a regression that hasn't been caught by our testsuite (because there were no tests for it). The problem happens when we try to print a convenience variable that holds the address of a string in the inferior. The following two-liners can reproduce the issue: $ echo -e 'int main(){const char a[]="test";return 0;}' | gcc -x c - -O0-g3 $ ./gdb/gdb --data-directory ./gdb/data-directory -q ./a.out -ex 'start' -ex 'set $x = (const char *) (&a[0] + 2)' -ex 'printf "%s\n", $x' After some investigation, I found that the problem happens on printcmd.c:printf_c_string. In the case above, we're taking the first branch of the 'if' condition, which assumes that there will be a value to be printed at "value_contents (value)". There isn't. We actually need to obtain the address that the variable points to, and read the contents from memory. It seems to me that we should avoid this branch if the TYPE_CODE of "value_type (value)" is TYPE_CODE_PTR (i.e., a pointer to the inferior's memory). This is what this patch does. I took the liberty to extend the current testcase under gdb.base/printcmds.exp and create a test that exercises this scenario. No regressions have been found on Buildbot. gdb/ChangeLog: 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> * printcmd.c (print_c_string): Check also for TYPE_CODE_PTR when verifying if dealing with a convenience variable. gdb/testsuite/ChangeLog: 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> * gdb.base/printcmds.exp: Add test to verify printf of a variable holding an address. --- gdb/printcmd.c | 3 ++- gdb/testsuite/gdb.base/printcmds.exp | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-)
Comments
* Sergio Durigan Junior <sergiodj@redhat.com> [2020-03-01 21:46:16 -0500]: > Back at: > > commit 1f6f6e21fa86dc3411a6498608f32e9eb24b7851 > Author: Philippe Waroquiers <philippe.waroquiers@skynet.be> > Date: Mon Jun 10 21:41:51 2019 +0200 > > Ensure GDB printf command can print convenience var strings without a target. > > GDB was extended in order to allow the printing of convenience > variables that are strings without a target. However, this introduced > a regression that hasn't been caught by our testsuite (because there > were no tests for it). > > The problem happens when we try to print a convenience variable that > holds the address of a string in the inferior. The following > two-liners can reproduce the issue: > > $ echo -e 'int main(){const char a[]="test";return 0;}' | gcc -x c - -O0-g3 > $ ./gdb/gdb --data-directory ./gdb/data-directory -q ./a.out -ex 'start' -ex 'set $x = (const char *) (&a[0] + 2)' -ex 'printf "%s\n", $x' > > After some investigation, I found that the problem happens on > printcmd.c:printf_c_string. In the case above, we're taking the first > branch of the 'if' condition, which assumes that there will be a value > to be printed at "value_contents (value)". There isn't. We actually > need to obtain the address that the variable points to, and read the > contents from memory. > > It seems to me that we should avoid this branch if the TYPE_CODE of > "value_type (value)" is TYPE_CODE_PTR (i.e., a pointer to the > inferior's memory). This is what this patch does. > > I took the liberty to extend the current testcase under > gdb.base/printcmds.exp and create a test that exercises this scenario. > > No regressions have been found on Buildbot. > > gdb/ChangeLog: > 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> > > * printcmd.c (print_c_string): Check also for TYPE_CODE_PTR > when verifying if dealing with a convenience variable. > > gdb/testsuite/ChangeLog: > 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> > > * gdb.base/printcmds.exp: Add test to verify printf of a > variable holding an address. LGTM. Thanks, Andrew > --- > gdb/printcmd.c | 3 ++- > gdb/testsuite/gdb.base/printcmds.exp | 8 ++++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index 797041484e..78d8d3d81e 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -2260,7 +2260,8 @@ printf_c_string (struct ui_file *stream, const char *format, > { > const gdb_byte *str; > > - if (VALUE_LVAL (value) == lval_internalvar > + if (TYPE_CODE (value_type (value)) != TYPE_CODE_PTR > + && VALUE_LVAL (value) == lval_internalvar > && c_is_string_type_p (value_type (value))) > { > size_t len = TYPE_LENGTH (value_type (value)); > diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp > index bd2afc8696..c87a1517f0 100644 > --- a/gdb/testsuite/gdb.base/printcmds.exp > +++ b/gdb/testsuite/gdb.base/printcmds.exp > @@ -1039,6 +1039,14 @@ gdb_test_no_output "set may-call-functions off" > test_printf_convenience_var "with target, may-call-functions off" > gdb_test_no_output "set may-call-functions on" > > +# Test printf of a variable that holds the address to a substring in > +# the inferior. This test will not work without a target. > +gdb_test_no_output "set var \$test_substr = \(char \*\) \(&teststring\[0\] + 4\)" \ > + "set \$test_substr var" > +gdb_test "printf \"test_substr val = %s\\n\", \$test_substr" \ > + "test_substr val = string contents" \ > + "print \$test_substr" > + > test_integer_literals_accepted > test_integer_literals_rejected > test_float_accepted > -- > 2.24.1 >
On Tuesday, March 03 2020, Andrew Burgess wrote: > * Sergio Durigan Junior <sergiodj@redhat.com> [2020-03-01 21:46:16 -0500]: > >> Back at: >> >> commit 1f6f6e21fa86dc3411a6498608f32e9eb24b7851 >> Author: Philippe Waroquiers <philippe.waroquiers@skynet.be> >> Date: Mon Jun 10 21:41:51 2019 +0200 >> >> Ensure GDB printf command can print convenience var strings without a target. >> >> GDB was extended in order to allow the printing of convenience >> variables that are strings without a target. However, this introduced >> a regression that hasn't been caught by our testsuite (because there >> were no tests for it). >> >> The problem happens when we try to print a convenience variable that >> holds the address of a string in the inferior. The following >> two-liners can reproduce the issue: >> >> $ echo -e 'int main(){const char a[]="test";return 0;}' | gcc -x c - -O0-g3 >> $ ./gdb/gdb --data-directory ./gdb/data-directory -q ./a.out -ex 'start' -ex 'set $x = (const char *) (&a[0] + 2)' -ex 'printf "%s\n", $x' >> >> After some investigation, I found that the problem happens on >> printcmd.c:printf_c_string. In the case above, we're taking the first >> branch of the 'if' condition, which assumes that there will be a value >> to be printed at "value_contents (value)". There isn't. We actually >> need to obtain the address that the variable points to, and read the >> contents from memory. >> >> It seems to me that we should avoid this branch if the TYPE_CODE of >> "value_type (value)" is TYPE_CODE_PTR (i.e., a pointer to the >> inferior's memory). This is what this patch does. >> >> I took the liberty to extend the current testcase under >> gdb.base/printcmds.exp and create a test that exercises this scenario. >> >> No regressions have been found on Buildbot. >> >> gdb/ChangeLog: >> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> >> >> * printcmd.c (print_c_string): Check also for TYPE_CODE_PTR >> when verifying if dealing with a convenience variable. >> >> gdb/testsuite/ChangeLog: >> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> >> >> * gdb.base/printcmds.exp: Add test to verify printf of a >> variable holding an address. > > LGTM. Thanks, pushed: 7b973adce2b486518d3150db257b179e1b9a5d33
On Tuesday, March 03 2020, I wrote: > On Tuesday, March 03 2020, Andrew Burgess wrote: > >> * Sergio Durigan Junior <sergiodj@redhat.com> [2020-03-01 21:46:16 -0500]: >> >>> Back at: >>> >>> commit 1f6f6e21fa86dc3411a6498608f32e9eb24b7851 >>> Author: Philippe Waroquiers <philippe.waroquiers@skynet.be> >>> Date: Mon Jun 10 21:41:51 2019 +0200 >>> >>> Ensure GDB printf command can print convenience var strings without a target. >>> >>> GDB was extended in order to allow the printing of convenience >>> variables that are strings without a target. However, this introduced >>> a regression that hasn't been caught by our testsuite (because there >>> were no tests for it). >>> >>> The problem happens when we try to print a convenience variable that >>> holds the address of a string in the inferior. The following >>> two-liners can reproduce the issue: >>> >>> $ echo -e 'int main(){const char a[]="test";return 0;}' | gcc -x c - -O0-g3 >>> $ ./gdb/gdb --data-directory ./gdb/data-directory -q ./a.out -ex 'start' -ex 'set $x = (const char *) (&a[0] + 2)' -ex 'printf "%s\n", $x' >>> >>> After some investigation, I found that the problem happens on >>> printcmd.c:printf_c_string. In the case above, we're taking the first >>> branch of the 'if' condition, which assumes that there will be a value >>> to be printed at "value_contents (value)". There isn't. We actually >>> need to obtain the address that the variable points to, and read the >>> contents from memory. >>> >>> It seems to me that we should avoid this branch if the TYPE_CODE of >>> "value_type (value)" is TYPE_CODE_PTR (i.e., a pointer to the >>> inferior's memory). This is what this patch does. >>> >>> I took the liberty to extend the current testcase under >>> gdb.base/printcmds.exp and create a test that exercises this scenario. >>> >>> No regressions have been found on Buildbot. >>> >>> gdb/ChangeLog: >>> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> >>> >>> * printcmd.c (print_c_string): Check also for TYPE_CODE_PTR >>> when verifying if dealing with a convenience variable. >>> >>> gdb/testsuite/ChangeLog: >>> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> >>> >>> * gdb.base/printcmds.exp: Add test to verify printf of a >>> variable holding an address. >> >> LGTM. > > Thanks, pushed: > > 7b973adce2b486518d3150db257b179e1b9a5d33 BTW, this also affects 9.1 (in fact, that's where I found the bug). I can create a tracking bug and backport it to the branch if the issue and fix are deemed important enough. Thanks,
Hi Sergio, > >>> gdb/ChangeLog: > >>> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> > >>> > >>> * printcmd.c (print_c_string): Check also for TYPE_CODE_PTR > >>> when verifying if dealing with a convenience variable. > >>> > >>> gdb/testsuite/ChangeLog: > >>> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> > >>> > >>> * gdb.base/printcmds.exp: Add test to verify printf of a > >>> variable holding an address. > >> > >> LGTM. > > > > Thanks, pushed: > > > > 7b973adce2b486518d3150db257b179e1b9a5d33 > > BTW, this also affects 9.1 (in fact, that's where I found the bug). I > can create a tracking bug and backport it to the branch if the issue and > fix are deemed important enough. Do you know if this issue is a regression, compared to previous released versions? In terms of putting it in 9.2, I think we could do it; the patch does look safe to me.
On Wednesday, March 04 2020, Joel Brobecker wrote: > Hi Sergio, > >> >>> gdb/ChangeLog: >> >>> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> >> >>> >> >>> * printcmd.c (print_c_string): Check also for TYPE_CODE_PTR >> >>> when verifying if dealing with a convenience variable. >> >>> >> >>> gdb/testsuite/ChangeLog: >> >>> 2020-03-02 Sergio Durigan Junior <sergiodj@redhat.com> >> >>> >> >>> * gdb.base/printcmds.exp: Add test to verify printf of a >> >>> variable holding an address. >> >> >> >> LGTM. >> > >> > Thanks, pushed: >> > >> > 7b973adce2b486518d3150db257b179e1b9a5d33 >> >> BTW, this also affects 9.1 (in fact, that's where I found the bug). I >> can create a tracking bug and backport it to the branch if the issue and >> fix are deemed important enough. > > Do you know if this issue is a regression, compared to previous > released versions? Hey Joel, Yeah, this is a regression. GDB 8.3 worked fine. > In terms of putting it in 9.2, I think we could do it; the patch > does look safe to me. Cool. So, is the process the same (now that we're using a different versioning scheme)? Do I just open a bug again 9.1 and push the backported patch to the branch? Thanks,
diff --git a/gdb/printcmd.c b/gdb/printcmd.c index 797041484e..78d8d3d81e 100644 --- a/gdb/printcmd.c +++ b/gdb/printcmd.c @@ -2260,7 +2260,8 @@ printf_c_string (struct ui_file *stream, const char *format, { const gdb_byte *str; - if (VALUE_LVAL (value) == lval_internalvar + if (TYPE_CODE (value_type (value)) != TYPE_CODE_PTR + && VALUE_LVAL (value) == lval_internalvar && c_is_string_type_p (value_type (value))) { size_t len = TYPE_LENGTH (value_type (value)); diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp index bd2afc8696..c87a1517f0 100644 --- a/gdb/testsuite/gdb.base/printcmds.exp +++ b/gdb/testsuite/gdb.base/printcmds.exp @@ -1039,6 +1039,14 @@ gdb_test_no_output "set may-call-functions off" test_printf_convenience_var "with target, may-call-functions off" gdb_test_no_output "set may-call-functions on" +# Test printf of a variable that holds the address to a substring in +# the inferior. This test will not work without a target. +gdb_test_no_output "set var \$test_substr = \(char \*\) \(&teststring\[0\] + 4\)" \ + "set \$test_substr var" +gdb_test "printf \"test_substr val = %s\\n\", \$test_substr" \ + "test_substr val = string contents" \ + "print \$test_substr" + test_integer_literals_accepted test_integer_literals_rejected test_float_accepted