[PATCH 0/3] Fix "attach" infinite loop

[PATCH 0/3] Fix "attach" infinite loop

[Tom Tromey]

A user noticed that if you strace the non-main thread of a program,
then if you try to attach gdb to that program, gdb will enter an
infinite loop.

This series fixes this bug, but has a couple extra small cleanup
patches as well.

Regression tested on x86-64 Fedora 36.

---
Tom Tromey (3):
      Minor cleanup in linux_proc_attach_tgid_threads
      Use gdb_dir_up in linux_proc_attach_tgid_threads
      Bail out of "attach" if a thread cannot be traced

 gdb/linux-nat.c                          |  49 +++++++++++----
 gdb/nat/linux-procfs.c                   |  11 ++--
 gdb/testsuite/gdb.base/traced-thread.c   | 105 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/traced-thread.exp |  54 ++++++++++++++++
 gdbserver/linux-low.cc                   |  15 ++++-
 5 files changed, 214 insertions(+), 20 deletions(-)
---
base-commit: 1c9b44fe07569b6ed99557faa87d5b1c21413fbf
change-id: 20231003-attach-bug-ec9c00e86086

Best regards,
-- 
Tom Tromey <tromey@adacore.com>

[PATCH 1/3] Minor cleanup in linux_proc_attach_tgid_threads

[Tom Tromey]

