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

[Simon Marchi <simark@simark.ca>]

On 2024-03-14 13:45, Aaron Merey wrote:
> On Wed, Mar 13, 2024 at 9:47 PM Simon Marchi <simark@simark.ca> wrote:
>>
>> We generally don't make functions error out like this if they are passed
>> bad parameters (we can use gdb_assert for that if we really want to).
>> It's up to the callers to respect the contract and not pass nullptr
>> where nullptr is not expected.  So I would instead suggest to add the
>> nullptr check in the caller (in scan_attributes itself, after the
>> peek_die_abbrev call lower).
> 
> Moving the nullptr check after peek_die_abbrev is fine, but replacing
> the throw with 'return nullptr' doesn't fix the crash.  It looks like
> scan_attributes currently cannot return nullptr without triggering
> a crash.

I was not suggesting to make scan_attributes return nullptr, throwing an
error (with error() as Tom pointed out it's fine).  I was just
suggesting doing it in the caller rather than in the callee.

>> However, I'd like if we could analyze the problem a bit further to
>> understand more precisely what happens, just to be sure that there isn't
>> a more fundamental problem and we're not just papering over the problem.
>> I added instructions on the bug that should help you reproduce.
>>
>> What I see is that read_unsigned_leb128 in peek_die_abbrev unexpectedly
>> returns 0.  Is it because the corrupted file contains zeros where it
>> shouldn't?  Or is the file truncated and we are reading past the end of
>> the buffer, and there happens to be a zero there?
> 
> The corrupt debuginfo that the reporter used to reproduce this [1] is the
> same size as the non-corrupt version.  A diff of hex dumps of the
> corrupt and non-corrupt versions of the debuginfo show that the corrupt
> file differs by containing all zeros in some places.

Thanks for checking!

> On Thu, Mar 14, 2024 at 8:36 AM Tom Tromey <tom@tromey.com> wrote:
>>
>> Also, I was wondering if this case can be tested somehow.  Perhaps the
>> DWARF assembler could be modified to allow the creation of corrupted
>> debug info.  It seems to me if we're going forward with the security
>> policy, then we're going to need to test these things.
> 
> We should be able to reproduce this in a testcase by overwriting part of
> a small binary with zeros.  I'll resubmit the patch with a testcase.

It sounds difficult to produce something that will give a reliable
result.  But if you can get it working, that's cool.

In the back of my mind, I always think that we should have a repo or
archive on the side (to avoid bloating the source repository) with
sample binaries like this one.  Crafting a test binary with the DWARF
assembler has its advantages, but so is trying it on the real thing.  We
could have tests in the testsuite that would run only if you have the
binary archive repo available locally.

Simon