[binutils-gdb] Fix GDBserver regression due to change to avoid reading shell registers

[binutils-gdb] Fix GDBserver regression due to change to avoid reading shell registers

[Pedro Alves]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=7e8621cf6db8d69c325d86dba59037367cd0f853

commit 7e8621cf6db8d69c325d86dba59037367cd0f853
Author: Pedro Alves <pedro@palves.net>
Date:   Wed Jun 29 16:38:43 2022 +0100

    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 calls 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

Diff:
---
 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;