Re: [PATCH] gdb/dwarf2: Check for missing abbrev

[Simon Marchi <simark@simark.ca>]

On 2024-03-14 08:31, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> However, I'd like if we could analyze the problem a bit further to
> Simon> understand more precisely what happens, just to be sure that there isn't
> Simon> a more fundamental problem and we're not just papering over the problem.
> 
> The issue is corrupt DWARF -- the DIE specifies an abbrev that doesn't exist.

If it was an abbrev number that didn't exist, then peek_die_abbrev would
throw here:

  const abbrev_info *abbrev
    = reader.abbrev_table->lookup_abbrev (abbrev_number);
  if (!abbrev)
    {
      error (_("Dwarf Error: Could not find abbrev number %d in %s"
	       " at offset %s [in module %s]"),
	     abbrev_number, cu->per_cu->is_debug_types ? "TU" : "CU",
	     sect_offset_str (cu->header.sect_off), bfd_get_filename (abfd));
    }

If peek_die_abbrev returns nullptr, it's because it read abbrev number 0
specifically (which normally means "end of DIE list", but is unexpected
at this particular place).  So I'm just wondering what "kind" of
corruption causes it to be 0.  The concrete case I'm worried about is
the possibility that the file is truncated and we go read an offset that
is out of our buffer's bounds.  In that case, some bounds checking
should probably be added somehwere.  If the file is not truncated and
just contains a bunch of 0s where it shouldn't because debuginfod
crashed while writing it, then just erroring out in scan_attributes
(after the peek_die_abbrev call) is fine.

> Whether erroring here is the best result is harder to say.  For example,
> would it make more sense to terminate the scanning but still install any
> symbols seen before this point?
> 
> I tend to agree that this approach is ok, though.  If the file is
> corrupt it seems fine to lose a few symbols, or maybe even the whole
> thing.

I think that erroring out is fine, certainly better than crashing.

Simon