[PATCH 1/2] gdb: push target earlier in procfs_target::attach (PR 27435)

[PATCH 1/2] gdb: push target earlier in procfs_target::attach (PR 27435)

[Simon Marchi]

Since this is a GDB 9 -> 10 regression, I would like to push it to
gdb-10-branch.

This is a follow-up to:

  https://sourceware.org/pipermail/gdb-patches/2021-February/176202.html

This patch fixes a segfault seen when attaching to a process on Solaris.
The steps leading to the segfault are:

 - procfs_target::attach calls do_attach, at this point the inferior's
   process slot in the target stack is empty.
 - do_attach adds a thread with `add_thread (&the_procfs_target, ptid)`
 - in add_thread_silent, the passed target (&the_procfs_target) is
   passed to find_inferior_ptid
 - find_inferior_ptid returns nullptr, as there is no inferior with this
   ptid that has &the_procfs_target as its process target
 - the nullptr `inf` is passed to find_thread_ptid, which dereferences
   it, causing a segfault
 - back in procfs_target::attach, after do_attach, we push the
   the_procfs_target on the inferior's target stack, although we never
   reach this because the segfault happens before.

To fix this, I think we need to do the same as is done in
inf_ptrace_target::attach: push the target early and unpush it in case
the attach fails (and keep it if the attach succeeds).

Implement it by moving target_unpush_up to target.h, so it can be
re-used here.  Make procfs_target::attach use it.  Note that just like
is mentioned in inf_ptrace_target::attach, we should push the target
before calling target_pid_to_str, so that calling target_pid_to_str ends
up in procfs_target::pid_to_str.

Tested by trying to attach on a process on gcc211 on the gcc compile
farm.

gdb/ChangeLog:

	PR gdb/27435
	* inf-ptrace.c (struct target_unpusher): Move to target.h.
	(target_unpush_up): Likewise.
	* procfs.c (procfs_target::attach): Push target early.  Use
	target_unpush_up to unpush target in case of error.
	* target.h (struct target_unpusher): Move here.
	(target_unpush_up): Likewise.

Change-Id: I88aff8b20204e1ca1d792e27ac6bc34fc1aa0d52
---
 gdb/inf-ptrace.c | 16 ----------------
 gdb/procfs.c     | 14 ++++++++++++--
 gdb/target.h     | 14 ++++++++++++++
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/gdb/inf-ptrace.c b/gdb/inf-ptrace.c
index 0f2f56cc3660..7ca02dfd8764 100644
--- a/gdb/inf-ptrace.c
+++ b/gdb/inf-ptrace.c
@@ -49,22 +49,6 @@ gdb_ptrace (PTRACE_TYPE_ARG1 request, ptid_t ptid, PTRACE_TYPE_ARG3 addr,
 #endif
 }
 
-/* A unique_ptr helper to unpush a target.  */
-
-struct target_unpusher
-{
-  void operator() (struct target_ops *ops) const
-  {
-    unpush_target (ops);
-  }
-};
-
-/* A unique_ptr that unpushes a target on destruction.  */
-
-typedef std::unique_ptr<struct target_ops, target_unpusher> target_unpush_up;
-
-\f
-
 inf_ptrace_target::~inf_ptrace_target ()
 {}
 
diff --git a/gdb/procfs.c b/gdb/procfs.c
index e73faa8d41dc..cab29c3cbbcb 100644
--- a/gdb/procfs.c
+++ b/gdb/procfs.c
@@ -1767,6 +1767,14 @@ procfs_target::attach (const char *args, int from_tty)
   if (pid == getpid ())
     error (_("Attaching GDB to itself is not a good idea..."));
 
+  /* Push the target if needed, ensure it gets un-pushed it if attach fails.  */
+  target_unpush_up unpusher;
+  if (!target_is_pushed (this))
+    {
+      push_target (this);
+      unpusher.reset (this);
+    }
+
   if (from_tty)
     {
       const char *exec_file = get_exec_file (0);
@@ -1780,9 +1788,11 @@ procfs_target::attach (const char *args, int from_tty)
 
       fflush (stdout);
     }
+
   do_attach (ptid_t (pid));
-  if (!target_is_pushed (this))
-    push_target (this);
+
+  /* Everything went fine, keep the target pushed.  */
+  unpusher.release ();
 }
 
 void
diff --git a/gdb/target.h b/gdb/target.h
index 0de78075e9b5..52e23b05ffd8 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -2392,6 +2392,20 @@ extern void push_target (target_ops_up &&);
 
 extern int unpush_target (struct target_ops *);
 
+/* A unique_ptr helper to unpush a target.  */
+
+struct target_unpusher
+{
+  void operator() (struct target_ops *ops) const
+  {
+    unpush_target (ops);
+  }
+};
+
+/* A unique_ptr that unpushes a target on destruction.  */
+
+typedef std::unique_ptr<struct target_ops, target_unpusher> target_unpush_up;
+
 extern void target_pre_inferior (int);
 
 extern void target_preopen (int);
-- 
2.30.1

[PATCH 2/2] gdb: add asserts in thread code

[Simon Marchi]

Unlike the previous patch, I don't propose that we take this patch into
gdb-10-branch.

This patch adds two asserts, prompted by investigating and fixing the
bug fixed by the previous patch.

