Re: [PATCHv2 3/6] gdb/arm: avoid undefined behaviour in arm_frame_is_thumb

[Luis Machado <luis.machado@arm.com>]

On 6/10/22 16:49, Andrew Burgess wrote:
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> On 6/10/22 14:08, Andrew Burgess via Gdb-patches wrote:
>>> This commit fixes real undefined behaviour in GDB which I spotted when
>>> working on a later patch in this series.  The later patch in this
>>> series detects when the result of gdbarch_tdep() is cast to the wrong
>>> type.
>>>
>>> The issue is revealed by the gdb.multi/multi-arch.exp test.
>>>
>>> In this test we setup two inferiors, an AArch64 process, and an ARM
>>> process, then at one point we have inferior 1 selected (the AArch64
>>> inferior), and we place a breakpoint on a symbol present in the other
>>> inferior (the ARM inferior).
>>>
>>> During the process of creating the breakpoint we call arm_pc_is_thumb,
>>> the GDBARCH passed into this function is correct, that is, represents
>>> the ARM process.
>>>
>>> For whatever reason we are unable to figure out if the address in
>>> question is thumb or not throughout most of arm_pc_is_thumb, and so we
>>> get to this code at the end of the function:
>>>
>>>     /* If we couldn't find any symbol, but we're talking to a running
>>>        target, then trust the current value of $cpsr.  This lets
>>>        "display/i $pc" always show the correct mode (though if there is
>>>        a symbol table we will not reach here, so it still may not be
>>>        displayed in the mode it will be executed).  */
>>>     if (target_has_registers ())
>>>       return arm_frame_is_thumb (get_current_frame ());
>>>
>>> Which I guess is a last attempt to figure out the thumb status of an
>>> address.  However, remember, we the AArch64 inferior is current at
>>> this time, so the current frame is an AArch64 frame.
>>
>> If we're trying to insert a breakpoint into a 32-bit inferior,
>> we should really have the 32-bit arm gdbarch at hand, not the AArch64
>> gdbarch.
> 
> We do.
> 
>>
>> I think the bug is somewhere else, in whoever passed the current inferior's gdbarch
>> as opposed to the gdbarch of the inferior that contains the symbol
>> we've found.
> 
> We do pass in the gdbarch of the inferior containing the breakpoint location.
> 
> If I change the condition to an assert, then run
> gdb.multi/multi-arch.exp and catch the assertion, the stack looks like
> this:
> 
>    #0  internal_error (file=0x55834840c8 "../../src/gdb/arm-tdep.c", line=551,
>        fmt=0x5583483d70 "%s: Assertion `%s' failed.") at ../../src/gdbsupport/errors.cc:51
>    #1  0x0000005582b0e6a4 in arm_frame_is_thumb (frame=0x55c0849a00) at ../../src/gdb/arm-tdep.c:551
>    #2  0x0000005582b0eb70 in arm_pc_is_thumb (gdbarch=0x55c0937080, memaddr=4195692) at ../../src/gdb/arm-tdep.c:687
>    #3  0x0000005582b17e70 in arm_adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692)
>        at ../../src/gdb/arm-tdep.c:4925
>    #4  0x0000005582af0a60 in gdbarch_adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692)
>        at ../../src/gdb/gdbarch.c:2840
>    #5  0x0000005582b73c90 in adjust_breakpoint_address (gdbarch=0x55c0937080, bpaddr=4195692, bptype=bp_breakpoint)
>        at ../../src/gdb/breakpoint.c:7147
>    #6  0x0000005582b76854 in code_breakpoint::add_location (this=0x55c088e340, sal=...)
>        at ../../src/gdb/breakpoint.c:8100
>    #7  0x0000005582b773b8 in code_breakpoint::code_breakpoint (this=0x55c088e340, gdbarch_=0x55c089c050,
>        type_=bp_breakpoint, sals=..., location_=..., filter_=std::unique_ptr<char> = {...},
>        cond_string_=std::unique_ptr<char> = {...}, extra_string_=std::unique_ptr<char> = {...},
>        disposition_=disp_donttouch, thread_=-1, task_=0, ignore_count_=0, from_tty=1, enabled_=1, flags=0,
>        display_canonical_=0) at ../../src/gdb/breakpoint.c:8329
>    #8  0x0000005582b90c00 in ordinary_breakpoint::code_breakpoint (this=0x55c088e340) at ../../src/gdb/breakpoint.c:266
>    #9  0x0000005582b88758 in new_breakpoint_from_type<gdb::array_view<symtab_and_line const>&, std::unique_ptr<event_location, event_location_deleter>, std::unique_ptr<char, gdb::xfree_deleter<char> >, std::unique_ptr<char, gdb::xfree_deleter<char> >, std::unique_ptr<char, gdb::xfree_deleter<char> >, bpdisp&, int&, int&, int&, int&, int&, unsigned int&, int&> (gdbarch=0x55c089c050, type=bp_breakpoint) at ../../src/gdb/breakpoint.c:1303
>    #10 0x0000005582b77710 in create_breakpoint_sal (gdbarch=0x55c089c050, sals=..., location=...,
>        filter=std::unique_ptr<char> = {...}, cond_string=std::unique_ptr<char> = {...},
>        extra_string=std::unique_ptr<char> = {...}, type=bp_breakpoint, disposition=disp_donttouch, thread=-1, task=0,
>        ignore_count=0, from_tty=1, enabled=1, internal=0, flags=0, display_canonical=0)
>        at ../../src/gdb/breakpoint.c:8395
>    #11 0x0000005582b779d8 in create_breakpoints_sal (gdbarch=0x55c089c050, canonical=0x7fd8d196c0,
>        cond_string=std::unique_ptr<char> = {...}, extra_string=std::unique_ptr<char> = {...}, type=bp_breakpoint,
>        disposition=disp_donttouch, thread=-1, task=0, ignore_count=0, from_tty=1, enabled=1, internal=0, flags=0)
>        at ../../src/gdb/breakpoint.c:8438
>    #12 0x0000005582b78d94 in create_breakpoint (gdbarch=0x55c089c050, location=0x55c0979750, cond_string=0x0, thread=-1,
>        extra_string=0x0, force_condition=false, parse_extra=1, tempflag=0, type_wanted=bp_breakpoint, ignore_count=0,
>        pending_break_support=AUTO_BOOLEAN_AUTO, ops=0x558389c650 <code_breakpoint_ops>, from_tty=1, enabled=1,
>        internal=0, flags=0) at ../../src/gdb/breakpoint.c:8923
>    #13 0x0000005582b792d8 in break_command_1 (arg=0x55c0732fc2 "", flag=0, from_tty=1) at ../../src/gdb/breakpoint.c:8994
>    #14 0x0000005582b79578 in break_command (arg=0x55c0732fb6 "hangout_loop", from_tty=1)
>        at ../../src/gdb/breakpoint.c:9065
> 
> In frame #6 we select a gdbarch based on the location of the breakpoint,
> its at this point that we select the bfd_arch_arm gdbarch.
> 
> For frames #5, #4, #3, and #2 we are passing in a bfd_arch_arm gdbarch,
> which is correct, and what you are asking for.
> 
> The problem is that in frame #2 we fail to find any of the special hints
> that indicate if the code is thumb or not.  Now, _maybe_ you could argue
> that one of the conditions in arm_pc_is_thumb should trigger, but, for
> me, the very fact that there is a "catch all" case at the end of the
> function means we have to be open to the possibility that non of the
> special symbols, or bottom bit of the address set cases might trigger.
> 
> And so, we get to this code in arm_pc_is_thumb:
> 
>    if (target_has_registers ())
>      return arm_frame_is_thumb (get_current_frame ());

