From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ++EfL5K112ohmDsAWB0awg (envelope-from ) for ; Tue, 20 Oct 2026 14:40:18 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1792521618; bh=o2fczlXapFLIElIuFzIkFQX5NAsobTVPHQthM51fdLc=; h=Date:Subject:To:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=iks9tL79uYUauIwn+pxoMkhOiAfBZGWJmY6RcFw5VzI7fKmA4x6nXX88XzrcV5f6f cK/7otakS0NLzgVxr3R+5nQ1PmDsc5bqrULniaNJq7IQvzefYF4oEgET+KEt1fa1Ab cAzoHOveicthlMuYWDAjoaZiI1kQhLT8EhaUUFcY= Received: by simark.ca (Postfix, from userid 112) id AD4801E051; Tue, 20 Oct 2026 14:40:18 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=gq/K4j5r; dkim-atps=neutral Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 58F861E04D for ; Tue, 20 Oct 2026 14:40:17 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id BBBB14A9F1DC for ; Tue, 20 Oct 2026 18:40:16 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BBBB14A9F1DC Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=gq/K4j5r Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 684364AA2996 for ; Tue, 20 Oct 2026 18:39:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 684364AA2996 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 684364AA2996 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1792521588; cv=none; b=c6B6tTdH44upSvbFQzrBYSeZJS1GhVswRdsJtR+2mmQsDsPbzhaSVOg9vfsfUd0CAiH3KSn4nUsRstRkfYfhV6CmAO9ThVKQwVQHZp0VUsGJf+GeLOwkXNe48EK/NbIChpGycb5tKVB3p2vpU74U+h4QzgZxxbz3h2E0bU2AYIQ= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1792521588; c=relaxed/simple; bh=o2fczlXapFLIElIuFzIkFQX5NAsobTVPHQthM51fdLc=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=XEwWkc1B6f8AIroyrbgfeOSQELRGn8RpOT55hPWJ30HXoZlovxuxx61T0LvO9nQ9lECgFUCV73zwIcFEZdRlSZVYoZl782odsmi+8X6NoMyjeXaa0zDjZDMcAdkqieYYJd3bi7fv1Ir36FxvbIxWWJAgeZY0GZv99yEdFFExh7o= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=gq/K4j5r DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 684364AA2996 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1792521587; bh=o2fczlXapFLIElIuFzIkFQX5NAsobTVPHQthM51fdLc=; h=Date:Subject:To:References:From:In-Reply-To:From; b=gq/K4j5ry4W+R2QSMgfFKoCg2Bo2Uuac894+ZEtnAQ+4dh0UnjxEO70hDPZZH9OJj NZteDmoH/QTMlA5uU/0XJ5pUKqDf929WPPQcUzBtqnYePTZT0h3ViXeBIy1EFew/H2 8vDZoir4ZLwOurNGZ+qhlbIzDoUFF0UCknC6b7QU= Received: by simark.ca (Postfix) id 989301E04D; Tue, 20 Oct 2026 14:39:46 -0400 (EDT) Message-ID: Date: Tue, 20 Oct 2026 14:39:46 -0400 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/6] gdb: a number of fixes for .gdb_index parsing To: Andrew Burgess , gdb-patches@sourceware.org References: Content-Language: fr From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org I focused on the non-test code (don't really have the time or mental energy to read nearly a thousand lines of spicy tcl code right now), but I also noted a few comments in the testsuite parts that came from Claude. On 10/18/26 4:50 PM, Andrew Burgess wrote: > I recently posted a patch which Simon pointed out during review caused > GDB to crash when running with cc-with-gdb-index board. The problem > was that I was now generating an invalid .gdb_index section. > > This got me thinking, rather than crashing, GDB should really be > rejecting the index as invalid. GDB should not treat the .gdb_index > as trusted input. Even if we take the position that GDB will always > generate valid .gdb_index sections, there's nothing to say that some > other tool might try to generate .gdb_index sections, and do it wrong. > We also write the .gdb_index into the index cache, disk corruption > could result in an invalid .gdb_index being read back in later. > > This commit includes a number of fixes which I detail below, but the > most important thing here is the addition of the 'gdb_index' proc to > testsuite/lib/dwarf.exp. This proc started as a copy of 'proc > debug_names' but updated to emit .gdb_index sections. This proc has > been further modified to allow invalid indexes to be generated. I > believe all of the features I've added so far are documented in the > comments, so I'll not repeat it all here, but the general idea is to > allow every offset to be overridden to allow for invalid offsets to be > encoded into the .gdb_index. There are also options to allow parts of > the index to be elided completely. > > Not every override feature of 'proc gdb_index' has a matching test > right now. If I'd done that then I was worried that the GDB side > fixes would be too large for a single patch. Further patches in this > series will make use of the currently unused features. > > Currently, in dwarf2/read-gdb-index.c, I have the following fixes: > > In mapped_gdb_index::build_name_components: > > (1) If the name index in the .gdb_index's symbol table is out of > bounds of the constant pool then ignore this symbol table entry. > > (2) Add an assert that the components list will contain at least > one entry, even an empty string will result in a single > component, the empty string. > > (3) When processing the CU vector, if the declared CU vector length > is larger than the constant pool then ignore this CU vector. > > (4) If we end up with an empty component string after splitting a > name from the index, then don't try to call .back() on the empty > string. > > In read_gdb_index_from_buffer: > > (5) If the symbol table is not empty, but the constant pool doesn't > end with a null character then reject this index. A non-empty > symbol table means that there must be strings in the constant > pool's string table. The strings in this table are > null-terminated. And the string table is at the end of the > constant pool. Therefore, the last byte in the constant pool > must be a null character. > > If this is not the case then reading strings from the constant > pool might not be null-terminated, and might read outside the > constant pool. Claude pointed out that according to the doc / spec of the format, nothing would prevent a producer to use non-zero padding at the end of the section... We can disregard that and assume it doesn't happen in real life, but I wanted to point it out because I believe it's technically true. Or we could change the spec of the format to say that any padding must be made of '\0' bytes. > > Placing this check here means that we can later on assume that > any string read from the constant pool is null-terminated > without reading outside the constant pool. > > In mapped_gdb_index::set_main_name: > > (6) If the main function name is outside the bounds of the constant > pool then warn, and ignore the main function name. > > (7) Assert that the constant pool is null-terminated, see (5) above. > --- > gdb/dwarf2/read-gdb-index.c | 85 ++- > .../gdb.dwarf2/gdb-index-corrupted.c | 47 ++ > .../gdb.dwarf2/gdb-index-corrupted.exp | 367 ++++++++++++ > gdb/testsuite/lib/dwarf.exp | 531 ++++++++++++++++++ > 4 files changed, 1022 insertions(+), 8 deletions(-) > create mode 100644 gdb/testsuite/gdb.dwarf2/gdb-index-corrupted.c > create mode 100644 gdb/testsuite/gdb.dwarf2/gdb-index-corrupted.exp > > diff --git a/gdb/dwarf2/read-gdb-index.c b/gdb/dwarf2/read-gdb-index.c > index 324984c0de2..86512c4ab9c 100644 > --- a/gdb/dwarf2/read-gdb-index.c > +++ b/gdb/dwarf2/read-gdb-index.c > @@ -187,8 +187,9 @@ struct mapped_gdb_index > const char *symbol_name_at (offset_type idx, > dwarf2_per_objfile *per_objfile) const > { > - return (const char *) (this->constant_pool.data () > - + symbol_name_index (idx)); > + offset_type name_index = symbol_name_index (idx); > + gdb_assert (name_index < this->constant_pool.size ()); > + return (const char *) (this->constant_pool.data () + name_index); > } > > size_t symbol_name_count () const > @@ -218,6 +219,17 @@ mapped_gdb_index::build_name_components (dwarf2_per_objfile *per_objfile) > if (this->symbol_name_slot_invalid (idx)) > continue; > > + /* If the name offset is out of range for the constant pool then > + we cannot get a valid string. We just skip this symbol. */ > + if (this->symbol_name_index (idx) >= this->constant_pool.size ()) > + { > + complaint (_(".gdb_index entry %s has bad name offset %s [in module %s]"), Line slightly too long. > + pulongest (idx), > + pulongest (this->symbol_name_index (idx)), > + objfile_name (per_objfile->objfile)); > + continue; > + } > + > const char *name = this->symbol_name_at (idx, per_objfile); > > /* This code only knows how to break apart components of C++ > @@ -256,9 +268,25 @@ mapped_gdb_index::build_name_components (dwarf2_per_objfile *per_objfile) > lang = (components.size () == 1) ? language_unknown : language_go; > } > > + /* Even if NAME was the empty string then COMPONENTS will contain one > + entry, the empty string. */ > + gdb_assert (components.size () > 0); > + > std::vector these_entries; > offset_view vec (constant_pool.slice (symbol_vec_index (idx))); > offset_type vec_len = vec[0]; > + > + /* Bounds check VEC_LEN against the CONSTANT_POOL. */ > + if (vec_len >= vec.size ()) > + { > + /* The vector count would exceed the available space in the > + constant pool. This is a corrupt .gdb_index. We could > + treat everything up to the end of the constant pool as a > + valid vector, but we have no reason to think that's not > + also wrong. Instead just pretend this vector is empty. */ > + vec_len = 0; Just wondering, why set vec_len to 0, instead of complaint + continue? > + } Perhaps work for a subsequent patch, but maybe you (or I) could reorganize this so that the bounds check is done before creating the view, such that `vec` has exactly the right length (VEC_LEN elements). I think that right now it's a view from the start of the vector until the end of the constant pool. The VEC view could also exclude the length itself, and then the for loop just below could become a simple ranged for loop. No need to change your patch, it is an improvement already. > + > for (offset_type vec_idx = 0; vec_idx < vec_len; ++vec_idx) > { > offset_type cu_index_and_attrs = vec[vec_idx + 1]; > @@ -347,9 +375,15 @@ mapped_gdb_index::build_name_components (dwarf2_per_objfile *per_objfile) > if (components.size () > 1) > { > std::string_view penultimate = components[components.size () - 2]; > - std::string_view prefix (name, &penultimate.back () + 1 - name); > - > - need_parents.emplace_back (prefix, std::move (these_entries)); > + /* When parsing the DIE, GDB strips out entries with an empty name, > + so in a well formed index PENULTIMATE should never be empty. > + But the index is external data, so we cannot assume it is well > + formed, ignore any empty prefixes. */ > + if (penultimate.size () > 0) > + { > + std::string_view prefix (name, &penultimate.back () + 1 - name); > + need_parents.emplace_back (prefix, std::move (these_entries)); > + } Given this recent fix: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8f7fb58377a9a50c668190a4a740a4078355076d Is it still possible for split_name to return an empty component? For the DOT_STYLE style I think it's no longer possible, but maybe it is for the CXX style. If some invalid input to split_name results in an empty component, then maybe we should change split_name to not do that instead. It would be consistent with my fix. If there is no legitimate case for split_name to return an empty component, then here we could assert that the component is not empty. When thinking about that (when I wrote the fix above), I was wondering if maybe splitting "::foo::bar" should yield an empty first component, The presence of that empty component could be a sign that the name was "fully qualified", meaning it started with "::". But as far as I could tell, no caller relied on that, so splitting "::foo::bar" could safely return {"foo", "bar"}, I think. > } > } > > @@ -502,6 +536,25 @@ read_gdb_index_from_buffer (const char *filename, > symbol_table)); > } > > + if (!map->symbol_table.empty ()) > + { > + gdb_assert (!map->constant_pool.empty ()); > + > + if (map->constant_pool[map->constant_pool.size () - 1] != '\0') Hmm, std::span has a back() method, should we add it to array_view? > + { > + static bool warning_printed = false; Not sure I would use a static flag here (which means we will print the warning just once for the whole GDB session, even if you load multiple objfiles that have the same problem). And it's not like this case actually happens in real life... I would just unconditionally print the warning. > + if (!warning_printed) > + { > + warning (_("skipping .gdb_index in %ps, null byte at end of " > + "constant pool is missing"), > + styled_string (file_name_style.style (), > + filename)); > + warning_printed = true; > + } > + return false; > + } > + } Can you add a comment to tell why this last null byte check is useful? Like you explained in the commit message. > + > return true; > } > > @@ -707,10 +760,26 @@ mapped_gdb_index::set_main_name (dwarf2_per_objfile *per_objfile) > } > ptr += 4; > > + const ULONGEST name_offset > + = extract_unsigned_integer (ptr, sizeof (offset_type), BFD_ENDIAN_LITTLE); > + > + if (name_offset >= this->constant_pool.size ()) > + { > + complaint (_(".gdb_index shortcut table has invalid main name offset %s"), > + pulongest (name_offset)); Maybe add the "in module %" part to the message? > + return; > + } > + > + /* If the symbol table is empty then we ignore the .gdb_index in > + dwarf2_read_gdb_index. If the symbol table is not empty, and doesn't > + end in a '\0' byte then we also ignore the .gdb_index in > + read_gdb_index_from_buffer. What this means is that, by the time we I wonder if this last sentence means to say "if the constant pool doesn't end in a '\0' byte", rather than the symbol table. > + get here, we know that the constant pool ends with '\0' and so the > + MAIN_NAME will be a correctly terminated C style string. */ > + gdb_assert ((this->constant_pool)[(this->constant_pool).size () - 1] > + == '\0'); I'm not sure this assert is really needed though... like why do it here but not in build_name_components where we read all the symbols name? It seems a bit random. (and if you leave if there: the parentheses around this->constant_pool are redundant, both times, but really it would be nicer with a back() method) > + # Check the expected warning was emitted while loading the > + # executable. The GDB_FILE_CMD_MSG global was set by the call to > + # gdb_load above. > + if { $expect_warning_re ne "" } { > + gdb_assert { [regexp $expect_warning_re $::gdb_file_cmd_msg] } \ > + "index rejected with expected warning" In some cases the index is accepted with a warning, so the test name should maybe be just "got expected warning". > + # A list of two entry lists describing CUs. Each sub-list has the > + # form {label length}. The