The assert in find_thread_ptid would have caught the original issue
before the segfault (I think it's slightly more use friendly).

The assert in add_thread_silent would have made it clear that the
solution proposed in [1] isn't the right one.  The solution ended up
passing nullptr as a target to add_thread.  We don't want that, because
add_thread_silent uses it to look up the inferior to which to add the
thread.  If the target is nullptr, we could find an inferior with the
same pid, but belonging to an unrelated target.  So we always want a
non-nullptr target in add_thread_silent.

gdb/ChangeLog:

	* thread.c (add_thread_silent): Add assert.
	(find_thread_ptid): Add assert.

[1] https://sourceware.org/pipermail/gdb-patches/2021-February/176202.html

Change-Id: Ie593ee45c5eb02235e8e9fbcda612d48ce883852
---
 gdb/thread.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/gdb/thread.c b/gdb/thread.c
index 821070672171..3e7d6e14bf74 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -246,6 +246,8 @@ new_thread (struct inferior *inf, ptid_t ptid)
 struct thread_info *
 add_thread_silent (process_stratum_target *targ, ptid_t ptid)
 {
+  gdb_assert (targ != nullptr);
+
   inferior *inf = find_inferior_ptid (targ, ptid);
 
   /* We may have an old thread with the same id in the thread list.
@@ -535,6 +537,8 @@ find_thread_ptid (process_stratum_target *targ, ptid_t ptid)
 struct thread_info *
 find_thread_ptid (inferior *inf, ptid_t ptid)
 {
+  gdb_assert (inf != nullptr);
+
   for (thread_info *tp : inf->non_exited_threads ())
     if (tp->ptid == ptid)
       return tp;
-- 
2.30.1

Re: [PATCH 1/2] gdb: push target earlier in procfs_target::attach (PR 27435)

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Tested by trying to attach on a process on gcc211 on the gcc compile
Simon> farm.

Simon> gdb/ChangeLog:

Simon> 	PR gdb/27435
Simon> 	* inf-ptrace.c (struct target_unpusher): Move to target.h.
Simon> 	(target_unpush_up): Likewise.
Simon> 	* procfs.c (procfs_target::attach): Push target early.  Use
Simon> 	target_unpush_up to unpush target in case of error.
Simon> 	* target.h (struct target_unpusher): Move here.
Simon> 	(target_unpush_up): Likewise.

FWIW this looks reasonable to me.

Tom

Re: [PATCH 1/2] gdb: push target earlier in procfs_target::attach (PR 27435)

[Simon Marchi]

On 2021-02-21 4:04 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Tested by trying to attach on a process on gcc211 on the gcc compile
> Simon> farm.
> 
> Simon> gdb/ChangeLog:
> 
> Simon> 	PR gdb/27435
> Simon> 	* inf-ptrace.c (struct target_unpusher): Move to target.h.
> Simon> 	(target_unpush_up): Likewise.
> Simon> 	* procfs.c (procfs_target::attach): Push target early.  Use
> Simon> 	target_unpush_up to unpush target in case of error.
> Simon> 	* target.h (struct target_unpusher): Move here.
> Simon> 	(target_unpush_up): Likewise.
> 
> FWIW this looks reasonable to me.
> 
> Tom

Thanks, I'll wait for Libor's feedback before pushing.

Simon

Re: Re: [PATCH 1/2] gdb: push target earlier in procfs_target::attach (PR 27435)

[Libor Bukata]

Many thanks to Simon for the patch.

I confirm that it fixes the attach crash on Solaris x86/sparc.
The testsuite did not reveal any new regressions on Fedora Linux.
Regarding Solaris, the tests require much more time,
so, I will let you know when I have results.

AFAIK, the code changes look good.

Libor

On 2/21/21 10:04 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> Simon> Tested by trying to attach on a process on gcc211 on the gcc compile
> Simon> farm.
>
> Simon> gdb/ChangeLog:
>
> Simon> 	PR gdb/27435
> Simon> 	* inf-ptrace.c (struct target_unpusher): Move to target.h.
> Simon> 	(target_unpush_up): Likewise.
> Simon> 	* procfs.c (procfs_target::attach): Push target early.  Use
> Simon> 	target_unpush_up to unpush target in case of error.
> Simon> 	* target.h (struct target_unpusher): Move here.
> Simon> 	(target_unpush_up): Likewise.
>
> FWIW this looks reasonable to me.
>
> Tom

Re: [PATCH 1/2] gdb: push target earlier in procfs_target::attach (PR 27435)

[Simon Marchi]

On 2021-02-22 6:03 a.m., Libor Bukata wrote:> Many thanks to Simon for the patch.
> 
> I confirm that it fixes the attach crash on Solaris x86/sparc.
> The testsuite did not reveal any new regressions on Fedora Linux.
> Regarding Solaris, the tests require much more time,
> so, I will let you know when I have results.
> 
> AFAIK, the code changes look good.

Thanks, I pushed the series to the master branch, and patch 1 to
gdb-10-branch.  Please let me know if you find unexpected regressions on
Solaris (although hopefully, you'll see some tests going from FAIL to
PASS).

Simon

Re: Re: [PATCH 1/2] gdb: push target earlier in procfs_target::attach (PR 27435)

[Libor Bukata]

Thank you for the integration. Unit tests did not reveal any new 
regressions on Solaris.

Libor

On 2/22/21 5:51 PM, Simon Marchi wrote:
> On 2021-02-22 6:03 a.m., Libor Bukata wrote:> Many thanks to Simon for the patch.
>> I confirm that it fixes the attach crash on Solaris x86/sparc.
>> The testsuite did not reveal any new regressions on Fedora Linux.
>> Regarding Solaris, the tests require much more time,
>> so, I will let you know when I have results.
>>
>> AFAIK, the code changes look good.
> Thanks, I pushed the series to the master branch, and patch 1 to
> gdb-10-branch.  Please let me know if you find unexpected regressions on
> Solaris (although hopefully, you'll see some tests going from FAIL to
> PASS).
>
> Simon