Thanks for the above backtrace, that was very helpful. Yeah, this call is making the wrong assumption
that the current frame is arm. This is clearly not always valid on a multi-arch scenario.

> 
> At this point GDBARCH _is_ a bfd_arch_arm architecture.
> 
> But, the current frame is bfd_arch_aarch64.
> 
> And so, we enter frame #1, arm_frame_is_thumb, passing in a frame that
> is not bfd_arch_arm.
> 
> So, are you are suggesting we should switch frames as part of the
> breakpoint setting process?  Because I'm not sure how you'd pick even a
> suitable inferior, multiple ARM inferiors might share a single program
> space, so we could have a single gdbarch, but multiple inferiors, each
> with multiple threads, and each thread with multiple frames...
> 

The correct way to do it is to fetch the frame from the inferior containing the
breakpoint location, but you're right that there isn't a good way to do it here.

Ideally GDB would keep per-inferior frame data, but it doesn't. This looks like a
limitation of GDB. If you have the same bfd for all inferiors, I suppose you could
get away with it.

In this case though, trying to use the aarch64 bfd to do an operation for the arm bfd
is not going to work.

> I guess we could push the architecture check out of arm_frame_is_thumb
> back to arm_pc_is_thumb, and only call arm_frame_is_thumb for the case
> where the architecture is ARM... that doesn't make sense to me, but
> maybe, I guess...

It also doesn't make sense, because then the answer to whether we have thumb or
not is always going to be false for an aarch64 bfd. We might as well return false
and not fetch the frame.

> 
> Anyway, let me know if the above makes any more sense.  If it does I can
> update the commit message
Given this is broken either way, let's go with your patch. But could you
please add a comment in the bfd check stating this is currently incorrect
because fetching the current_frame doesn't reflect the correct inferior?

I'll take a look at this later, but I don't want to block your series.