[PATCH] gdb/dwarf2: Check for missing abbrev

[PATCH] gdb/dwarf2: Check for missing abbrev

[Aaron Merey]

A corrupt debuginfo file can result in a null abbrev_info pointer
being passed to cooked_indexer::scan_attributes.  There is no check
for whether the abbrev pointer is null and SIGSEGV occurs when
attempting to dereference the pointer.

Fix this by calling throw_error at the beginning of scan_attributes
when the abbrev is nullptr.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31478
---
 gdb/dwarf2/read.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 4afb026b8ce..a6e5a3c856e 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16077,6 +16077,9 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu,
 				 bool *is_enum_class,
 				 bool for_specification)
 {
+  if (abbrev == nullptr)
+    throw_error (NOT_AVAILABLE_ERROR, _("Unable to find DWARF attributes."));
+
   bool origin_is_dwz = false;
   bool is_declaration = false;
   sect_offset origin_offset {};
-- 
2.43.0

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

[Simon Marchi]

On 3/13/24 16:18, Aaron Merey wrote:
> A corrupt debuginfo file can result in a null abbrev_info pointer
> being passed to cooked_indexer::scan_attributes.  There is no check
> for whether the abbrev pointer is null and SIGSEGV occurs when
> attempting to dereference the pointer.
> 
> Fix this by calling throw_error at the beginning of scan_attributes
> when the abbrev is nullptr.
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31478
> ---
>  gdb/dwarf2/read.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index 4afb026b8ce..a6e5a3c856e 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -16077,6 +16077,9 @@ cooked_indexer::scan_attributes (dwarf2_per_cu_data *scanning_per_cu,
>  				 bool *is_enum_class,
>  				 bool for_specification)
>  {
> +  if (abbrev == nullptr)
> +    throw_error (NOT_AVAILABLE_ERROR, _("Unable to find DWARF attributes."));

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).

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?

Simon

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

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey <amerey@redhat.com> writes:

Aaron> +  if (abbrev == nullptr)
Aaron> +    throw_error (NOT_AVAILABLE_ERROR, _("Unable to find DWARF attributes."));

NOT_AVAILABLE_ERROR is specific to unavailable values.
This should just use error()

Tom

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

[Tom Tromey]

>>>>> "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.

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.

Tom

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

[Tom Tromey]

> A corrupt debuginfo file can result in a null abbrev_info pointer
> being passed to cooked_indexer::scan_attributes.  There is no check
> for whether the abbrev pointer is null and SIGSEGV occurs when
> attempting to dereference the pointer.

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.

Tom

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

[Simon Marchi]

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

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

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> The issue is corrupt DWARF -- the DIE specifies an abbrev that doesn't exist.

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

Yeah sorry about that.

Simon> If peek_die_abbrev returns nullptr, it's because it read abbrev number 0
Simon> specifically (which normally means "end of DIE list", but is unexpected
Simon> at this particular place).  So I'm just wondering what "kind" of
Simon> corruption causes it to be 0.  The concrete case I'm worried about is
Simon> the possibility that the file is truncated and we go read an offset that
Simon> is out of our buffer's bounds.  In that case, some bounds checking
Simon> should probably be added somehwere.

Yes, this definitely needs to be done at some point.

Tom

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

[Aaron Merey]

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.

> 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.

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.

Aaron

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=31478

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

[Simon Marchi]

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