Re: [PATCH v2] Fixed core dump from incorrect location expression on bad dwarfs

[Bruno Larsen <blarsen@redhat.com>]

On 9/24/21 1:12 AM, Simon Marchi wrote:
> On 2021-09-23 10:45 p.m., Simon Marchi via Gdb-patches wrote:
>> On 2021-08-30 4:10 p.m., Bruno Larsen via Gdb-patches wrote:
>>> Some incorrectly constructed location expressions in inheritance members
>>> of a class could lead to a core dump, or printing garbage instead of a
>>> correct value. The added test case always core dumped during my
>>> testing, but it could be changed to print garbage by changing the
>>> location expression on the exp file to not include DW_OP_stack_value,
>>> but just use a large constant value.
>>>
>>> The solution is, when copying contents of a value struct, check if
>>> contents will actually be copied (ie length > 0) and if the
>>> offset of the copied member is greater than the size of the struct
>>> itself, raising an error if so.
>>
>> Thanks for the test, that makes it really easy to reproduce and
>> investigate.
>>
>> I think the way you fix it only hides the real problem.  The
>> particularity of your debug info appears to be that a DWARF expression
>> is used in the DW_TAG_inheritance's DW_AT_data_member_location
>> attribute, instead of a constant (for non-virtual inheritance, that is).
>>
>> Right now I'm stopped here:
>>
>>      #0  0x000055892cf7023d in type::field (this=0x621000190550, idx=0) at /home/simark/src/binutils-gdb/gdb/gdbtypes.h:973
>>      #1  0x000055892d9ced8e in gnuv3_baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/gnu-v3-abi.c:464
>>      #2  0x000055892d56a368 in baseclass_offset (type=0x621000190550, index=0, valaddr=0x6030001fbbb0 "\b", embedded_offset=0, address=0x0, val=0x6110000aba80) at /home/simark/src/binutils-gdb/gdb/cp-abi.c:78
>>      #3  0x000055892d5998fe in cp_print_value (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=1, options=0x7ffde7bd0da0, dont_print_vb=0x0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:436
>>      #4  0x000055892d59718e in cp_print_value_fields (val=0x6110000aba80, stream=0x7ffde7bd1270, recurse=0, options=0x7ffde7bd0da0, dont_print_vb=0x0, dont_print_statmem=0) at /home/simark/src/binutils-gdb/gdb/cp-valprint.c:161
>>
>> ... as GDB is trying to get the offset of base class A in class B.  GDB
>> represents base classes as fields, so frame #0 is getting the field
>> representing base class A of class B.  Here, $13 represents that struct
>> field:
>>
>>      $13 = {
>>        loc = {
>>          bitpos = 107820860638704,
>>          enumval = 107820860638704,
>>          physaddr = 0x6210001905f0,
>>          physname = 0x6210001905f0 "L!\v",
>>          dwarf_block = 0x6210001905f0
>>        },
>>        artificial = 0,
>>        loc_kind = FIELD_LOC_KIND_DWARF_BLOCK,
>>        bitsize = 0,
>>        m_type = 0x621000190380,
>>        name = 0x62100016aa50 "A"
>>      }
>>
>> loc_kind being FIELD_LOC_KIND_DWARF_BLOCK, the active field of the `loc`
>> union is `dwarf_block`.  It points to the dwarf_block (the expression)
>> to evaluate to get the offset, created in handle_data_member_location,
>> in dwarf2/read.c.
>>
>> If we look at code of frame 1, in gnuv3_baseclass_offset:
>>
>>    /* If it isn't a virtual base, this is easy.  The offset is in the
>>       type definition.  */
>>    if (!BASETYPE_VIA_VIRTUAL (type, index))
>>      return TYPE_BASECLASS_BITPOS (type, index) / 8;
>>
>> This code accesses blindly the `bitpos` field of the union.  So the
>> value it returns for the offset of base class A in class B is actually
>> the pointer to the dwarf_block, cast to an int (and divided by 8), and
>> therefore bogus.  That seems to end up creating an impossible
>> value_embedded_offset, and is the cause for the out of bounds read you
>> see.
> 
> I made a patch that adds some asserts in struct field, when getting the
> location, to ensure we don't get the location as a wrong kind:
> 
>    https://sourceware.org/pipermail/gdb-patches/2021-September/182136.html
> 
> It catches the problem when running your test:
> 
>      313 finish^M
>      314 Run till exit from #0  foo () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c:38^M
>      315 main () at /home/simark/src/binutils-gdb/gdb/testsuite/gdb.dwarf2/dw2-inheritance-stack-location.c:49^M
>      316 49        return 0;                                     /* main return */^M
>      317 Value returned is $1 = /home/simark/src/binutils-gdb/gdb/gdbtypes.h:667: internal-error: LONGEST field::loc_bitpos() const: Assertion `m_loc_kind == FIELD_LOC_KIND_BITPOS' failed.^M
> 
> So I think this will be the root issue to fix.
> 
> Simon
> 
First of all, thanks for the review and explanation of what was going on. This will definitely help.

Second, from your explanation, it sounds like I can solve the root cause of the core-dump/assertion by detecting if the non-trivial location expression for the inheritance is accompanied by a DW_AT_virtuality, so my plan would be to check for that as we read the DIE and issue a complain if something like this happens. Does that sound reasonable?

Third, I still think this patch could be useful because if the location expression is incorrect but doesn't trigger that assertion, GDB can end up printing garbage (which would make people blame GDB, rather than looking at their dwarf for incorrect offsets). To test that, you can just change the inheritance DIE to:

DW_TAG_inheritance {
   {DW_AT_type :$class_A_label}
   {DW_AT_data_member_location {DW_OP_constu 9000} SPECIAL_expr}
   {DW_AT_accessibility 1 DW_FORM_data1}
}

--
Cheers!
Bruno