linux_proc_attach_tgid_threads computes a file name, and then
re-computes it for a warning.  It is better to reuse the
already-computed name here.
---
 gdb/nat/linux-procfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index 9c2d1beb91f..95ecae443e4 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -287,7 +287,7 @@ linux_proc_attach_tgid_threads (pid_t pid,
   dir = opendir (pathname);
   if (dir == NULL)
     {
-      warning (_("Could not open /proc/%ld/task."), (long) pid);
+      warning (_("Could not open %s."), pathname);
       return;
     }
 

-- 
2.40.1

Re: [PATCH 1/3] Minor cleanup in linux_proc_attach_tgid_threads

[Simon Marchi]

On 2023-10-03 13:56, Tom Tromey via Gdb-patches wrote:
> linux_proc_attach_tgid_threads computes a file name, and then
> re-computes it for a warning.  It is better to reuse the
> already-computed name here.
> ---
>  gdb/nat/linux-procfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
> index 9c2d1beb91f..95ecae443e4 100644
> --- a/gdb/nat/linux-procfs.c
> +++ b/gdb/nat/linux-procfs.c
> @@ -287,7 +287,7 @@ linux_proc_attach_tgid_threads (pid_t pid,
>    dir = opendir (pathname);
>    if (dir == NULL)
>      {
> -      warning (_("Could not open /proc/%ld/task."), (long) pid);
> +      warning (_("Could not open %s."), pathname);
>        return;
>      }
>  
> 

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon

[PATCH 2/3] Use gdb_dir_up in linux_proc_attach_tgid_threads

[Tom Tromey]

This changes linux_proc_attach_tgid_threads to use gdb_dir_up.  This
makes it robust against exceptions.
---
 gdb/nat/linux-procfs.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index 95ecae443e4..c7d8ba2bd67 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -275,7 +275,6 @@ void
 linux_proc_attach_tgid_threads (pid_t pid,
 				linux_proc_attach_lwp_func attach_lwp)
 {
-  DIR *dir;
   char pathname[128];
   int new_threads_found;
   int iterations;
@@ -284,7 +283,7 @@ linux_proc_attach_tgid_threads (pid_t pid,
     return;
 
   xsnprintf (pathname, sizeof (pathname), "/proc/%ld/task", (long) pid);
-  dir = opendir (pathname);
+  gdb_dir_up dir (opendir (pathname));
   if (dir == NULL)
     {
       warning (_("Could not open %s."), pathname);
@@ -300,7 +299,7 @@ linux_proc_attach_tgid_threads (pid_t pid,
       struct dirent *dp;
 
       new_threads_found = 0;
-      while ((dp = readdir (dir)) != NULL)
+      while ((dp = readdir (dir.get ())) != NULL)
 	{
 	  unsigned long lwp;
 
@@ -321,10 +320,8 @@ linux_proc_attach_tgid_threads (pid_t pid,
 	  iterations = -1;
 	}
 
-      rewinddir (dir);
+      rewinddir (dir.get ());
     }
-
-  closedir (dir);
 }
 
 /* See linux-procfs.h.  */

-- 
2.40.1

Re: [PATCH 2/3] Use gdb_dir_up in linux_proc_attach_tgid_threads

[Simon Marchi]

On 2023-10-03 13:56, Tom Tromey via Gdb-patches wrote:
> This changes linux_proc_attach_tgid_threads to use gdb_dir_up.  This
> makes it robust against exceptions.
> ---
>  gdb/nat/linux-procfs.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
> index 95ecae443e4..c7d8ba2bd67 100644
> --- a/gdb/nat/linux-procfs.c
> +++ b/gdb/nat/linux-procfs.c
> @@ -275,7 +275,6 @@ void
>  linux_proc_attach_tgid_threads (pid_t pid,
>  				linux_proc_attach_lwp_func attach_lwp)
>  {
> -  DIR *dir;
>    char pathname[128];
>    int new_threads_found;
>    int iterations;
> @@ -284,7 +283,7 @@ linux_proc_attach_tgid_threads (pid_t pid,
>      return;
>  
>    xsnprintf (pathname, sizeof (pathname), "/proc/%ld/task", (long) pid);
> -  dir = opendir (pathname);
> +  gdb_dir_up dir (opendir (pathname));
>    if (dir == NULL)
>      {
>        warning (_("Could not open %s."), pathname);
> @@ -300,7 +299,7 @@ linux_proc_attach_tgid_threads (pid_t pid,
>        struct dirent *dp;
>  
>        new_threads_found = 0;
> -      while ((dp = readdir (dir)) != NULL)
> +      while ((dp = readdir (dir.get ())) != NULL)
>  	{
>  	  unsigned long lwp;
>  
> @@ -321,10 +320,8 @@ linux_proc_attach_tgid_threads (pid_t pid,
>  	  iterations = -1;
>  	}
>  
> -      rewinddir (dir);
> +      rewinddir (dir.get ());
>      }
> -
> -  closedir (dir);
>  }
>  
>  /* See linux-procfs.h.  */
> 

Approved-By: Simon Marchi <simon.marchi@efficios.com>

If you want to have more fun, I think it would be nice to add a
gdb_opendir function that wraps opendir and returns a gdb_dir_up.

Simon

[PATCH 3/3] Bail out of "attach" if a thread cannot be traced

[Tom Tromey]

On Linux, threads are treated much like separate processes by the
kernel.  In particular, it's possible to ptrace just a single thread.
If gdb tries to attach to a multi-threaded inferior, where a non-main
thread is already being traced (e.g., by strace), then gdb will get
into an infinite loop attempting to attach.

This patch fixes this problem by having the attach fail if ptrace
fails to attach to any thread of the inferior.
---
 gdb/linux-nat.c                          |  49 +++++++++++----
 gdb/testsuite/gdb.base/traced-thread.c   | 105 +++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.base/traced-thread.exp |  54 ++++++++++++++++
 gdbserver/linux-low.cc                   |  15 ++++-
 4 files changed, 210 insertions(+), 13 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 7e36ced6292..5974ab95390 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -1024,8 +1024,8 @@ attach_proc_task_lwp_callback (ptid_t ptid)
 	      std::string reason
 		= linux_ptrace_attach_fail_reason_string (ptid, err);
 
-	      warning (_("Cannot attach to lwp %d: %s"),
-		       lwpid, reason.c_str ());
+	      error (_("Cannot attach to lwp %d: %s"),
+		     lwpid, reason.c_str ());
 	    }
 	}
       else
@@ -1045,13 +1045,6 @@ attach_proc_task_lwp_callback (ptid_t ptid)
 
 	  /* So that wait collects the SIGSTOP.  */
 	  lp->resumed = 1;
-
-	  /* Also add the LWP to gdb's thread list, in case a
-	     matching libthread_db is not found (or the process uses
-	     raw clone).  */
-	  add_thread (linux_target, lp->ptid);
-	  set_running (linux_target, lp->ptid, true);
-	  set_executing (linux_target, lp->ptid, true);
 	}
 
       return 1;
@@ -1146,8 +1139,42 @@ linux_nat_target::attach (const char *args, int from_tty)
      of threads/LWPs, and those structures may well be corrupted.
      Note that once thread_db is loaded, we'll still use it to list
      threads and associate pthread info with each LWP.  */
-  linux_proc_attach_tgid_threads (lp->ptid.pid (),
-				  attach_proc_task_lwp_callback);
+  try
+    {
+      linux_proc_attach_tgid_threads (lp->ptid.pid (),
+				      attach_proc_task_lwp_callback);
+    }
+  catch (const gdb_exception_error &)
+    {
+      /* Failed to attach to some LWP.  Detach any we've already
+	 attached to.  */
+      iterate_over_lwps (ptid_t (ptid.pid ()),
+			 [] (struct lwp_info *lwp) -> int
+			 {
+			   /* Ignore errors when detaching.  */
+			   ptrace (PTRACE_DETACH, lwp->ptid.lwp (), 0, 0);
+			   delete_lwp (lwp->ptid);
+			   return 0;
+			 });
+
+      target_terminal::ours ();
+      target_mourn_inferior (inferior_ptid);
+
+      throw;
+    }
+
+  /* Add all the LWPs to gdb's thread list.  */
+  iterate_over_lwps (ptid_t (ptid.pid ()),
+		     [] (struct lwp_info *lwp) -> int
+		     {
+		       if (lwp->ptid.pid () != lwp->ptid.lwp ())
+			 {
+			   add_thread (linux_target, lwp->ptid);
+			   set_running (linux_target, lwp->ptid, true);
+			   set_executing (linux_target, lwp->ptid, true);
+			 }
+		       return 0;
+		     });
 }
 
 /* Ptrace-detach the thread with pid PID.  */
diff --git a/gdb/testsuite/gdb.base/traced-thread.c b/gdb/testsuite/gdb.base/traced-thread.c
new file mode 100644
index 00000000000..f30e09b1fc5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/traced-thread.c
@@ -0,0 +1,105 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#define _GNU_SOURCE
+
+#include <assert.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/ptrace.h>
+#include <pthread.h>
+#include <stdlib.h>
+#include <errno.h>
+
+int fds[2];
+
+_Atomic pid_t bg_tid = 0;
+
+pthread_barrier_t barrier;
+
+#define FIVE_MINUTES (5 * 60)
+
+/* One thread of the child process.  This is traced by the parent
+   process.  */
+void *
+block (void *ignore)
+{
+  bg_tid = gettid ();
+  pthread_barrier_wait (&barrier);
+  sleep (FIVE_MINUTES);
+  return 0;
+}
+
+/* The parent process blocks in this function.  */
+void
+parent_stop (pid_t child_thread_pid)
+{
+  sleep (FIVE_MINUTES);
+}
+
+int
+main ()
+{
+  int result;
+
+  pthread_barrier_init (&barrier, NULL, 2);
+
+  result = pipe (fds);
+  assert (result != -1);
+
+  pid_t child = fork ();
+  if (child != 0)
+    {
+      /* Parent.  */
+      close (fds[1]);
+
+      pid_t the_tid;
+      result = read (fds[0], &the_tid, sizeof (the_tid));
+      assert (result == sizeof (the_tid));
+
+      /* Trace a single, non-main thread of the child.  This should
+	 prevent gdb from attaching to the child at all.  The bug here
+	 was that gdb would get into an infinite loop repeatedly
+	 trying to attach to this thread.  */
+      result = ptrace (PTRACE_SEIZE, the_tid, (void *) 0, (void *) 0);
+      if (result == -1)
+	perror ("ptrace");
+
+      parent_stop (child);
+    }
+  else
+    {
+      /* Child.  */
+
+      close (fds[0]);
+
+      pthread_t thr;
+      result = pthread_create (&thr, 0, block, 0);
+      assert (result == 0);
+
+      /* Wait until the TID has been assigned.  */
+      pthread_barrier_wait (&barrier);
+      assert (bg_tid != 0);
+
+      result = write (fds[1], &bg_tid, sizeof (bg_tid));
+      assert (result == sizeof (bg_tid));
+
+      sleep (FIVE_MINUTES);
+    }
+
+  exit (0);
+}
diff --git a/gdb/testsuite/gdb.base/traced-thread.exp b/gdb/testsuite/gdb.base/traced-thread.exp
new file mode 100644
index 00000000000..d590ab1bc5e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/traced-thread.exp
@@ -0,0 +1,54 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+# This test only works on GNU/Linux.
+require !use_gdb_stub isnative
+require {!is_remote host}
+require {istarget *-linux*}
+
+standard_testfile
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
+	 {debug pthreads}]} {
+    return -1
+}
+
+if {![runto "parent_stop"]} {
+  return -1
+}
+
+# We don't want to be bothered.
+gdb_test_no_output "set confirm off"
+
+set child_pid [get_integer_valueof child_thread_pid -1 "get child pid"]
+
+# We should not be able to attach to the child at all, because one
+# thread is being traced.  The bug here was that gdb would get into an
+# infinite loop trying to attach to this thread.
+gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"
+gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2"
+# Recognize failures from either gdb or gdbserver.
+gdb_test "attach $child_pid" \
+    "(Cannot attach to|Attaching to process $decimal failed).*" \
+    "should not attach to child process"
+
+
+# Now kill the parent process, ending the trace.
+gdb_test "inferior 1" "Switching to inferior 1.*" "switch to inferior 1"
+gdb_test "kill" ".*" "kill the parent process"
+
+# Kill the child process as well.  Use the shell to avoid funny
+# business with gdbserver testing.
+remote_exec target "kill -9 $child_pid"
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 40b6a907ad9..cdaf730af47 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -1128,7 +1128,7 @@ attach_proc_task_lwp_callback (ptid_t ptid)
 	  std::string reason
 	    = linux_ptrace_attach_fail_reason_string (ptid, err);
 
-	  warning (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ());
+	  error (_("Cannot attach to lwp %d: %s"), lwpid, reason.c_str ());
 	}
 
       return 1;
@@ -1181,7 +1181,18 @@ linux_process_target::attach (unsigned long pid)
      threads/LWPs, and those structures may well be corrupted.  Note
      that once thread_db is loaded, we'll still use it to list threads
      and associate pthread info with each LWP.  */
-  linux_proc_attach_tgid_threads (pid, attach_proc_task_lwp_callback);
+  try
+    {
+      linux_proc_attach_tgid_threads (pid, attach_proc_task_lwp_callback);
+    }
+  catch (const gdb_exception_error &)
+    {
+      /* Make sure we do not deliver the SIGSTOP to the process.  */
+      initial_thread->last_resume_kind = resume_continue;
+
+      this->detach (proc);
+      throw;
+    }
 
   /* GDB will shortly read the xml target description for this
      process, to figure out the process' architecture.  But the target

-- 
2.40.1

Re: [PATCH 3/3] Bail out of "attach" if a thread cannot be traced

[Simon Marchi]

On 2023-10-03 13:56, Tom Tromey via Gdb-patches wrote:
> On Linux, threads are treated much like separate processes by the
> kernel.  In particular, it's possible to ptrace just a single thread.
> If gdb tries to attach to a multi-threaded inferior, where a non-main
> thread is already being traced (e.g., by strace), then gdb will get
> into an infinite loop attempting to attach.

Can you explain why GDB falls in an infinite loop?

> 
> This patch fixes this problem by having the attach fail if ptrace
> fails to attach to any thread of the inferior.

I don't rely on that behavior myself, but I would think that the current
behavior of just warning when failing to attach to a thread was on
purpose, at least it allows the user to debug the other threads.  It
might be useful in some edge case scenarios I guess?

> ---
>  gdb/linux-nat.c                          |  49 +++++++++++----
>  gdb/testsuite/gdb.base/traced-thread.c   | 105 +++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/traced-thread.exp |  54 ++++++++++++++++
>  gdbserver/linux-low.cc                   |  15 ++++-
>  4 files changed, 210 insertions(+), 13 deletions(-)
> 
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 7e36ced6292..5974ab95390 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1024,8 +1024,8 @@ attach_proc_task_lwp_callback (ptid_t ptid)
>  	      std::string reason
>  		= linux_ptrace_attach_fail_reason_string (ptid, err);
>  
> -	      warning (_("Cannot attach to lwp %d: %s"),
> -		       lwpid, reason.c_str ());
> +	      error (_("Cannot attach to lwp %d: %s"),
> +		     lwpid, reason.c_str ());
>  	    }
>  	}
>        else
> @@ -1045,13 +1045,6 @@ attach_proc_task_lwp_callback (ptid_t ptid)
>  
>  	  /* So that wait collects the SIGSTOP.  */
>  	  lp->resumed = 1;
> -
> -	  /* Also add the LWP to gdb's thread list, in case a
> -	     matching libthread_db is not found (or the process uses
> -	     raw clone).  */
> -	  add_thread (linux_target, lp->ptid);
> -	  set_running (linux_target, lp->ptid, true);
> -	  set_executing (linux_target, lp->ptid, true);
>  	}
>  
>        return 1;
> @@ -1146,8 +1139,42 @@ linux_nat_target::attach (const char *args, int from_tty)
>       of threads/LWPs, and those structures may well be corrupted.
>       Note that once thread_db is loaded, we'll still use it to list
>       threads and associate pthread info with each LWP.  */
> -  linux_proc_attach_tgid_threads (lp->ptid.pid (),
> -				  attach_proc_task_lwp_callback);
> +  try
> +    {
> +      linux_proc_attach_tgid_threads (lp->ptid.pid (),
> +				      attach_proc_task_lwp_callback);
> +    }
> +  catch (const gdb_exception_error &)
> +    {
> +      /* Failed to attach to some LWP.  Detach any we've already
> +	 attached to.  */
> +      iterate_over_lwps (ptid_t (ptid.pid ()),
> +			 [] (struct lwp_info *lwp) -> int
> +			 {
> +			   /* Ignore errors when detaching.  */
> +			   ptrace (PTRACE_DETACH, lwp->ptid.lwp (), 0, 0);
> +			   delete_lwp (lwp->ptid);
> +			   return 0;
> +			 });
> +
> +      target_terminal::ours ();
> +      target_mourn_inferior (inferior_ptid);

I'm wondering if the target_mourn_inferior could be done in back in
generic code, it sounds relevant for any target failing to attach.

Another thing to consider when attach fails: inf_ptrace_target::attach
takes care of unpushing itself if it fails and it wasn't pushed
initially.  Should linux_nat_target::attach do the same?  This is
something that could maybe get checked in the test by checking the
output of "maint print target-stack".

> +
> +      throw;
> +    }
> +
> +  /* Add all the LWPs to gdb's thread list.  */
> +  iterate_over_lwps (ptid_t (ptid.pid ()),
> +		     [] (struct lwp_info *lwp) -> int
> +		     {
> +		       if (lwp->ptid.pid () != lwp->ptid.lwp ())
> +			 {
> +			   add_thread (linux_target, lwp->ptid);
> +			   set_running (linux_target, lwp->ptid, true);
> +			   set_executing (linux_target, lwp->ptid, true);
> +			 }
> +		       return 0;
> +		     });
>  }
>  
>  /* Ptrace-detach the thread with pid PID.  */
> diff --git a/gdb/testsuite/gdb.base/traced-thread.c b/gdb/testsuite/gdb.base/traced-thread.c
> new file mode 100644
> index 00000000000..f30e09b1fc5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/traced-thread.c
> @@ -0,0 +1,105 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2023 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#define _GNU_SOURCE
> +
> +#include <assert.h>
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <sys/ptrace.h>
> +#include <pthread.h>
> +#include <stdlib.h>
> +#include <errno.h>
> +
> +int fds[2];
> +
> +_Atomic pid_t bg_tid = 0;
> +
> +pthread_barrier_t barrier;
> +
> +#define FIVE_MINUTES (5 * 60)
> +
> +/* One thread of the child process.  This is traced by the parent
> +   process.  */
> +void *
> +block (void *ignore)
> +{
> +  bg_tid = gettid ();
> +  pthread_barrier_wait (&barrier);
> +  sleep (FIVE_MINUTES);
> +  return 0;
> +}
> +
> +/* The parent process blocks in this function.  */
> +void
> +parent_stop (pid_t child_thread_pid)
> +{
> +  sleep (FIVE_MINUTES);
> +}
> +
> +int
> +main ()
> +{
> +  int result;
> +
> +  pthread_barrier_init (&barrier, NULL, 2);
> +
> +  result = pipe (fds);
> +  assert (result != -1);
> +
> +  pid_t child = fork ();
> +  if (child != 0)
> +    {
> +      /* Parent.  */
> +      close (fds[1]);
> +
> +      pid_t the_tid;
> +      result = read (fds[0], &the_tid, sizeof (the_tid));
> +      assert (result == sizeof (the_tid));
> +
> +      /* Trace a single, non-main thread of the child.  This should
> +	 prevent gdb from attaching to the child at all.  The bug here
> +	 was that gdb would get into an infinite loop repeatedly
> +	 trying to attach to this thread.  */
> +      result = ptrace (PTRACE_SEIZE, the_tid, (void *) 0, (void *) 0);
> +      if (result == -1)
> +	perror ("ptrace");
> +
> +      parent_stop (child);
> +    }
> +  else
> +    {
> +      /* Child.  */
> +
> +      close (fds[0]);
> +
> +      pthread_t thr;
> +      result = pthread_create (&thr, 0, block, 0);
> +      assert (result == 0);
> +
> +      /* Wait until the TID has been assigned.  */
> +      pthread_barrier_wait (&barrier);
> +      assert (bg_tid != 0);
> +
> +      result = write (fds[1], &bg_tid, sizeof (bg_tid));
> +      assert (result == sizeof (bg_tid));
> +
> +      sleep (FIVE_MINUTES);
> +    }
> +
> +  exit (0);
> +}
> diff --git a/gdb/testsuite/gdb.base/traced-thread.exp b/gdb/testsuite/gdb.base/traced-thread.exp
> new file mode 100644
> index 00000000000..d590ab1bc5e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/traced-thread.exp
> @@ -0,0 +1,54 @@
> +# Copyright 2023 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +# This test only works on GNU/Linux.
> +require !use_gdb_stub isnative
> +require {!is_remote host}
> +require {istarget *-linux*}

