[binutils-gdb] gdbserver/linux: free process_info_private and arch_process_info when failing to attach

[binutils-gdb] gdbserver/linux: free process_info_private and arch_process_info when failing to attach

[Simon Marchi]

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

commit f551c8ef326a022ec44279e66ed9734692225067
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Fri Apr 22 11:34:54 2022 -0400

    gdbserver/linux: free process_info_private and arch_process_info when failing to attach
    
    Running
    
      $ ../gdbserver/gdbserver --once --attach :1234 539436
    
    with ASan while /proc/sys/kernel/yama/ptrace_scope is set to 1 (prevents
    attaching) shows that we fail to free some platform-specific objects
    tied to the process_info (process_info_private and arch_process_info):
    
        Direct leak of 32 byte(s) in 1 object(s) allocated from:
            #0 0x7f6b558b3fb9 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
            #1 0x562eaf15d04a in xcalloc /home/simark/src/binutils-gdb/gdbserver/../gdb/alloc.c:100
            #2 0x562eaf251548 in xcnew<process_info_private> /home/simark/src/binutils-gdb/gdbserver/../gdbsupport/poison.h:122
            #3 0x562eaf22810c in linux_process_target::add_linux_process_no_mem_file(int, int) /home/simark/src/binutils-gdb/gdbserver/linux-low.cc:426
            #4 0x562eaf22d33f in linux_process_target::attach(unsigned long) /home/simark/src/binutils-gdb/gdbserver/linux-low.cc:1132
            #5 0x562eaf1a7222 in attach_inferior /home/simark/src/binutils-gdb/gdbserver/server.cc:308
            #6 0x562eaf1c1016 in captured_main /home/simark/src/binutils-gdb/gdbserver/server.cc:3949
            #7 0x562eaf1c1d60 in main /home/simark/src/binutils-gdb/gdbserver/server.cc:4084
            #8 0x7f6b552f630f in __libc_start_call_main (/usr/lib/libc.so.6+0x2d30f)
    
        Indirect leak of 56 byte(s) in 1 object(s) allocated from:
            #0 0x7f6b558b3fb9 in __interceptor_calloc /usr/src/debug/gcc/libsanitizer/asan/asan_malloc_linux.cpp:154
            #1 0x562eaf15d04a in xcalloc /home/simark/src/binutils-gdb/gdbserver/../gdb/alloc.c:100
            #2 0x562eaf2a0d79 in xcnew<arch_process_info> /home/simark/src/binutils-gdb/gdbserver/../gdbsupport/poison.h:122
            #3 0x562eaf295e2c in x86_target::low_new_process() /home/simark/src/binutils-gdb/gdbserver/linux-x86-low.cc:723
            #4 0x562eaf22819b in linux_process_target::add_linux_process_no_mem_file(int, int) /home/simark/src/binutils-gdb/gdbserver/linux-low.cc:428
            #5 0x562eaf22d33f in linux_process_target::attach(unsigned long) /home/simark/src/binutils-gdb/gdbserver/linux-low.cc:1132
            #6 0x562eaf1a7222 in attach_inferior /home/simark/src/binutils-gdb/gdbserver/server.cc:308
            #7 0x562eaf1c1016 in captured_main /home/simark/src/binutils-gdb/gdbserver/server.cc:3949
            #8 0x562eaf1c1d60 in main /home/simark/src/binutils-gdb/gdbserver/server.cc:4084
            #9 0x7f6b552f630f in __libc_start_call_main (/usr/lib/libc.so.6+0x2d30f)
    
    Those objects are deleted by linux_process_target::mourn, but that is
    not called if we fail to attach, we only call remove_process.  I
    initially fixed this by making linux_process_target::attach call
    linux_process_target::mourn on failure (before calling error).  But this
    isn't done anywhere else (including in GDB) so it would just be
    confusing to do things differently here.
    
    Instead, add a linux_process_target::remove_linux_process helper method
    (which calls remove_process), and call that instead of remove_process in
    the Linux target.  Move the free-ing of the extra data from the mourn
    method to that new method.
    
    Change-Id: I277059a69d5f08087a7f3ef0b8f1792a1fcf7a85

Diff:
---
 gdbserver/linux-low.cc | 27 ++++++++++++++++-----------
 gdbserver/linux-low.h  |  3 +++
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 449c6641f58..8b8614f6ed4 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -440,6 +440,20 @@ linux_process_target::add_linux_process (int pid, int attached)
   return proc;
 }
 
+void
+linux_process_target::remove_linux_process (process_info *proc)
+{
+  if (proc->priv->mem_fd >= 0)
+    close (proc->priv->mem_fd);
+
+  this->low_delete_process (proc->priv->arch_private);
+
+  xfree (proc->priv);
+  proc->priv = nullptr;
+
+  remove_process (proc);
+}
+
 arch_process_info *
 linux_process_target::low_new_process ()
 {
@@ -1136,7 +1150,7 @@ linux_process_target::attach (unsigned long pid)
   err = attach_lwp (ptid);
   if (err != 0)
     {
-      remove_process (proc);
+      this->remove_linux_process (proc);
 
       std::string reason = linux_ptrace_attach_fail_reason_string (ptid, err);
       error ("Cannot attach to process %ld: %s", pid, reason.c_str ());
@@ -1565,8 +1579,6 @@ linux_process_target::detach (process_info *process)
 void
 linux_process_target::mourn (process_info *process)
 {
-  struct process_info_private *priv;
-
 #ifdef USE_THREAD_DB
   thread_db_mourn (process);
 #endif
@@ -1576,14 +1588,7 @@ linux_process_target::mourn (process_info *process)
       delete_lwp (get_thread_lwp (thread));
     });
 
-  /* Freeing all private data.  */
-  priv = process->priv;
-  close (priv->mem_fd);
-  low_delete_process (priv->arch_private);
-  free (priv);
-  process->priv = NULL;
-
-  remove_process (process);
+  this->remove_linux_process (process);
 }
 
 void
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 4284fb3348a..79be31b8f72 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -547,6 +547,9 @@ private:
      yet.  */
   process_info *add_linux_process_no_mem_file (int pid, int attached);
 
+  /* Free resources associated to PROC and remove it.  */
+  void remove_linux_process (process_info *proc); 
+
   /* Add a new thread.  */
   lwp_info *add_lwp (ptid_t ptid);