Re: [PATCH 1/6] gdb: a number of fixes for .gdb_index parsing

[Simon Marchi <simark@simark.ca>]

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<cooked_index_entry *> 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 <label> is a CU label, created when the
> +	# CU itself was created.  The <length> is optional, and will usually
> +	# be the empty string, but can be set to a number to override the
> +	# automatically calculated length.
> +	variable _gdb_index_cus
> +	set _gdb_index_cus []
> +
> +	# A list of lists.  Each sub-list has the form: {name entries
> +	# offset}, where <name> is a string, entries is a list of CU
> +	# descriptors, and <offset> is usually the empty string, but can be
> +	# a number which overrides the offset from the CU vector to the
> +	# <name> string.  See _gdb_index_name for more details.

CU vector -> symbol table, I think.

Also, if you named it "name_offset" it would be a bit clearer than just
"offset".

> +	variable _gdb_index_names
> +	set _gdb_index_names []
> +
> +	# The value to encode as the main function language.  The
> +	# default value 0 is what the .gdb_index uses to indicate no
> +	# main language information is present.
> +	variable _gdb_index_main_lang
> +	set _gdb_index_main_lang 0
> +
> +	# The main function name.
> +	variable _gdb_index_main_name
> +	set _gdb_index_main_name ""
> +
> +	# The index into the constant pool's string table of the main name
> +	# string.  For testing this can sometimes be set to an invalid
> +	# value, so don't rely on this always pointing into the string
> +	# table.
> +	variable _gdb_index_main_name_idx
> +	set _gdb_index_main_name_idx -1
> +
> +	# Create an entry in the CU list within the index.  LABEL is created
> +	# when the CU itself is created (see the 'label' option for 'proc
> +	# cu').  The CU list also encodes the length of the CU.  Usually
> +	# this is done automatically, but LENGTH, which is a raw number, can
> +	# be used to override this for testing invalid lengths appearing in
> +	# the index.
> +	proc _gdb_index_cu { label {length ""} } {
> +	    variable _gdb_index_cus
> +	    lappend _gdb_index_cus [list $label $length]
> +	}
> +
> +	# Implement the 'name' command.  Record a string NAME into the
> +	# constant pool's string table, and use ENTRIES_LIST to generate a
> +	# CU vector in the constant pool's CU vectors table.
> +	#
> +	# The CU vector contains the offset of NAME in the string table.

Should this say:

The symbol table contains the offset of NAME in the constant pool.

?

> +	set num_names [llength $_gdb_index_names]
> +
> +	# Compute hash for each name and record the index of the main
> +	# function if we find a matching name.
> +	set name_idx 0
> +	set hashes []
> +	foreach entry $_gdb_index_names {
> +	    lassign $entry name entries_list
> +	    lappend hashes [_gdb_index_hash $name]
> +	    if { $_gdb_index_main_lang != 0 } {
> +		if { $_gdb_index_main_name eq $name } {

Can you join these two ifs together?

Claude tells me to tell you:

    Nit: dwarf.exp:4127: when main is given a raw offset,
    _gdb_index_main_name is "", so a name "" entry would overwrite
    _gdb_index_main_name_idx. Only check names when _gdb_index_main_name
    ne "".

... which I guess could happen in theory.  Might as well gate the main
name check on that.

Simon