Can you add a short description at the top to indicate the intent of the
test?

> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	 {debug pthreads}]} {
> +    return -1
> +}
> +
> +if {![runto "parent_stop"]} {
> +  return -1
> +}
> +
> +# We don't want to be bothered.
> +gdb_test_no_output "set confirm off"
> +
> +set child_pid [get_integer_valueof child_thread_pid -1 "get child pid"]
> +
> +# We should not be able to attach to the child at all, because one
> +# thread is being traced.  The bug here was that gdb would get into an
> +# infinite loop trying to attach to this thread.
> +gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"
> +gdb_test "inferior 2" "Switching to inferior 2.*" "switch to inferior 2"
> +# Recognize failures from either gdb or gdbserver.
> +gdb_test "attach $child_pid" \
> +    "(Cannot attach to|Attaching to process $decimal failed).*" \
> +    "should not attach to child process"
> +
> +
> +# Now kill the parent process, ending the trace.
> +gdb_test "inferior 1" "Switching to inferior 1.*" "switch to inferior 1"
> +gdb_test "kill" ".*" "kill the parent process"
> +
> +# Kill the child process as well.  Use the shell to avoid funny
> +# business with gdbserver testing.
> +remote_exec target "kill -9 $child_pid"

To further verify that the child we failed to attach to was left in a
good state, it might be good to check if GDB can successfully attach it
after killing the parent.  And while at it, we could kill the child
using GDB's kill command too.

