From patchwork Sun Feb 26 13:20:08 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Matthew Malcomson X-Patchwork-Id: 19388 Received: (qmail 11375 invoked by alias); 26 Feb 2017 13:20:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 11356 invoked by uid 89); 26 Feb 2017 13:20:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.7 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f43.google.com Received: from mail-wm0-f43.google.com (HELO mail-wm0-f43.google.com) (74.125.82.43) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 26 Feb 2017 13:20:13 +0000 Received: by mail-wm0-f43.google.com with SMTP id v77so44006128wmv.1 for ; Sun, 26 Feb 2017 05:20:13 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to; bh=SpyRwzhVJQwX0MYcNLz0oB5r1BDyMUEtjC3TrejFoCM=; b=HcirkUAry+UUdsEk43THFT6DjNHCNBNlz1tf7ecocS94xzyb6N7/scVdRag47Avr1n KMOikIDSE2xWEY5hOOrX18rq1Q8kOiySpZWzOlELMC9yoWmx5SAfGViXFxg7N5TxXyfo esOmFIsvgSqHqxpmzNHGqqbbRNx9WmZnTQpLIXEJ8NRGjTwxCkIpMOO/gmWSldIQNpBb gIg/fe1EKeFoxW0N3VTCSTygQyiSlzcNcQF8kBwXBlqi1e2PGXUNQ/6qPCA/Kp6iTUKV jXNNDMh3ar4qgcMKErFbIYcfhCq9TMCshxNJbzT0Q25P2pP4hDdxGWnDLP4SLoGlXlCh H7fA== X-Gm-Message-State: AMke39l9LE0ttasuCutsqgEOfo+HWLtJBNFySeIrFIfu9VsQmBF3lCe10JAo0OSi6IvmkA== X-Received: by 10.28.140.130 with SMTP id o124mr10231791wmd.48.1488115211264; Sun, 26 Feb 2017 05:20:11 -0800 (PST) Received: from [10.0.0.57] (host86-170-201-9.range86-170.btcentralplus.com. [86.170.201.9]) by smtp.gmail.com with ESMTPSA id b51sm18234310wrd.39.2017.02.26.05.20.09 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 26 Feb 2017 05:20:10 -0800 (PST) Subject: Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` To: Simon Marchi References: <959cdc8e-1e54-a2e7-53d0-d80aaaea9ea8@gmail.com> <1d49ea752aae175256c0278bf3a999bc@polymtl.ca> <1cbe8b68-b592-825a-c662-56096ef0f795@gmail.com> <2da82ddc637e4d9fb61ee5b446a94c57@polymtl.ca> Cc: gdb-patches@sourceware.org From: Matthew Malcomson Message-ID: <8e018305-1691-1049-412f-7f668075bfd1@gmail.com> Date: Sun, 26 Feb 2017 13:20:08 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <2da82ddc637e4d9fb61ee5b446a94c57@polymtl.ca> I've attached the patch with correct formatting because my email client replaces tabs with spaces. I'll leave the changelog entries as you suggested. Thanks again, Matthew > Just make sure to put each change in the relevant ChangeLog, the one > "closest" to the change in the directory structure. For example, for > you change, I would do: > > gdb/ChangeLog: > > * python/py-value.c (convert_value_from_python): Consider terminating > NULL byte in string length. > > gdb/doc/ChangeLog: > > * gdb.texinfo (Convenience Variables for Tracepoints): Mention that > trace_func should not be used with output and not printf. > > gdb/testsuite/ChangeLog: > > * gdb.python/py-as-string.c (malloc): New function. > * gdb.python/py-as-string.exp (test_as_string): Test $_as_string on > a string with printf. > * gdb.python/py-mi.exp: Adjust array length. > >>> IIUC, the goal of overriding malloc is to ensure that the memory >>> return by malloc is not all zeroes, which would potentially hide the >>> bug? If that's right, you could instead write a wrapper for malloc >>> instead of a replacement. The wrapper would memset the allocated >>> buffer to 'x'es, for example. This way, it will be safer in case >>> there are many calls to malloc or calls with size > 51. >>> >>> See option #2 of this answer: http://stackoverflow.com/a/262481 >> >> Yes, that was the reason. I used this way because I read that gdb also >> worked on non-POSIX systems (windows especially) and thought having a >> working test on all systems would be preferred (though I didn't check >> that all systems support the testing framework). >> I believe that no other calls to malloc are made in the inferior for >> this test, and that this program isn't used anywhere else, so this >> limit of 51 bytes is never hit. >> I agree this is a bug waiting to happen, so I can accept if the >> alternate would be preferred, but I thought I'd mention my reasoning. > > That's a good justification too, I'm ok with either. > > commit 28312c70fcba81ef50a93ff52dde47230efc35cb Author: Matthew Malcomson Date: Sun Feb 26 13:10:09 2017 +0000 convert_value_from_python include terminating NULL When converting python strings to internal gdb Value strings, the NULL byte was initially left out, this can result in extra data from the inferior being printed when the resulting value is used with printf "%s\n", value diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 962325be3a..486b7899fb 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -13645,8 +13645,8 @@ The source file for the current trace snapshot. The name of the function containing @code{$tracepoint}. @end table -Note: @code{$trace_file} is not suitable for use in @code{printf}, -use @code{output} instead. +Note: @code{$trace_file} and @code{$trace_file} are not suitable for use in +@code{printf}, use @code{output} instead. Here's a simple example of using these convenience variables for stepping through all the trace snapshots and printing some of their diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c index eb3d307b19..c786f68865 100644 --- a/gdb/python/py-value.c +++ b/gdb/python/py-value.c @@ -1615,7 +1615,7 @@ convert_value_from_python (PyObject *obj) gdb::unique_xmalloc_ptr s = python_string_to_target_string (obj); if (s != NULL) - value = value_cstring (s.get (), strlen (s.get ()), + value = value_cstring (s.get (), strlen (s.get ()) + 1, builtin_type_pychar); } else if (PyObject_TypeCheck (obj, &value_object_type)) diff --git a/gdb/testsuite/gdb.python/py-as-string.c b/gdb/testsuite/gdb.python/py-as-string.c index de2e8a1951..e53f3a9b64 100644 --- a/gdb/testsuite/gdb.python/py-as-string.c +++ b/gdb/testsuite/gdb.python/py-as-string.c @@ -15,6 +15,8 @@ You should have received a copy of the GNU General Public License along with this program. If not, see . */ +#include + enum EnumType { ENUM_VALUE_A, ENUM_VALUE_B, @@ -22,6 +24,20 @@ enum EnumType { ENUM_VALUE_D, }; +static char arena[51] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; + +/* Override malloc() so value_coerce_to_target() gets a known pointer, and we + know we'll see an error if $_as_string() returns a string that isn't NULL + terminated. */ +void +*malloc (size_t size) +{ + if (size > sizeof (arena)) + return NULL; + + return arena; +} + static enum EnumType enum_valid = ENUM_VALUE_B; static enum EnumType enum_invalid = 20; diff --git a/gdb/testsuite/gdb.python/py-as-string.exp b/gdb/testsuite/gdb.python/py-as-string.exp index 0c44d5f174..e4625631c2 100644 --- a/gdb/testsuite/gdb.python/py-as-string.exp +++ b/gdb/testsuite/gdb.python/py-as-string.exp @@ -35,6 +35,13 @@ proc test_as_string { } { gdb_test "p \$_as_string(2)" "\"2\"" gdb_test "p \$_as_string(enum_valid)" "\"ENUM_VALUE_B\"" gdb_test "p \$_as_string(enum_invalid)" "\"20\"" + + # Test that the NULL character is included in the returned value. + gdb_test "printf \"%s\\n\", \$_as_string(\"hi\")" "\"hi\"" + # Quote once to define the string, and once for the regexp. + gdb_test "interpreter-exec mi '-var-create test * \$_as_string(\"Hello\")'" \ + "\\^done,name=\"test\",numchild=\"8\",value=\"\\\[8]\",type=\"char \\\[8]\",has_more=\"0\"" + gdb_test "interpreter-exec mi '-var-delete test'" "\\^done,ndeleted=\"1\"" } test_as_string diff --git a/gdb/testsuite/gdb.python/py-mi.exp b/gdb/testsuite/gdb.python/py-mi.exp index 736dc7a0d6..a5ad3f0f44 100644 --- a/gdb/testsuite/gdb.python/py-mi.exp +++ b/gdb/testsuite/gdb.python/py-mi.exp @@ -281,7 +281,7 @@ mi_create_dynamic_varobj nstype2 nstype2 1 \ "create nstype2 varobj" mi_list_varobj_children nstype2 { - { {nstype2.} {} 6 {char \[6\]} } + { {nstype2.} {} 7 {char \[7\]} } } "list children after setting exception flag" mi_create_varobj me me \