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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2021-09-28 13:55, Bruno Larsen wrote:
> On 9/24/21 5:59 PM, Simon Marchi wrote:
>> On 2021-09-24 16:19, Bruno Larsen wrote:
>>>  From what I read in the DWARF spec, if you have a constant
>>>  DW_AT_data_member_location, your inheritance is non-virtual, I
>>>  guess my intent was to turn that A => B into a A <=> B (making it
>>>  so if you have a non-trivial expression, you must have virtual
>>>  inheritance), but you're right, there's nothing on the DWARF specs
>>>  that say it should be so.
>>
>> Indeed, all I see is this non-normative text:
>>
>>      For a C++ virtual base, the data member location attribute will
>>      usually consist of a non-trivial location description.
>>
>> The virtuality of base classes is tracked separately, so there
>> doesn't seem to be a need to tie inheritance virtuality and the
>> location kind.
>>
>>> This means there is no way (that I can see at least) to fix the root
>>> cause or the core dump/assert, which is getting a bogus location for
>>> a member of the class, other than adding a validation step
>>> somewhere, that checks if the pointers are actually pointing to
>>> inside the base class. This patch is the next best thing I could
>>> think of, alerting the user in a non-destructive manner.
>>
>> Well, can't we teach gnuv3_baseclass_offset to evaluate offsets given
>> as location descriptions even if the inheritance is non-virtual?  It
>> seems to be like that would be a good start.
> 
> I've spent some time looking at this. This sounds like a trivial fix,
> because we can re-use what's already there, basically.

Hopefully it is!

>> But I think you are right, we probably need some validation / bailing
>> out somewhere to make sure that the offset makes sense (whether that
>> is in value_contents_copy_raw as you have done, or somehwere else).

> Another options for validation is adding a trivial validation on
> gnuv3_baseclass_offset (like the original one from this patch,
> checking if the offset is greater than the size of the class), which
> only work for non-virtual inheritance and returning a bitpos location.
> I am not familiar with virtual inheritance, much less how GDB deals
> with it internally, so I don't really know a good way to validate it.
> A quick google tells me that my validation on value_contents_copy_raw
> could assume that a problem has happened when we are just copying a
> virtual inheritance member. A better spot to put it could be
> cp_print_value, since we still have virtuality information at that
> point, and can validate differently at that point.
> 
> The trivial validation fixes the second case, but not the first, so we
> still need the second validation. This still doesn't sound perfect to
> me, but I can start work on a v3 at least. Thoughts?

I would suggest adding a gdb_assert in value_contents_copy_raw, that
validate that we are not copying outside the values bounds.  I think
that we can consider any case like this to be a bug in the caller of
value_contents_copy_raw.  Doing it like this (pushing the responsibility
to the caller to check bounds if needed) will lead to more precise error
messages.  In value_contents_copy_raw, you can only give a rather
generic error message, but in the caller it can be a bit more
precise.

Let's see what you come up with in v3.  If you are not sure how to
proceed, then at least write the update test case, and I'll be able to
take a look.

Thanks,

Simon