Simon

Re: [PATCH 3/3] Bail out of "attach" if a thread cannot be traced

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Quick response to just this one bit, will read the rest a little later...

>> On Linux, threads are treated much like separate processes by the
>> kernel.  In particular, it's possible to ptrace just a single thread.
>> If gdb tries to attach to a multi-threaded inferior, where a non-main
>> thread is already being traced (e.g., by strace), then gdb will get
>> into an infinite loop attempting to attach.

Simon> Can you explain why GDB falls in an infinite loop?

gdb has to iterate over all the threads, trying to ptrace each one.
It stops when it has done two iterations without finding a new thread.
This is because Linux doesn't have a way to ptrace to all threads and
stop the entire process atomically - so threads might be spawning new
threads during the attach operation.

In this situation, gdb can't attach to one particular thread.  It keeps
trying that one over and over, since there's no real provision for the
attach to fail as a whole... which is what the patch adds.

>> This patch fixes this problem by having the attach fail if ptrace
>> fails to attach to any thread of the inferior.

Simon> I don't rely on that behavior myself, but I would think that the current
Simon> behavior of just warning when failing to attach to a thread was on
Simon> purpose, at least it allows the user to debug the other threads.  It
Simon> might be useful in some edge case scenarios I guess?

Yes, I don't really know.  However, it seemed to me to be less bad to
simply fail entirely.  We could make gdb just ignore these
already-traced threads -- but then debugging might be quite strange,
because the untraced thread would never stop.

