Re: [PATCH v2] gdb+gdbserver/Linux: avoid reading registers while going through shell (was: Re: [PATCHv3 6/6] gdb: native target invalid architecture detection)

[Simon Marchi <simark@simark.ca>]

On 6/28/22 08:42, Pedro Alves wrote:
> On 2022-06-28 11:37, Andrew Burgess wrote:
>> Pedro Alves <pedro@palves.net> writes:
>>
>>> On 2022-06-27 17:27, Andrew Burgess wrote:
>>>> Pedro Alves <pedro@palves.net> writes:
>>> So I gave the patch below a try, and it seems to works well -- readcache_read_pc
>>> is no longer called before target_find_description, and I saw no testsuite
>>> regressions (this is without your series, just pristine master).
>>>
>>> Now, this patch as is makes sense for linux-nat.c, but then I went to do the
>>> same thing for gdbserver, which has an equivalent save_stop_reason in linux-low.cc,
>>> and there this approach won't work as is for the simple fact that lwp->stop_pc
>>> is used in a lot more places.
>>>
>>> So to avoid gdb vs gdbserver divergence, it may be better to not take this patch
>>> as is, but instead tweak save_stop_reason to avoid reading the stop pc while
>>> the inferior is going through the shell, based on some per-inferior flag or some
>>> such.  Maybe we can repurpose progspace->executing_startup for this?  That
>>> flag used to be used by Cell (which is gone), but seems unused nowadays.  There's
>>> also other similar flags in inferior itself, like needs_setup, and
>>> in_initial_library_scan.  So we'd add a flag like one of those, set it while the
>>> inferior is going through the startup_inferior dance, and make save_stop_reason avoid
>>> reading the PC while that flag is set.
> 
> ...
> 
>>
>> Thanks, this looks good.  Maybe you should go ahead and merge this, and
>> the attach problem can be solved separately.
> 
> There's the gdb vs gdbserver divergence issue I pointed out above, though.
> 
> I was thinking we'd instead fix it with the alternative patch below, which
> works for both gdb and gdbserver.
> 
> Tested with no regressions on x86-64 Ubuntu 20.04, native and gdbserver.
> 
> WDYT?
> 
> From 1b7b50e7c8359553af8b8858a82f09d5389d4a1b Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Mon, 27 Jun 2022 20:41:50 +0100
> Subject: [PATCH] gdb+gdbserver/Linux: avoid reading registers while going
>  through shell
> 
> For every stop, linux-nat.c saves the stopped thread's PC, in
> lwp->stop_pc.  This is done in save_stop_reason, in both
> gdb/linux-nat.c and gdbserver/linux-low.cc.  However, while we're
> going through the shell after "run", in startup_inferior, we shouldn't
> be reading registers, as we haven't yet determined the target's
> architecture -- the shell's architecture may not even be the same as
> the final inferior's.
> 
> In linux-nat.c, lwp->stop_pc is only needed when the thread has
> stopped for a breakpoint, and since when going through the shell, no
> breakpoint is going to hit, we could simply teach save_stop_reason to
> only record the stop pc when the thread stopped for a breakpoint.
> 
> However, in gdbserver/linux-low.cc, lwp->stop_pc is used in more cases
> than breakpoint hits (e.g., it's used in tracepoints & the
> "while-stepping" feature).
> 
> So to avoid GDB vs GDBserver divergence, we apply the same approach to
> both implementations.
> 
> We set a flag in the inferior (process in gdbserver) whenever it is
> being nursed through the shell, and when that flag is set,
> save_stop_reason bails out early.  While going through the shell,
> we'll only ever get process exits (normal or signalled), and exec
> events, so nothing is lost.
> 
> Change-Id: If0f01831514d3a74d17efd102875de7d2c6401ad

Hi,

This introduces the following ASan crash for me:

$ /home/smarchi/build/binutils-gdb/gdb/testsuite/../../gdb/../gdbserver/gdbserver stdio non-existing-program
=================================================================
==127719==ERROR: AddressSanitizer: heap-use-after-free on address 0x60f0000000e9 at pc 0x55bcbfa301f4 bp 0x7ffd238a7320 sp 0x7ffd238a7310
WRITE of size 1 at 0x60f0000000e9 thread T0
    #0 0x55bcbfa301f3 in scoped_restore_tmpl<bool>::~scoped_restore_tmpl() /home/smarchi/src/binutils-gdb/gdbserver/../gdbsupport/scoped_restore.h:86
    #1 0x55bcbfa2ffe9 in post_fork_inferior(int, char const*) /home/smarchi/src/binutils-gdb/gdbserver/fork-child.cc:120
    #2 0x55bcbf9c9199 in linux_process_target::create_inferior(char const*, std::__debug::vector<char*, std::allocator<char*> > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:991
    #3 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941
    #4 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084
    #5 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2)
    #6 0x55bcbf8ef2bd in _start (/home/smarchi/build/binutils-gdb/gdbserver/gdbserver+0x1352bd)

0x60f0000000e9 is located 169 bytes inside of 176-byte region [0x60f000000040,0x60f0000000f0)
freed by thread T0 here:
    #0 0x7ff9d6c6f0c7 in operator delete(void*) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:160
    #1 0x55bcbf910d00 in remove_process(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/inferiors.cc:164
    #2 0x55bcbf9c4ac7 in linux_process_target::remove_linux_process(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:454
    #3 0x55bcbf9cdaa6 in linux_process_target::mourn(process_info*) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:1599
    #4 0x55bcbf988dc4 in target_mourn_inferior(ptid_t) /home/smarchi/src/binutils-gdb/gdbserver/target.cc:205
    #5 0x55bcbfa32020 in startup_inferior(process_stratum_target*, int, int, target_waitstatus*, ptid_t*) /home/smarchi/src/binutils-gdb/gdbserver/../gdb/nat/fork-inferior.c:515
    #6 0x55bcbfa2fdeb in post_fork_inferior(int, char const*) /home/smarchi/src/binutils-gdb/gdbserver/fork-child.cc:111
    #7 0x55bcbf9c9199 in linux_process_target::create_inferior(char const*, std::__debug::vector<char*, std::allocator<char*> > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:991
    #8 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941
    #9 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084
    #10 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2)

previously allocated by thread T0 here:
    #0 0x7ff9d6c6e5a7 in operator new(unsigned long) ../../../../src/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x55bcbf910ad0 in add_process(int, int) /home/smarchi/src/binutils-gdb/gdbserver/inferiors.cc:144
    #2 0x55bcbf9c477d in linux_process_target::add_linux_process_no_mem_file(int, int) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:425
    #3 0x55bcbf9c8f4c in linux_process_target::create_inferior(char const*, std::__debug::vector<char*, std::allocator<char*> > const&) /home/smarchi/src/binutils-gdb/gdbserver/linux-low.cc:985
    #4 0x55bcbf954549 in captured_main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:3941
    #5 0x55bcbf9552f0 in main /home/smarchi/src/binutils-gdb/gdbserver/server.cc:4084
    #6 0x7ff9d663b0b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x240b2)

It looks like in that non-existing-program case, the process gets deleted before
the starting_up flag gets restored to false.

Simon