[PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[Denis Lukianov]

Following change 4bce7cdaf4 "gdbsupport: add array_view copy function",
dwarf stack fetch sometimes cause an internal-error in
array_view::copy, where a gdb_assert expects the source and destination
view sizes to match. When called from dwarf_expr_context::fetch_result
sometimes the lengths don't match.

Both the source and destination views each have a separate implicit
length. The source is correctly sliced for the copy. However, the
destination is passed with the full allocated length, which does not
necessarily match the source length.

This patch slices the destination to match the source length.

diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
index 3549745df04..aa203e87bfb 100644
--- a/gdb/dwarf2/expr.c
+++ b/gdb/dwarf2/expr.c
@@ -1025,7 +1025,7 @@ dwarf_expr_context::fetch_result (struct type
*type, struct type *subobj_type,
              subobj_offset += n - max;
 
            copy (value_contents_all (val).slice (subobj_offset, len),
-                 value_contents_raw (retval));
+                 value_contents_raw (retval).slice (0, len));
          }
          break;

Re: [PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[Simon Marchi]

On 8/8/22 07:24, Denis Lukianov wrote:
> Following change 4bce7cdaf4 "gdbsupport: add array_view copy function",
> dwarf stack fetch sometimes cause an internal-error in
> array_view::copy, where a gdb_assert expects the source and destination
> view sizes to match. When called from dwarf_expr_context::fetch_result
> sometimes the lengths don't match.
>
> Both the source and destination views each have a separate implicit
> length. The source is correctly sliced for the copy. However, the
> destination is passed with the full allocated length, which does not
> necessarily match the source length.
>
> This patch slices the destination to match the source length.
>
> diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c
> index 3549745df04..aa203e87bfb 100644
> --- a/gdb/dwarf2/expr.c
> +++ b/gdb/dwarf2/expr.c
> @@ -1025,7 +1025,7 @@ dwarf_expr_context::fetch_result (struct type
> *type, struct type *subobj_type,
>               subobj_offset += n - max;
>
>             copy (value_contents_all (val).slice (subobj_offset, len),
> -                 value_contents_raw (retval));
> +                 value_contents_raw (retval).slice (0, len));
>           }
>           break;
>

Hi Denis,

Do you have a reproducer for this, that we could turn into a test?

I don't think your change is necessary. retval's content length comes
from the length of `subobj_type`:

  retval = allocate_value (subobj_type);

and the variable `len`, used to slice the source view, also comes from
the length of `subobj_type`:

  size_t len = TYPE_LENGTH (subobj_type);

So the two views should be of that same length.

Your patch reminded me of a pending patch I had in the same area, which
I just merged:

  https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=bde195b84a862f31ac111c0881ad13b89ee89492

Maybe you were seeing the same problem as described there?

Simon