Tom

Re: [PATCH 3/3] Bail out of "attach" if a thread cannot be traced

[Simon Marchi]

On 10/4/23 10:14, Tom Tromey wrote:
> Simon> I don't rely on that behavior myself, but I would think that the current
> Simon> behavior of just warning when failing to attach to a thread was on
> Simon> purpose, at least it allows the user to debug the other threads.  It
> Simon> might be useful in some edge case scenarios I guess?
> 
> Yes, I don't really know.  However, it seemed to me to be less bad to
> simply fail entirely.  We could make gdb just ignore these
> already-traced threads -- but then debugging might be quite strange,
> because the untraced thread would never stop.

Yeah, and it will be weird if an untraced thread hits a breakpoint.
Whatever traces it will get notified that the thread received a SIGTRAP.
I would ask Pedro for his opinion, just to be sure.

Simon

Re: [PATCH 0/3] Fix "attach" infinite loop

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> A user noticed that if you strace the non-main thread of a program,
Tom> then if you try to attach gdb to that program, gdb will enter an
Tom> infinite loop.

Tom> This series fixes this bug, but has a couple extra small cleanup
Tom> patches as well.

Tom> Regression tested on x86-64 Fedora 36.

I'm going to push this series now.

Tom

Re: [PATCH 0/3] Fix "attach" infinite loop

