[PATCH] gdbserver/linux: free process_info_private and arch_process_info when failing to attach

[PATCH] gdbserver/linux: free process_info_private and arch_process_info when failing to attach

[Simon Marchi]

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 new delete_process_info_private target hook, and call it
from remove_process.  Move the free-ing of process_info_private from
linux_process_target::mourn to that new method.

I considered making process_info_private a base class and have the
implementations derive from it.  So we could have everything managed by
having process_info::priv be an std::unique_ptr.  But this has two
disadvantages:

  - It unnecessarily (since there is only one process_info_private
    implementation per GDBserver build) makes a class hierarchy with
    virtual inheritance out of it (not that it would cause any
    significant performance impact, but still).
  - Everywhere we use process_info_private in linux-low, we would need
    to down-cast it to linux_process_info_private.  Not a big deal, but
    still annoying.

Change-Id: I277059a69d5f08087a7f3ef0b8f1792a1fcf7a85
---
 gdbserver/inferiors.cc |  1 +
 gdbserver/linux-low.cc | 21 ++++++++++++---------
 gdbserver/linux-low.h  |  2 ++
 gdbserver/target.h     | 11 +++++++++++
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc
index 678d93c77a1c..b10f69fa91a8 100644
--- a/gdbserver/inferiors.cc
+++ b/gdbserver/inferiors.cc
@@ -149,6 +149,7 @@ add_process (int pid, int attached)
 void
 remove_process (struct process_info *process)
 {
+  target_delete_process_info_private (process);
   clear_symbol_cache (&process->symbol_cache);
   free_all_breakpoints (process);
   gdb_assert (find_thread_process (process) == NULL);
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 449c6641f58f..e91def3b10a2 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -1565,8 +1565,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,16 +1574,21 @@ 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);
 }
 
+void
+linux_process_target::delete_process_info_private (process_info *proc)
+{
+  if (proc->priv->mem_fd >= 0)
+    close (proc->priv->mem_fd);
+
+  low_delete_process (proc->priv->arch_private);
+
+  xfree (proc->priv);
+  proc->priv = nullptr;
+}
+
 void
 linux_process_target::join (int pid)
 {
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 4284fb3348a6..6d163799de5d 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -153,6 +153,8 @@ class linux_process_target : public process_stratum_target
 
   void mourn (process_info *proc) override;
 
+  void delete_process_info_private (process_info *proc) override;
+
   void join (int pid) override;
 
   bool thread_alive (ptid_t pid) override;
diff --git a/gdbserver/target.h b/gdbserver/target.h
index f3172e2ed7ef..0598345125e9 100644
--- a/gdbserver/target.h
+++ b/gdbserver/target.h
@@ -109,6 +109,11 @@ class process_stratum_target
   /* The inferior process has died.  Do what is right.  */
   virtual void mourn (process_info *proc) = 0;
 
+  /* Delete the process_info::priv field of PROC.
+
+     This is optional, the default implemention is a no-op.  */
+  virtual void delete_process_info_private (process_info *proc) {}
+
   /* Wait for process PID to exit.  */
   virtual void join (int pid) = 0;
 
@@ -697,6 +702,12 @@ target_thread_pending_child (thread_info *thread)
   return the_target->thread_pending_child (thread);
 }
 
+static inline void
+target_delete_process_info_private (process_info *proc)
+{
+  return the_target->delete_process_info_private (proc);
+}
+
 int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len);
 
 int set_desired_thread ();

base-commit: 6acc36f71dfc60e357496174672103f133f85e97
-- 
2.35.2

Re: [PATCH] gdbserver/linux: free process_info_private and arch_process_info when failing to attach

[Pedro Alves]

On 2022-04-22 16:34, Simon Marchi via Gdb-patches wrote:
> 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 new delete_process_info_private target hook, and call it
> from remove_process.  Move the free-ing of process_info_private from
> linux_process_target::mourn to that new method.

Offhand, it feels a bit surprising for this to be done differently from add_linux_process,
via common code calling into the target backend.  I mean, instead of doing the
reverse of add_linux_process.  I.e., add_linux_process calls add_process and then
adds the private bits.  We could have a remove_linux_process counterpart in linux-low.cc
that deletes the private bits and then calls remove_process.  We'd call remove_linux_process
instead of remove_process in linux-low.cc.  This would spare a virtual method, and it's
OK because common code should never delete the process directly anyhow.

I can't say I object to your approach, though.  It also works.  But please move
the delete_process_info_private method implementation next to
add_linux_process / add_linux_process_no_mem_file, so construction and destruction
are next to each other.

> 
> I considered making process_info_private a base class and have the
> implementations derive from it.  So we could have everything managed by
> having process_info::priv be an std::unique_ptr.  But this has two
> disadvantages:
> 
>   - It unnecessarily (since there is only one process_info_private
>     implementation per GDBserver build) makes a class hierarchy with
>     virtual inheritance out of it (not that it would cause any
>     significant performance impact, but still).
>   - Everywhere we use process_info_private in linux-low, we would need
>     to down-cast it to linux_process_info_private.  Not a big deal, but
>     still annoying.

The second downside reads a little weird since it only exists if no
virtual inheritance, which the first downside assumes.

Re: [PATCH] gdbserver/linux: free process_info_private and arch_process_info when failing to attach

[Simon Marchi]

On 2022-04-22 13:14, Pedro Alves wrote:
> On 2022-04-22 16:34, Simon Marchi via Gdb-patches wrote:
>> 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 new delete_process_info_private target hook, and call it
>> from remove_process.  Move the free-ing of process_info_private from
>> linux_process_target::mourn to that new method.
>
> Offhand, it feels a bit surprising for this to be done differently from add_linux_process,
> via common code calling into the target backend.  I mean, instead of doing the
> reverse of add_linux_process.  I.e., add_linux_process calls add_process and then
> adds the private bits.  We could have a remove_linux_process counterpart in linux-low.cc
> that deletes the private bits and then calls remove_process.  We'd call remove_linux_process
> instead of remove_process in linux-low.cc.  This would spare a virtual method, and it's
> OK because common code should never delete the process directly anyhow.
>
> I can't say I object to your approach, though.  It also works.  But please move
> the delete_process_info_private method implementation next to
> add_linux_process / add_linux_process_no_mem_file, so construction and destruction
> are next to each other.

Your suggestion is better / simpler.  It wouldn't work if processes
could be removed from outside the target, but that's not the case.

>
>>
>> I considered making process_info_private a base class and have the
>> implementations derive from it.  So we could have everything managed by
>> having process_info::priv be an std::unique_ptr.  But this has two
>> disadvantages:
>>
>>   - It unnecessarily (since there is only one process_info_private
>>     implementation per GDBserver build) makes a class hierarchy with
>>     virtual inheritance out of it (not that it would cause any
>>     significant performance impact, but still).
>>   - Everywhere we use process_info_private in linux-low, we would need
>>     to down-cast it to linux_process_info_private.  Not a big deal, but
>>     still annoying.
>
> The second downside reads a little weird since it only exists if no
> virtual inheritance, which the first downside assumes.

I mean: process_info::priv is a std::unique_ptr<process_info_private>.
In inferiors.h, we have:

  struct process_info_private
  {
    virtual ~process_info_private () = 0;
  };

In linux-low, we have:

  struct linux_process_info_private : public process_info_private
  {
    ... stuff ...
  };

Now, in linux-low.cc, every time we want to use process_info::priv, we
must downcast (or is it upcast? I never know) process_info::priv to a
linux_process_info_private to access the fields.  Anyway, doesn't
matter.

I pushed the patch after implementing your suggestion, since it became
relatively simple:

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

Simon