From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 1AA933850219 for ; Tue, 4 Apr 2023 13:58:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 1AA933850219 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca Received: from [10.0.0.170] (unknown [217.28.27.60]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B126C1E110; Tue, 4 Apr 2023 09:58:37 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1680616717; bh=vsoOluornU0eMEox40aPRRrkTw3YE43MU+FYEGTH+y4=; h=Date:Subject:To:References:From:In-Reply-To:From; b=PzF1dLxlP2t+hCxYgm4dmWBDV1x2vcYR+LB/IbFQ/diou10qyyvuWdP3n4Z8UEesA oOKqpbpE6KDY2XOHLwi06daJpRJucYhHVXaVHJZLAfT8LdUKBV5q4mESsg0Il96fjN aXwtf1nvipdf1dmqL2U3Iv93f2cVXsDtuhREncT8= Message-ID: <3d7197c2-424c-1458-93aa-d23fedac3d70@simark.ca> Date: Tue, 4 Apr 2023 09:58:37 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.1 Subject: Re: [PATCH] gdb: fix missing null-character when using value_cstring Content-Language: fr To: Andrew Burgess , gdb-patches@sourceware.org References: From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 4/3/23 17:49, Andrew Burgess via Gdb-patches wrote: > In PR gdb/21699 an issue was reported with the $_as_string convenience > function. It was observed that the string returned by this function, > when pushed into the inferior, was not null terminated. > > This was causing problems when using the result with GDB's printf > command, as this command relies on the string having been pushed into > the inferior and being null terminated. > > The bug includes a simple reproducer: > > #include > 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() gives a string that isn't NULL > terminated. */ > void > *malloc (size_t size) > { > if (size > sizeof (arena)) > return NULL; > return arena; > } > > int > main () > { > return 0; > } > > Then use this in a GDB session like this: > > $ gdb -q test > Reading symbols from /tmp/test... > (gdb) start > Temporary breakpoint 1 at 0x4004c8: file test.c, line 17. > Starting program: /tmp/test > > Temporary breakpoint 1, main () at test.c:17 > 17 return 0; > (gdb) printf "%s\n", $_as_string("hello") > "hello"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx > (gdb) quit > > The problem is with the GDB function value_cstring, or at least, how > this function is being used. > > When $_as_string is called we enter fnpy_call (python/py-function.c), > this calls into Python code. The Python code returns a result, which > will be a Python string, and then we call convert_value_from_python to > convert the Python string to a GDB value. > > In convert_value_from_python (python/py-value.c) we enter the > gdbpy_is_string case (as the result is a string), then we call > python_string_to_target_string, which returns a null terminated C > string. Next we then make this call: > > value = value_cstring (s.get (), strlen (s.get ()), > builtin_type_pychar); > > This passes the pointer to the first character 's.get ()' and the > length of the string 'strlen (s.get ())', however, this length does > not include the trailing null character. > > If we look at value_cstring (valops.c) we see that an array is created > using the passed in length, and characters are copied into the newly > allocated array value. This means we do not copy the strings trailing > null character, nor does value_cstring add a trailing null. > > Finally, when we use this array value with printf, GDB pushed the > array to the inferior, which mallocs some space based on the array > length, and then copies the array content to the inferior. > > As the array doesn't include a trailing null, non is written into the non -> none > inferior. So what we place into the inferior is not a C string, but > is actually an array of characters. > > When printf tries to print the value it starts at the address of the > first character and continues until it reaches a null. When that will > be is undefined, so we may end up printing random garbage. > > Now, ignore for a moment that the whole push an array to the inferior > just so we can fetch it in order to print it is clearly crazy. That's > a problem for another day I think. The important question here is: > should value_cstring include a null character or not. > > Given the function name include 'cstring', which I'm assuming means C > style string, I think that we should be including a trailing null. > > Given that, I see two possibilities, either value_cstring can always > add a trailing null, or value_cstring can assert that there is a > trailing null, and the caller is responsible for making sure that the > passed in length includes the null character. > > Given we're always passing from a C style string to begin with the > question is really, should the length being passed to value_cstring > include the null, or not include the null? > > The only place where we currently include the null in the passed > length is from c_string_operation::evaluate. Every other use of > value_cstring passes the length excluding the null. > > I was tempted to adjust c_string_operation::evaluate to exclude the > null, and then have value_cstring add a trailing null. However, this > does mean that if, in the future, a use is introduced that incorrectly > includes the trailing null in the passed length, then we are unlikely > to spot immediately - we'd instead create an array with two null > characters at the end.o You can always assert that the last character is not '\0'. > > Alternatively, if we change the requirements of value_cstring so that > we require the passed length includes the trailing null, then we can > assert that this is indeed the case within value_cstring. Any > incorrect uses in the future will be quickly spotted. > > So that's what I did, c_string_operation::evaluate is left unchanged, > but every other use of value_cstring is adjusted with a '+ 1' so that > we include the null within the length being passed. That sounds counterintuitive to me. With an API of style pointer + length, I don't expect the length to include the null terminator. It also unnecessarily forces the caller to have a null-terminated version of the string, which may not always be the case (you might want to call value_cstring on a subset of an existing string). I think that: struct value * value_cstring (const char *ptr, ssize_t len, struct type *char_type) should take a length excluding the null terminator, but a null terminator in the result (its job is to build a C string, and a C string requires a null terminator at the end). We can have the following overload, for convenience, for places that already have a C string but don't already know its length: struct value * value_cstring (const char *str, struct type *char_type) { return value_cstring (str, strlen (str), char_type); } > I've added a header comment to value_cstring (value.h) to describe the > requirements. > > Upon testing there were two tests that failed after this fix, > gdb.base/settings.exp and gdb.python/py-mi.exp. In both of these > cases we end up asking for the type of a character array allocated > through value_cstring. The length of this array has now increased by > one. Here's the previous behaviour: > > (gdb) set args abc > (gdb) p $_gdb_setting("args") > $1 = "abc" > (gdb) ptype $1 > type = char [3] > (gdb) > > And here's the modified behaviour: > > (gdb) set args abc > (gdb) p $_gdb_setting("args") > $1 = "abc" > (gdb) ptype $1 > type = char [4] > (gdb) > > Notice the type of $1 changed from an array of length 3 to an array of > length 4. However, I don't think this is a bad thing, consider: > > char lit[] = "zzz"; > int > main() > { > return 0; > } > > And in GDB: > > (gdb) ptype lit > type = char [4] > (gdb) > > The null character is considered part of the array, so I think the new > behaviour makes sense. Makes sense. > diff --git a/gdb/testsuite/gdb.base/cstring-exprs.c b/gdb/testsuite/gdb.base/cstring-exprs.c > new file mode 100644 > index 00000000000..8135edd97d4 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/cstring-exprs.c > @@ -0,0 +1,51 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2023 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include > +#include > + > +/* A memory area used as the malloc memory buffer. */ > + > +static char arena[] = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; > + > +/* Override malloc(). When GDB tries to push strings into the inferior we > + always return the same pointer to arena. This does mean we can't have > + multiple strings in use at the same time, but that's fine for our basic > + testing, and this is simpler than using dlsym. */ > + > +void > +*malloc (size_t size) The * is on the wrong line. Simon