[Luis Machado]

Hi Tom,

On 12/1/23 17:41, Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> A user noticed that if you strace the non-main thread of a program,
> Tom> then if you try to attach gdb to that program, gdb will enter an
> Tom> infinite loop.
> 
> Tom> This series fixes this bug, but has a couple extra small cleanup
> Tom> patches as well.
> 
> Tom> Regression tested on x86-64 Fedora 36.
> 
> I'm going to push this series now.
> 
> Tom

I think patch 3 of this series (Bail out of "attach" if a thread cannot be traced) might have
made gdb.threads/attach-many-short-lived-threads.exp unhappy. At least that's what the bisect
indicates.

I'm getting the following on aarch64-linux Ubuntu 22.04:

FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 1 (the program is no longer running)
FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 2 (the program is no longer running)
FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 3 (the program is no longer running)
FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: reset timer in the inferior
FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: detach (the program is no longer running)

Re: [PATCH 0/3] Fix "attach" infinite loop

[Tom Tromey]

>>>>> "Luis" == Luis Machado <luis.machado@arm.com> writes:

Luis> I'm getting the following on aarch64-linux Ubuntu 22.04:

Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 1 (the program is no longer running)
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 2 (the program is no longer running)
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 3 (the program is no longer running)
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: reset timer in the inferior
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: detach (the program is no longer running)

