Re: [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote:
>> This fixes a GDB crash reported in bug pr/28900, related to reading in
>> some stabs debug information.
>> 
>> In this commit my goal is to stop GDB crashing.  I am not trying to
>> ensure that GDB makes the best possible use of the available stabs
>> debug information.  At this point I consider stabs a legacy debug
>> format, with only limited support in GDB.
>> 
>> So, the problem appears to be that, when reading in the stabs data, we
>> need to find a N_SO entry, this is the entry that defines the start of
>> a compilation unit (or at least the location of a corresponding source
>> file).
>> 
>> It is while handling an N_SO that GDB creates a psymtab to hold the
>> incoming debug information (symbols, etc).
>> 
>> The problem we hit in the bug is that we encounter some symbol
>> information (an N_PC entry) outside of an N_SO entry - that is we find
>> some symbol information that is not associated with any source file.
>> 
>> We already have some protection for this case, look (in
>> read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f',
>> if we have no psymtab (the pst variable is nullptr) then we issue a
>> complaint.  However, for whatever reason, in both 'f' and 'F'
>> handling, there is one place where we assume that the pst
>> variable (the psymtab) is not nullptr.  This is a mistake.
>> 
>> In this commit, I guard these two locations (in 'f' and 'F' handling)
>> so we no longer assume pst is not nullptr.
>> 
>> While I was at it, I audited all the other uses of pst in
>> read_dbx_symtab, and in every potentially dangerous case I added a
>> nullptr check, and issue a suitable complaint if pst is found to be
>> nullptr.
>> 
>> It might well be true that we could/should do something smarter if we
>> see a debug symbol outside of an N_SO entry, and if anyone wanted to
>> do that work, they're welcome too.  But this commit is just about
>> preventing the nullptr access, and the subsequent GDB crash.
>> 
>> I don't have any tests for this change, I have no idea how to generate
>> weird stabs data for testing.  The original binary from the bug report
>> now loads just fine without GDB crashing.
>> 
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900
>
> For all I know, this looks correct.  Thanks for doing this.

Thanks, I pushed this patch.

Andrew