Re: [PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[Denis Lukianov]

On Thu, 2022-08-18 at 12:27 -0400, Simon Marchi wrote:
> On 8/8/22 07:24, Denis Lukianov wrote:
> 
> 
> Your patch reminded me of a pending patch I had in the same area,
> which
> I just merged:
> 
>  
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=bde195b84a862f31ac111c0881ad13b89ee89492
> 
> Maybe you were seeing the same problem as described there?
> 
> Simon

Hi Simon,

Your check_typedef fixes my use case on little endian architectures.

However, note that DWARF_VALUE_STACK case also re-slices the source
val. It looks like after that it remains mismatched to the destination
which also must be re-sliced to match (as per my patch, or implicitly
back when this was a simple memcpy with a single length). I have no big
endian resource configured to test this.

Best regards,
Denis

Re: [PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[Simon Marchi]

On 2022-08-19 17:33, Denis Lukianov wrote:
> On Thu, 2022-08-18 at 12:27 -0400, Simon Marchi wrote:
>> On 8/8/22 07:24, Denis Lukianov wrote:
>>
>>
>> Your patch reminded me of a pending patch I had in the same area,
>> which
>> I just merged:
>>
>>  
>> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=bde195b84a862f31ac111c0881ad13b89ee89492
>>
>> Maybe you were seeing the same problem as described there?
>>
>> Simon
> 
> Hi Simon,
> 
> Your check_typedef fixes my use case on little endian architectures.
> 
> However, note that DWARF_VALUE_STACK case also re-slices the source
> val. It looks like after that it remains mismatched to the destination
> which also must be re-sliced to match (as per my patch, or implicitly
> back when this was a simple memcpy with a single length). I have no big
> endian resource configured to test this.

I don't understand what you are saying.  All I can see is that the
source array_view size will be of the length of subobj_type: we call
slice with length == len, where len is `TYPE_LENGTH (subobj_type)`.  And
the destination array_view size is also of the length of subobj_type:
retval was allocated as `allocate_value (subobj_type)`, so its contents
are of the length of subobj_type.

Simon

Re: [PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[Denis Lukianov]

On Fri, 2022-08-19 at 20:55 -0400, Simon Marchi wrote:
> 
> I don't understand what you are saying.  All I can see is that the
> source array_view size will be of the length of subobj_type: we call
> slice with length == len, where len is `TYPE_LENGTH (subobj_type)`. 
> And
> the destination array_view size is also of the length of subobj_type:
> retval was allocated as `allocate_value (subobj_type)`, so its
> contents
> are of the length of subobj_type.
> 

I know nothing about gdb internals, just reading code in the file:

copy (value_contents_all (val).slice (subobj_offset, len),
		  value_contents_raw (retval));

We know val is allocated to be the length of subobj_type.
We know retval is allocated to be the length of subobj_type.
We know len is the length of subobj_type.
We know slice only gives a view same size or smaller.

So, the source array_view length must be len - subobj_offset.

Therefore source array_view size will be of the length of subobj_type
and match the destination only in the event that subobj_offset happens
to be zero. The line above does not guarantee it:

subobj_offset += n - max;

So it looks broken for big endian systems. And maybe any subtype that
is not the first element of a type on the others.

Re: [PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[Denis Lukianov]

On Sat, 2022-08-20 at 02:38 +0100, Denis Lukianov wrote:
> 
> I know nothing about gdb internals, just reading code in the file:
> 
> copy (value_contents_all (val).slice (subobj_offset, len),
>                   value_contents_raw (retval));
> 
> We know val is allocated to be the length of subobj_type.
> We know retval is allocated to be the length of subobj_type.
> We know len is the length of subobj_type.
> We know slice only gives a view same size or smaller.
> 
> So, the source array_view length must be len - subobj_offset.
> 
> Therefore source array_view size will be of the length of subobj_type
> and match the destination only in the event that subobj_offset
> happens
> to be zero. The line above does not guarantee it:
> 
> subobj_offset += n - max;
> 
> So it looks broken for big endian systems. And maybe any subtype that
> is not the first element of a type on the others.
> 

My bad, val is allocated to be length of type.

So I'm wrong about all this and the code is fine :)

Re: [PATCH] dwarf2: Fix dwarf stack fetch array view size mismatch

[Simon Marchi]

On 2022-08-19 21:55, Denis Lukianov wrote:
> On Sat, 2022-08-20 at 02:38 +0100, Denis Lukianov wrote:
>>
>> I know nothing about gdb internals, just reading code in the file:
>>
>> copy (value_contents_all (val).slice (subobj_offset, len),
>>                   value_contents_raw (retval));
>>
>> We know val is allocated to be the length of subobj_type.
>> We know retval is allocated to be the length of subobj_type.
>> We know len is the length of subobj_type.
>> We know slice only gives a view same size or smaller.
>>
>> So, the source array_view length must be len - subobj_offset.
>>
>> Therefore source array_view size will be of the length of subobj_type
>> and match the destination only in the event that subobj_offset
>> happens
>> to be zero. The line above does not guarantee it:
>>
>> subobj_offset += n - max;
>>
>> So it looks broken for big endian systems. And maybe any subtype that
>> is not the first element of a type on the others.
>>
> 
> My bad, val is allocated to be length of type.
> 
> So I'm wrong about all this and the code is fine :)
> 

Ok, thanks for digging into it, the more eyes on the code the better.

Simon