I was able to reproduce this, I'll take a look.

Tom

Re: [PATCH 0/3] Fix "attach" infinite loop

[Tom Tromey]

Luis> I'm getting the following on aarch64-linux Ubuntu 22.04:

Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 1 (the program is no longer running)
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 2 (the program is no longer running)
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 3 (the program is no longer running)
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: reset timer in the inferior
Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: detach (the program is no longer running)

Tom> I was able to reproduce this, I'll take a look.

Well, I can't reproduce this any more -- I managed to make it fail once,
when I wrote that email, but not again.  I wonder if this is one of the
intermittent failure tests?

If it fails consistently for you, maybe sending the gdb.log would help.
Not sure though.

Tom

Re: [PATCH 0/3] Fix "attach" infinite loop

[Luis Machado]

On 12/15/23 19:33, Tom Tromey wrote:
> Luis> I'm getting the following on aarch64-linux Ubuntu 22.04:
> 
> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 1 (the program is no longer running)
> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 2 (the program is no longer running)
> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 3 (the program is no longer running)
> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: reset timer in the inferior
> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: detach (the program is no longer running)
> 
> Tom> I was able to reproduce this, I'll take a look.
> 
> Well, I can't reproduce this any more -- I managed to make it fail once,
> when I wrote that email, but not again.  I wonder if this is one of the
> intermittent failure tests?
> 
> If it fails consistently for you, maybe sending the gdb.log would help.
> Not sure though.
> 
> Tom

Sure. Let me check what the current situation is.

Re: [PATCH 0/3] Fix "attach" infinite loop

[Luis Machado]

On 12/18/23 09:50, Luis Machado wrote:
> On 12/15/23 19:33, Tom Tromey wrote:
>> Luis> I'm getting the following on aarch64-linux Ubuntu 22.04:
>>
>> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 1 (the program is no longer running)
>> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 2 (the program is no longer running)
>> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 3 (the program is no longer running)
>> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: reset timer in the inferior
>> Luis> FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: detach (the program is no longer running)
>>
>> Tom> I was able to reproduce this, I'll take a look.
>>
>> Well, I can't reproduce this any more -- I managed to make it fail once,
>> when I wrote that email, but not again.  I wonder if this is one of the
>> intermittent failure tests?
>>
>> If it fails consistently for you, maybe sending the gdb.log would help.
>> Not sure though.
>>
>> Tom
> 
> Sure. Let me check what the current situation is.

The most important bit seems to be this fragment:

--

