Re: [PATCH] Fix GDBserver regression due to change to avoid reading shell registers

[Simon Marchi <simark@simark.ca>]

On 6/29/22 12:22, Pedro Alves wrote:
> On 2022-06-29 16:17, Simon Marchi wrote:
> 
>> This introduces the following ASan crash for me:
> 
> ...
> 
>> It looks like in that non-existing-program case, the process gets deleted before
>> the starting_up flag gets restored to false.
> 
> Thanks.  The patch below fixes it.  I've also run the whole testsuite with 
> --target_board=native-extended-gdbserver, on x86-64 Ubuntu 20.04, with Asan,
> and saw no regressions.
> 
> From 5ceb44cdf51b764193bea229c80e55ebb0d9c55c Mon Sep 17 00:00:00 2001
> From: Pedro Alves <pedro@palves.net>
> Date: Wed, 29 Jun 2022 16:38:43 +0100
> Subject: [PATCH] Fix GDBserver regression due to change to avoid reading shell
>  registers
> 
> Simon reported that the recent change to make GDB and GDBserver avoid
> reading shell registers caused a GDBserver regression, caught with
> ASan while running gdb.server/non-existing-program.exp:
> 
>  $ /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)
> 
> Above we see that in the non-existing-program case, the process gets
> deleted before the starting_up flag gets restored to false.
> 
> This happens because startup_inferior call target_mourn_inferior
> before throwing an error, and in GDBserver, unlike in GDB, mourning
> deletes the process.
> 
> Fix this by not using a scoped_restore to manage the starting_up flag,
> since we should only clear it when startup_inferior doesn't throw.
> 
> Change-Id: I67325d6f81c64de4e89e20e4ec4556f57eac7f6c
> ---
>  gdbserver/fork-child.cc | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/gdbserver/fork-child.cc b/gdbserver/fork-child.cc
> index 7ea66f2a435..078612f7091 100644
> --- a/gdbserver/fork-child.cc
> +++ b/gdbserver/fork-child.cc
> @@ -105,12 +105,21 @@ post_fork_inferior (int pid, const char *program)
>  #endif
>  
>    process_info *proc = find_process_pid (pid);
> -  scoped_restore save_starting_up
> -    = make_scoped_restore (&proc->starting_up, true);
> +
> +  /* If the inferior fails to start, startup_inferior mourns the
> +     process (which deletes it), and then throws an error.  This means
> +     that on exception return, we don't need or want to clear this
> +     flag back, as PROC won't exist anymore.  Thus, we don't use a
> +     scoped_restore.  */
> +  proc->starting_up = true;
>  
>    startup_inferior (the_target, pid,
>  		    START_INFERIOR_TRAPS_EXPECTED,
>  		    &cs.last_status, &cs.last_ptid);
> +
> +  /* If we get here, the process was successfully started.  */
> +  proc->starting_up = false;
> +
>    current_thread->last_resume_kind = resume_stop;
>    current_thread->last_status = cs.last_status;
>    signal_pid = pid;
> 
> base-commit: b955c336f93a1b8ab433b713739c145c56f5c027
> -- 
> 2.36.0
> 

Thanks, LGTM.  This is the same fix I had imagined.

Simon