(gdb) file /binutils-gdb-arm64-focal/gdb/testsuite/outputs/gdb.threads/attach-many-short-lived-threads/attach-many-short-lived-threads^M
Reading symbols from /binutils-gdb-arm64-focal/gdb/testsuite/outputs/gdb.threads/attach-many-short-lived-threads/attach-many-short-lived-threads...
(gdb) builtin_spawn /binutils-gdb-arm64-focal/gdb/testsuite/outputs/gdb.threads/attach-many-short-lived-threads/attach-many-short-lived-threads
attach 1545611
Attaching to program: /binutils-gdb-arm64-focal/gdb/testsuite/outputs/gdb.threads/attach-many-short-lived-threads/attach-many-short-lived-threads, process 1545611
Cannot attach to lwp 1609323: Operation not permitted (1)
(gdb) PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 1: attach     
info threads
No threads.
(gdb) PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 1: no new threads
set breakpoint always-inserted on
(gdb) PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 1: set breakpoint always-inserted on
break break_fn
Breakpoint 1 at 0xd64: file /binutils-gdb-arm64-focal/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.threads/attach-many-short-lived-threads.c, line 57.
(gdb) PASS: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break break_fn
continue
The program is not being run.
(gdb) FAIL: gdb.threads/attach-many-short-lived-threads.exp: iter 1: break at break_fn: 1 (the program is no longer running)

--

We spawn the process and try to attach to it, but it (supposedly) fails. I see the test seems
to assume the attaching succeeded even though ptrace returned EPERM.

There seems to be some kernel-related issue here, as I see the failures for a 5.11-based
machine but don't see it for a 5.15-based one.

Are we missing XFAIL-ing the test when we see the ptrace attach failure?

Re: [PATCH 0/3] Fix "attach" infinite loop

[Tom Tromey]

Luis> Attaching to program: /binutils-gdb-arm64-focal/gdb/testsuite/outputs/gdb.threads/attach-many-short-lived-threads/attach-many-short-lived-threads, process 1545611
Luis> Cannot attach to lwp 1609323: Operation not permitted (1)

Luis> Are we missing XFAIL-ing the test when we see the ptrace attach failure?

I see this in the test case:

    -re "warning: Cannot attach to lwp $decimal: Operation not permitted" {
        # On Linux, PTRACE_ATTACH sometimes fails with
        # EPERM, even though /proc/PID/status indicates
        # the thread is running.
        set eperm 1
        exp_continue
    }

However, this changed from a warning to an error.

Could you try removing the 'warning: ' text and see if that helps?  It
seems like it should issue an xfail instead.

Tom

Re: [PATCH 0/3] Fix "attach" infinite loop

[Luis Machado]

On 12/18/23 14:30, Tom Tromey wrote:
> Luis> Attaching to program: /binutils-gdb-arm64-focal/gdb/testsuite/outputs/gdb.threads/attach-many-short-lived-threads/attach-many-short-lived-threads, process 1545611
> Luis> Cannot attach to lwp 1609323: Operation not permitted (1)
> 
> Luis> Are we missing XFAIL-ing the test when we see the ptrace attach failure?
> 
> I see this in the test case:
> 
>     -re "warning: Cannot attach to lwp $decimal: Operation not permitted" {
>         # On Linux, PTRACE_ATTACH sometimes fails with
>         # EPERM, even though /proc/PID/status indicates
>         # the thread is running.
>         set eperm 1
>         exp_continue
>     }
> 
> However, this changed from a warning to an error.

Ah, I failed to spot that.

> 
> Could you try removing the 'warning: ' text and see if that helps?  It
> seems like it should issue an xfail instead.

I played with it for a bit, and it seems the testcase is somewhat broken.

It seems to assume everything is OK if it sees the message "Attaching to program: .*", which explains
the PASS we get from the above test.

If we ever see the EPERM error, we will set the eperm variable, but it will not matter, because the
matching for the attachment message will gobble things all the way to the gdb prompt, and will issue
a PASS, resuming the test (assuming there are live threads) and failing.

I think the testcase needs a bit of a rework to bail out if we fail to attach to the process. I'll
give it a go since it reproduces easily for me.

Re: [PATCH 0/3] Fix "attach" infinite loop

[Carl Love]

On Mon, 2023-12-18 at 15:10 +0000, Luis Machado wrote:
> On 12/18/23 14:30, Tom Tromey wrote:

<snip>
> 
> I think the testcase needs a bit of a rework to bail out if we fail
> to attach to the process. I'll
> give it a go since it reproduces easily for me.

I am seeing a lot of failures for this test on Power as well.  I would
be happy to help test any patches to fix this issue.  Please let me
know if I can help with this.  Thanks.

                     Carl