public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter
@ 2023-03-31  3:44 Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 1/5] gdbserver: Use current_process in handle_qxfer_auxv Thiago Jung Bauermann
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Thiago Jung Bauermann @ 2023-03-31  3:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

Hello,

Back in February, I pushed commit 43e5fbd8b788 ("gdbserver: Add PID
parameter to linux_get_auxv and linux_get_hwcap"), and mentioned that
there was no change in gdbserver behaviour. Pedro corrected me,
mentioning that before the patch gdbserver would read the auxv from
/proc/<current_thread's LWP>/auxv, while after the patch it would read
it from /proc/<current_thread's PID>/auxv. This causes trouble in case
the inferior's main thread exits.

I created a testcase exercising this scenario (the last patch in this
series) and confirmed that both GDB and gdbserver have this problem (GDB
wasn't changed by my patch). The problem is largely mitigated by GDB's
auxv cache though, and is very hard to hit because one of the first
things that GDB does when starting/attaching to an inferior is read the
auxv and cache it. It will only be a problem if the cache is invalidated
by one of the cache-clearing events ("inferior_exit",
"inferior_appeared", "executable_changed").

In the discussion about my patch there were also some questions about
other race conditions in this area. I created a test program to
experiment and these are the results:

Q1: What happens exactly if GDB/gdbserver tries to read
    /proc/<thread's PID>/auxv after the main thread exits?
A1: If GDB/gdbserver is root, then open() will succeed but read() will
    return 0, indicating an empty file. If GDB/gdbserver isn't root,
    then open() will fail with errno = EACCES ("Permission denied") even
    if it's running as the same user as the inferior.

Q2: What happens if an inferior thread exits after GDB/gdbserver opens its
    /proc/<thread's LWP>/auxv file but before it has a chance to read it?
A2: The read() call will return 0, indicating an empty file.

Patch 3 fixes the problem for gdbserver, and patch 4 fixes it for GDB.

Patches 1 and 2 implement a couple of suggestions made by Pedro in the
same thread.

Regression tested on native and remote aarch64-linux and x86_64-linux.

Thiago Jung Bauermann (5):
  gdbserver: Use current_process in handle_qxfer_auxv
  gdbserver: Use the PID of the current process instead of the current
    thread
  gdbserver/linux: Read auxv from any thread of the process
  gdb/linux-nat: Read auxv from any thread of the process
  gdb/testsuite: Add test that reads auxv in a multi-threaded inferior

 gdb/auxv.c                         | 16 +++++++
 gdb/linux-nat.c                    | 40 +++++++++++++++++-
 gdb/nat/linux-procfs.c             | 67 ++++++++++++++++++++++++++++++
 gdb/nat/linux-procfs.h             |  7 ++++
 gdb/testsuite/gdb.base/auxv.exp    | 56 -------------------------
 gdb/testsuite/gdb.threads/auxv.c   | 62 +++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/auxv.exp | 30 +++++++++++++
 gdb/testsuite/lib/gdb.exp          | 62 +++++++++++++++++++++++++++
 gdbserver/linux-aarch64-low.cc     | 10 ++---
 gdbserver/linux-arm-low.cc         |  6 +--
 gdbserver/linux-low.cc             | 21 ++--------
 gdbserver/linux-ppc-low.cc         |  6 +--
 gdbserver/mem-break.cc             |  2 +-
 gdbserver/regcache.cc              |  2 +-
 gdbserver/server.cc                |  7 ++--
 gdbserver/tracepoint.cc            |  2 +-
 gdbserver/win32-i386-low.cc        |  4 +-
 17 files changed, 305 insertions(+), 95 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/auxv.c
 create mode 100644 gdb/testsuite/gdb.threads/auxv.exp


base-commit: 66f76c545b293f8b89fef0f996a3a48fa59fae61

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/5] gdbserver: Use current_process in handle_qxfer_auxv
  2023-03-31  3:44 [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Thiago Jung Bauermann
@ 2023-03-31  3:44 ` Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 2/5] gdbserver: Use the PID of the current process instead of the current thread Thiago Jung Bauermann
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Thiago Jung Bauermann @ 2023-03-31  3:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann, Pedro Alves

Commit 43e5fbd8b788 ("gdbserver: Add PID parameter to linux_get_auxv and
linux_get_hwcap") changed the functions for reading auxv from using
current_thread to accepting a PID argument.  Pedro then suggested: "Since
we're removing the thread dependency, I think that check should be replaced
by current_process() == NULL instead".

This patch implements his suggestion.

Suggested-by: Pedro Alves <pedro@palves.net>
---
 gdbserver/server.cc | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 949849b63a2d..b6786baafadd 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1445,11 +1445,10 @@ handle_qxfer_auxv (const char *annex,
   if (!the_target->supports_read_auxv () || writebuf != NULL)
     return -2;
 
-  if (annex[0] != '\0' || current_thread == NULL)
+  if (annex[0] != '\0' || current_process () == nullptr)
     return -1;
 
-  return the_target->read_auxv (current_thread->id.pid (), offset, readbuf,
-				len);
+  return the_target->read_auxv (current_process ()->pid, offset, readbuf, len);
 }
 
 /* Handle qXfer:exec-file:read.  */

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/5] gdbserver: Use the PID of the current process instead of the current thread
  2023-03-31  3:44 [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 1/5] gdbserver: Use current_process in handle_qxfer_auxv Thiago Jung Bauermann
@ 2023-03-31  3:44 ` Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 3/5] gdbserver/linux: Read auxv from any thread of the process Thiago Jung Bauermann
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Thiago Jung Bauermann @ 2023-03-31  3:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann, Pedro Alves

Commit 43e5fbd8b788 ("gdbserver: Add PID parameter to linux_get_auxv and
linux_get_hwcap") used current_thread->id.pid () to get the current
process' PID.  Pedro then mentioned that using current_process ()->pid is
more to the point.  He added that "sometimes there's a current process, but
not a current thread, so when we don't actually need a thread,
current_process() is better".

So this patch goes through the places that use current_thread to get the
current process' PID and changes them to use current_process () instead.

Suggested-by: Pedro Alves <pedro@palves.net>
---
 gdbserver/linux-aarch64-low.cc | 10 +++++-----
 gdbserver/linux-arm-low.cc     |  6 +++---
 gdbserver/linux-ppc-low.cc     |  6 +++---
 gdbserver/mem-break.cc         |  2 +-
 gdbserver/regcache.cc          |  2 +-
 gdbserver/server.cc            |  2 +-
 gdbserver/tracepoint.cc        |  2 +-
 gdbserver/win32-i386-low.cc    |  4 ++--
 8 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
index 2ed6e95562c5..0c524c036d89 100644
--- a/gdbserver/linux-aarch64-low.cc
+++ b/gdbserver/linux-aarch64-low.cc
@@ -436,7 +436,7 @@ aarch64_target::low_insert_point (raw_bkpt_type type, CORE_ADDR addr,
   int ret;
   enum target_hw_bp_type targ_type;
   struct aarch64_debug_reg_state *state
-    = aarch64_get_debug_reg_state (pid_of (current_thread));
+    = aarch64_get_debug_reg_state (current_process ()->pid);
 
   if (show_debug_regs)
     fprintf (stderr, "insert_point on entry (addr=0x%08lx, len=%d)\n",
@@ -487,7 +487,7 @@ aarch64_target::low_remove_point (raw_bkpt_type type, CORE_ADDR addr,
   int ret;
   enum target_hw_bp_type targ_type;
   struct aarch64_debug_reg_state *state
-    = aarch64_get_debug_reg_state (pid_of (current_thread));
+    = aarch64_get_debug_reg_state (current_process ()->pid);
 
   if (show_debug_regs)
     fprintf (stderr, "remove_point on entry (addr=0x%08lx, len=%d)\n",
@@ -575,7 +575,7 @@ aarch64_target::low_stopped_data_address ()
     = aarch64_remove_non_address_bits ((CORE_ADDR) siginfo.si_addr);
 
   /* Check if the address matches any watched address.  */
-  state = aarch64_get_debug_reg_state (pid_of (current_thread));
+  state = aarch64_get_debug_reg_state (current_process ()->pid);
   for (i = aarch64_num_wp_regs - 1; i >= 0; --i)
     {
       const unsigned int offset
@@ -846,7 +846,7 @@ aarch64_target::low_arch_setup ()
   if (is_elf64)
     {
       struct aarch64_features features;
-      int pid = current_thread->id.pid ();
+      int pid = current_process ()->pid;
 
       features.vq = aarch64_sve_get_vq (tid);
       /* A-profile PAC is 64-bit only.  */
@@ -3323,7 +3323,7 @@ aarch64_target::supports_memory_tagging ()
 #endif
     }
 
-  return (linux_get_hwcap2 (current_thread->id.pid (), 8) & HWCAP2_MTE) != 0;
+  return (linux_get_hwcap2 (current_process ()->pid, 8) & HWCAP2_MTE) != 0;
 }
 
 bool
diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
index 5975b44af0ae..76d15baa8512 100644
--- a/gdbserver/linux-arm-low.cc
+++ b/gdbserver/linux-arm-low.cc
@@ -636,7 +636,7 @@ arm_target::low_insert_point (raw_bkpt_type type, CORE_ADDR addr,
 	pts[i] = p;
 
 	/* Only update the threads of the current process.  */
-	for_each_thread (current_thread->id.pid (), [&] (thread_info *thread)
+	for_each_thread (current_process ()->pid, [&] (thread_info *thread)
 	  {
 	    update_registers_callback (thread, watch, i);
 	  });
@@ -681,7 +681,7 @@ arm_target::low_remove_point (raw_bkpt_type type, CORE_ADDR addr,
 	pts[i].control = arm_hwbp_control_disable (pts[i].control);
 
 	/* Only update the threads of the current process.  */
-	for_each_thread (current_thread->id.pid (), [&] (thread_info *thread)
+	for_each_thread (current_process ()->pid, [&] (thread_info *thread)
 	  {
 	    update_registers_callback (thread, watch, i);
 	  });
@@ -958,7 +958,7 @@ get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self)
 static const struct target_desc *
 arm_read_description (void)
 {
-  unsigned long arm_hwcap = linux_get_hwcap (current_thread->id.pid (), 4);
+  unsigned long arm_hwcap = linux_get_hwcap (current_process ()->pid, 4);
 
   if (arm_hwcap & HWCAP_IWMMXT)
     return arm_linux_read_description (ARM_FP_TYPE_IWMMXT);
diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
index f8dd770b8eb3..fe44ba47992b 100644
--- a/gdbserver/linux-ppc-low.cc
+++ b/gdbserver/linux-ppc-low.cc
@@ -894,8 +894,8 @@ ppc_target::low_arch_setup ()
 
   /* The value of current_process ()->tdesc needs to be set for this
      call.  */
-  ppc_hwcap = linux_get_hwcap (current_thread->id.pid (), features.wordsize);
-  ppc_hwcap2 = linux_get_hwcap2 (current_thread->id.pid (), features.wordsize);
+  ppc_hwcap = linux_get_hwcap (current_process ()->pid, features.wordsize);
+  ppc_hwcap2 = linux_get_hwcap2 (current_process ()->pid, features.wordsize);
 
   features.isa205 = ppc_linux_has_isa205 (ppc_hwcap);
 
@@ -1097,7 +1097,7 @@ is_elfv2_inferior (void)
   const struct target_desc *tdesc = current_process ()->tdesc;
   int wordsize = register_size (tdesc, 0);
 
-  if (!linux_get_auxv (current_thread->id.pid (), wordsize, AT_PHDR, &phdr))
+  if (!linux_get_auxv (current_process ()->pid, wordsize, AT_PHDR, &phdr))
     return def_res;
 
   /* Assume ELF header is at the beginning of the page where program headers
diff --git a/gdbserver/mem-break.cc b/gdbserver/mem-break.cc
index c669842228d0..4cd7a3de093f 100644
--- a/gdbserver/mem-break.cc
+++ b/gdbserver/mem-break.cc
@@ -1392,7 +1392,7 @@ set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid)
 {
   struct single_step_breakpoint *bp;
 
-  gdb_assert (current_ptid.pid () == ptid.pid ());
+  gdb_assert (current_process ()->pid == ptid.pid ());
 
   bp = (struct single_step_breakpoint *) set_breakpoint_type_at (single_step_breakpoint,
 								stop_at, NULL);
diff --git a/gdbserver/regcache.cc b/gdbserver/regcache.cc
index 0b1141662ac6..15dda96965f1 100644
--- a/gdbserver/regcache.cc
+++ b/gdbserver/regcache.cc
@@ -106,7 +106,7 @@ void
 regcache_invalidate (void)
 {
   /* Only update the threads of the current process.  */
-  int pid = current_thread->id.pid ();
+  int pid = current_process ()->pid;
 
   regcache_invalidate_pid (pid);
 }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index b6786baafadd..b6c23f280f86 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -1469,7 +1469,7 @@ handle_qxfer_exec_file (const char *annex,
       if (current_thread == NULL)
 	return -1;
 
-      pid = pid_of (current_thread);
+      pid = current_process ()->pid;
     }
   else
     {
diff --git a/gdbserver/tracepoint.cc b/gdbserver/tracepoint.cc
index 3f60989e4c7f..590210260bf5 100644
--- a/gdbserver/tracepoint.cc
+++ b/gdbserver/tracepoint.cc
@@ -6813,7 +6813,7 @@ static int
 run_inferior_command (char *cmd, int len)
 {
   int err = -1;
-  int pid = current_ptid.pid ();
+  int pid = current_process ()->pid;
 
   trace_debug ("run_inferior_command: running: %s", cmd);
 
diff --git a/gdbserver/win32-i386-low.cc b/gdbserver/win32-i386-low.cc
index f78e01206785..af96b464681b 100644
--- a/gdbserver/win32-i386-low.cc
+++ b/gdbserver/win32-i386-low.cc
@@ -57,7 +57,7 @@ x86_dr_low_set_addr (int regnum, CORE_ADDR addr)
   gdb_assert (DR_FIRSTADDR <= regnum && regnum <= DR_LASTADDR);
 
   /* Only update the threads of this process.  */
-  for_each_thread (current_thread->id.pid (), update_debug_registers);
+  for_each_thread (current_process ()->pid, update_debug_registers);
 }
 
 /* Update the inferior's DR7 debug control register from STATE.  */
@@ -66,7 +66,7 @@ static void
 x86_dr_low_set_control (unsigned long control)
 {
   /* Only update the threads of this process.  */
-  for_each_thread (current_thread->id.pid (), update_debug_registers);
+  for_each_thread (current_process ()->pid, update_debug_registers);
 }
 
 /* Return the current value of a DR register of the current thread's

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/5] gdbserver/linux: Read auxv from any thread of the process
  2023-03-31  3:44 [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 1/5] gdbserver: Use current_process in handle_qxfer_auxv Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 2/5] gdbserver: Use the PID of the current process instead of the current thread Thiago Jung Bauermann
@ 2023-03-31  3:44 ` Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 4/5] gdb/linux-nat: " Thiago Jung Bauermann
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Thiago Jung Bauermann @ 2023-03-31  3:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann, Pedro Alves

If the initial thread of the process exits, reading the process' auxiliary
vector via /proc/PID/auxv fails in one of two ways:

1. If gdbserver is root, then opening the file succeeds but reading from it
   returns 0 bytes.

2. If gdbserver isn't root, then opening the file fails with EACCES.

This race isn't easy to run into because one of the first things that GDB
does when connecting to gdbserver is to read the inferior's auxiliary
vector and store it in the auxv cache.  All further queries of the auxiliary
vector will be served from there, unless one of the cache-clearing
events ("inferior_exit", "inferior_appeared", "executable_changed") occurs.

To fix the race condition iterate through tasks in /proc/PID/task/ and
try to read their auxv file, returning the first one that succeeds.

Suggested-by: Pedro Alves <pedro@palves.net>
---
 gdb/nat/linux-procfs.c | 67 ++++++++++++++++++++++++++++++++++++++++++
 gdb/nat/linux-procfs.h |  7 +++++
 gdbserver/linux-low.cc | 21 +++----------
 3 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c
index 9c2d1beb91fa..d53d3ea98bed 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -329,6 +329,73 @@ linux_proc_attach_tgid_threads (pid_t pid,
 
 /* See linux-procfs.h.  */
 
+bool
+linux_proc_read_auxv (pid_t pid, gdb_byte *readbuf, off_t offset, size_t len,
+		      ssize_t &xfered_len)
+{
+  if (linux_proc_get_tgid (pid) != pid)
+    return false;
+
+  std::string path_tasks = string_printf ("/proc/%ld/task", (long) pid);
+  gdb_dir_up dir (opendir (path_tasks.c_str ()));
+  if (dir == nullptr)
+    {
+      warning (_("Could not open /proc/%ld/task."), (long) pid);
+      return false;
+    }
+
+  /* In a multi-threaded process, any given thread (including the initial one)
+     can exit at any time.  Because of this, iterate through the threads trying
+     to read their auxv file and return the first one that succeeds.  */
+  struct dirent *dp;
+  while ((dp = readdir (dir.get ())) != nullptr)
+    {
+      /* Fetch one lwp.  */
+      unsigned long lwp = strtoul (dp->d_name, nullptr, 10);
+      if (lwp == 0)
+	continue;
+
+      std::string path_auxv = string_printf ("/proc/%lu/auxv", lwp);
+      scoped_fd fd = gdb_open_cloexec (path_auxv, O_RDONLY, 0);
+      if (fd.get () < 0)
+	{
+	  /* If the leader thread of the process exited and we aren't root, then
+	     trying to read its auxv file results in EACCES error.  */
+	  if (errno == EACCES)
+	    continue;
+
+	  /* Unexpected failure.  Give up.  */
+	  return false;
+	}
+
+      ssize_t l;
+      if (offset != 0 && lseek (fd.get (), offset, SEEK_SET) != offset)
+	l = -1;
+      else
+	l = read (fd.get (), readbuf, len);
+
+      /* Unexpected failure.  Give up.  */
+      if (l < 0)
+	return false;
+      /* The read call returns 0 if the leader thread of the process exited and
+	 we are root, or if any thread exited after we opened its auxv file but
+	 before we had a chance to read it.  This only applies during the first
+	 read into the file though (i.e., offset == 0).  */
+      else if (l != 0 || offset != 0)
+	{
+	  xfered_len = l;
+	  return true;
+	}
+    }
+
+  /* If we get here, either all open call failed with EACCES (meaning the
+     threads are gone), or all read calls returned 0 (meaning either EOF or that
+     the threads are gone).  */
+  return true;
+}
+
+/* See linux-procfs.h.  */
+
 int
 linux_proc_task_list_dir_exists (pid_t pid)
 {
diff --git a/gdb/nat/linux-procfs.h b/gdb/nat/linux-procfs.h
index 639d8efaf0bf..bcdcedcdc2a9 100644
--- a/gdb/nat/linux-procfs.h
+++ b/gdb/nat/linux-procfs.h
@@ -80,6 +80,13 @@ extern int linux_proc_task_list_dir_exists (pid_t pid);
 
 extern const char *linux_proc_pid_to_exec_file (int pid);
 
+/* Read the auxiliary vector for the PID process into READBUF which has LEN bytes,
+   starting at OFFSET.  Returns true if successful, with XFERED_LEN indicating how many
+   bytes were read, and false on error.  */
+
+extern bool linux_proc_read_auxv (pid_t pid, gdb_byte *readbuf, off_t offset,
+				  size_t len, ssize_t &xfered_len);
+
 /* Display possible problems on this system.  Display them only once
    per GDB execution.  */
 
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index e6a39202a98a..d42eb3a49ad7 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -5486,24 +5486,11 @@ int
 linux_process_target::read_auxv (int pid, CORE_ADDR offset,
 				 unsigned char *myaddr, unsigned int len)
 {
-  char filename[PATH_MAX];
-  int fd, n;
-
-  xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
-
-  fd = open (filename, O_RDONLY);
-  if (fd < 0)
-    return -1;
-
-  if (offset != (CORE_ADDR) 0
-      && lseek (fd, (off_t) offset, SEEK_SET) != (off_t) offset)
-    n = -1;
-  else
-    n = read (fd, myaddr, len);
-
-  close (fd);
+  ssize_t xfered_len;
+  bool rc = linux_proc_read_auxv ((pid_t) pid, (gdb_byte *) myaddr,
+				  (off_t) offset, (size_t) len, xfered_len);
 
-  return n;
+  return rc ? (int) xfered_len : -1;
 }
 
 int

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 4/5] gdb/linux-nat: Read auxv from any thread of the process
  2023-03-31  3:44 [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Thiago Jung Bauermann
                   ` (2 preceding siblings ...)
  2023-03-31  3:44 ` [PATCH 3/5] gdbserver/linux: Read auxv from any thread of the process Thiago Jung Bauermann
@ 2023-03-31  3:44 ` Thiago Jung Bauermann
  2023-03-31  3:44 ` [PATCH 5/5] gdb/testsuite: Add test that reads auxv in a multi-threaded inferior Thiago Jung Bauermann
  2023-04-04 15:19 ` [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Simon Marchi
  5 siblings, 0 replies; 8+ messages in thread
From: Thiago Jung Bauermann @ 2023-03-31  3:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann, Pedro Alves

If the initial thread of the process exits, reading the process' auxiliary
vector via /proc/PID/auxv fails in one of two ways:

1. If GDB is root, then opening the file succeeds but reading from it
   returns 0 bytes.

2. If gdbserver isn't root, then opening the file fails with EACCES.

This race isn't easy to run into because one of the first things that GDB
does when starting an inferior is to read its auxiliary vector and store it
in the auxv cache.  All further queries of the auxiliary vector will be
served from there, unless one of the cache-clearing events
("inferior_exit", "inferior_appeared", "executable_changed") occurs.

To fix the race, use linux_proc_read_auxv instead of the generic procfs
implementation.

Suggested-by: Pedro Alves <pedro@palves.net>
---
 gdb/linux-nat.c | 40 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index fd80fd975c14..938ae1c9b8c6 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3667,6 +3667,33 @@ linux_xfer_siginfo (ptid_t ptid, enum target_object object,
   return TARGET_XFER_OK;
 }
 
+/* Implement the to_xfer_partial target_ops method for TARGET_OBJECT_AUXV.  This
+   function handles access via /proc/LWP/auxv, which allows handling possible
+   races in multi-threaded inferiors.  */
+
+static enum target_xfer_status
+linux_nat_xfer_auxv (gdb_byte *readbuf, const gdb_byte *writebuf,
+		     ULONGEST offset, ULONGEST len, ULONGEST *xfered_len)
+{
+  /* Linux doesn't support writing to the auxv file.  */
+  if (readbuf == nullptr || writebuf != nullptr)
+    return TARGET_XFER_E_IO;
+
+  ssize_t xfered;
+  bool rc = linux_proc_read_auxv (inferior_ptid.pid (), readbuf, (off_t) offset,
+				  (size_t) len, xfered);
+
+  if (!rc)
+    return TARGET_XFER_E_IO;
+  else if (xfered == 0)
+    return TARGET_XFER_EOF;
+  else
+    {
+      *xfered_len = (ULONGEST) xfered;
+      return TARGET_XFER_OK;
+    }
+}
+
 static enum target_xfer_status
 linux_nat_xfer_osdata (enum target_object object,
 		       const char *annex, gdb_byte *readbuf,
@@ -3695,8 +3722,17 @@ linux_nat_target::xfer_partial (enum target_object object,
     return TARGET_XFER_EOF;
 
   if (object == TARGET_OBJECT_AUXV)
-    return memory_xfer_auxv (this, object, annex, readbuf, writebuf,
-			     offset, len, xfered_len);
+    {
+      /* For attached inferiors, use memory_xfer_auxv's ld.so support which
+	 works with virtual executables being executed by Valgrind's
+	 memcheck.  */
+      if (current_inferior ()->attach_flag)
+	return memory_xfer_auxv (this, object, annex, readbuf, writebuf,
+				 offset, len, xfered_len);
+      else
+	return linux_nat_xfer_auxv (readbuf, writebuf, offset, len,
+				    xfered_len);
+    }
 
   if (object == TARGET_OBJECT_OSDATA)
     return linux_nat_xfer_osdata (object, annex, readbuf, writebuf,

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 5/5] gdb/testsuite: Add test that reads auxv in a multi-threaded inferior
  2023-03-31  3:44 [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Thiago Jung Bauermann
                   ` (3 preceding siblings ...)
  2023-03-31  3:44 ` [PATCH 4/5] gdb/linux-nat: " Thiago Jung Bauermann
@ 2023-03-31  3:44 ` Thiago Jung Bauermann
  2023-04-04 14:00   ` Alexandra Petlanova Hajkova
  2023-04-04 15:19 ` [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Simon Marchi
  5 siblings, 1 reply; 8+ messages in thread
From: Thiago Jung Bauermann @ 2023-03-31  3:44 UTC (permalink / raw)
  To: gdb-patches; +Cc: Thiago Jung Bauermann

This test exercises reading the auxiliary vector in a program where the
main thread exits before other threads.

Because GDB's auxv cache makes it difficult to hit the race that this test
exercises, add a maintenance command that flushes the cache.

Also, move the fetch_auxv procedure from gdb.base/auxv to lib/gdb.exp so
that the new testcase can use it.
---
 gdb/auxv.c                         | 16 ++++++++
 gdb/testsuite/gdb.base/auxv.exp    | 56 ---------------------------
 gdb/testsuite/gdb.threads/auxv.c   | 62 ++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/auxv.exp | 30 +++++++++++++++
 gdb/testsuite/lib/gdb.exp          | 62 ++++++++++++++++++++++++++++++
 5 files changed, 170 insertions(+), 56 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/auxv.c
 create mode 100644 gdb/testsuite/gdb.threads/auxv.exp

diff --git a/gdb/auxv.c b/gdb/auxv.c
index 812b28075548..a39d8afd2990 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -27,6 +27,7 @@
 #include "observable.h"
 #include "gdbsupport/filestuff.h"
 #include "objfiles.h"
+#include "cli/cli-cmds.h"
 
 #include "auxv.h"
 #include "elf/common.h"
@@ -602,6 +603,17 @@ info_auxv_command (const char *cmd, int from_tty)
     }
 }
 
+/* Implement 'maint flush auxv' command.  */
+
+static void
+auxv_flush_command (const char *command, int from_tty)
+{
+  /* Force-flush the auxv cache.  */
+  invalidate_auxv_cache();
+  if (from_tty)
+    gdb_printf (_ ("Auxiliary vector cache flushed.\n"));
+}
+
 void _initialize_auxv ();
 void
 _initialize_auxv ()
@@ -610,6 +622,10 @@ _initialize_auxv ()
 	    _("Display the inferior's auxiliary vector.\n\
 This is information provided by the operating system at program startup."));
 
+  add_cmd ("auxv-cache", class_maintenance, auxv_flush_command,
+	   _ ("Force gdb to flush its auxiliary vector cache."),
+	   &maintenanceflushlist);
+
   /* Observers used to invalidate the auxv cache when needed.  */
   gdb::observers::inferior_exit.attach (invalidate_auxv_cache_inf, "auxv");
   gdb::observers::inferior_appeared.attach (invalidate_auxv_cache_inf, "auxv");
diff --git a/gdb/testsuite/gdb.base/auxv.exp b/gdb/testsuite/gdb.base/auxv.exp
index 89242b6f4321..7f58bd318fbc 100644
--- a/gdb/testsuite/gdb.base/auxv.exp
+++ b/gdb/testsuite/gdb.base/auxv.exp
@@ -59,62 +59,6 @@ set print_core_line [gdb_get_line_number "ABORT;"]
 gdb_test "tbreak $print_core_line"
 gdb_test continue ".*ABORT;.*"
 
-proc fetch_auxv {test} {
-    global gdb_prompt
-
-    set auxv_lines {}
-    set bad -1
-    # Former trailing `\[\r\n\]+' may eat just \r leaving \n in the buffer
-    # corrupting the next matches.
-    if {[gdb_test_multiple "info auxv" $test {
-	-re "info auxv\r\n" {
-	    exp_continue
-	}
-	-ex "The program has no auxiliary information now" {
-	    set bad 1
-	    exp_continue
-	}
-	-ex "Auxiliary vector is empty" {
-	    set bad 1
-	    exp_continue
-	}
-	-ex "No auxiliary vector found" {
-	    set bad 1
-	    exp_continue
-	}
-	-re "^\[0-9\]+\[ \t\]+(AT_\[^ \t\]+)\[^\r\n\]+\r\n" {
-	    lappend auxv_lines $expect_out(0,string)
-	    exp_continue
-	}
-	-re "^\[0-9\]+\[ \t\]+\\?\\?\\?\[^\r\n\]+\r\n" {
-	    warning "Unrecognized tag value: $expect_out(0,string)"
-	    set bad 1
-	    lappend auxv_lines $expect_out(0,string)
-	    exp_continue
-	}
-	-re "$gdb_prompt $" {
-	    incr bad
-	}
-	-re "^\[^\r\n\]+\r\n" {
-	    if {!$bad} {
-		warning "Unrecognized output: $expect_out(0,string)"
-		set bad 1
-	    }
-	    exp_continue
-	}
-    }] != 0} {
-	return {}
-    }
-
-    if {$bad} {
-	fail $test
-	return {}
-    }
-
-    pass $test
-    return $auxv_lines
-}
-
 set live_data [fetch_auxv "info auxv on live process"]
 
 # Now try gcore.
diff --git a/gdb/testsuite/gdb.threads/auxv.c b/gdb/testsuite/gdb.threads/auxv.c
new file mode 100644
index 000000000000..5ad280145c89
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/auxv.c
@@ -0,0 +1,62 @@
+/* Copyright 2023 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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 program creates an additional thread and then exits the leader thread.
+   It uses semaphores so that GDB can stop the program at specific points and
+   test how it deals with accessing the auxv information of a multi-threaded
+   program in different moments of the thread life cycle.  */
+
+#include <pthread.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+
+static volatile pthread_t main_thread;
+
+static void *
+subthread (void *arg)
+{
+    int rc;
+
+    rc = pthread_join (main_thread, NULL);
+    if (rc != 0)
+      fprintf (stderr, "Warning: pthread_join failed: %s\n", strerror (rc));
+
+    return NULL; /* Main thread exited.  */
+}
+
+int
+main (int argc, char *argv[])
+{
+  int rc;
+  pthread_t thread_id;
+
+  main_thread = pthread_self ();
+
+  rc = pthread_create (&thread_id, NULL, &subthread, NULL);
+  if (rc != 0)
+    {
+      fprintf (stderr, "Error: pthread_create failed: %s\n", strerror (rc));
+      return EXIT_FAILURE;
+    }
+
+  pthread_exit (NULL);
+  /* NOTREACHED */
+  return EXIT_FAILURE;
+}
diff --git a/gdb/testsuite/gdb.threads/auxv.exp b/gdb/testsuite/gdb.threads/auxv.exp
new file mode 100644
index 000000000000..1ad3564642bd
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/auxv.exp
@@ -0,0 +1,30 @@
+# Test 'info auxv' and related functionality with a multi-threaded inferior.
+
+# Copyright (C) 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/>.
+
+standard_testfile
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  {debug pthreads}] } {
+    return
+}
+
+if ![runto ${srcfile}:[gdb_get_line_number "Main thread exited."]] {
+    return
+}
+
+gdb_test -nopass "maintenance flush auxv-cache" "Auxiliary vector cache flushed."
+
+fetch_auxv "Get auxv after main thread exits"
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 14ce39e8ed72..8dd79ba5008a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -7821,6 +7821,68 @@ proc get_var_address { var } {
     return ""
 }
 
+# Retrieve the auxiliary vector from the inferior.
+#
+# Issues a pass/fail using TEST as the message, reflecting whether the "info auxv"
+# command worked.
+#
+# Returns the command's output.
+proc fetch_auxv {test} {
+    global gdb_prompt
+
+    set auxv_lines {}
+    set bad -1
+    # Former trailing `\[\r\n\]+' may eat just \r leaving \n in the buffer
+    # corrupting the next matches.
+    if {[gdb_test_multiple "info auxv" $test {
+	-re "info auxv\r\n" {
+	    exp_continue
+	}
+	-ex "The program has no auxiliary information now" {
+	    set bad 1
+	    exp_continue
+	}
+	-ex "Auxiliary vector is empty" {
+	    set bad 1
+	    exp_continue
+	}
+	-ex "No auxiliary vector found" {
+	    set bad 1
+	    exp_continue
+	}
+	-re "^\[0-9\]+\[ \t\]+(AT_\[^ \t\]+)\[^\r\n\]+\r\n" {
+	    lappend auxv_lines $expect_out(0,string)
+	    exp_continue
+	}
+	-re "^\[0-9\]+\[ \t\]+\\?\\?\\?\[^\r\n\]+\r\n" {
+	    warning "Unrecognized tag value: $expect_out(0,string)"
+	    set bad 1
+	    lappend auxv_lines $expect_out(0,string)
+	    exp_continue
+	}
+	-re "$gdb_prompt $" {
+	    incr bad
+	}
+	-re "^\[^\r\n\]+\r\n" {
+	    if {!$bad} {
+		warning "Unrecognized output: $expect_out(0,string)"
+		set bad 1
+	    }
+	    exp_continue
+	}
+    }] != 0} {
+	return {}
+    }
+
+    if {$bad} {
+	fail $test
+	return {}
+    }
+
+    pass $test
+    return $auxv_lines
+}
+
 # Return the frame number for the currently selected frame
 proc get_current_frame_number {{test_name ""}} {
     global gdb_prompt

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 5/5] gdb/testsuite: Add test that reads auxv in a multi-threaded inferior
  2023-03-31  3:44 ` [PATCH 5/5] gdb/testsuite: Add test that reads auxv in a multi-threaded inferior Thiago Jung Bauermann
@ 2023-04-04 14:00   ` Alexandra Petlanova Hajkova
  0 siblings, 0 replies; 8+ messages in thread
From: Alexandra Petlanova Hajkova @ 2023-04-04 14:00 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

[-- Attachment #1: Type: text/plain, Size: 1820 bytes --]

On Fri, Mar 31, 2023 at 5:46 AM Thiago Jung Bauermann via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> This test exercises reading the auxiliary vector in a program where the
> main thread exits before other threads.
>
> Because GDB's auxv cache makes it difficult to hit the race that this test
> exercises, add a maintenance command that flushes the cache.
>
> Also, move the fetch_auxv procedure from gdb.base/auxv to lib/gdb.exp so
> that the new testcase can use it.
> ---
>  gdb/auxv.c                         | 16 ++++++++
>  gdb/testsuite/gdb.base/auxv.exp    | 56 ---------------------------
>  gdb/testsuite/gdb.threads/auxv.c   | 62 ++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.threads/auxv.exp | 30 +++++++++++++++
>  gdb/testsuite/lib/gdb.exp          | 62 ++++++++++++++++++++++++++++++
>  5 files changed, 170 insertions(+), 56 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.threads/auxv.c
>  create mode 100644 gdb/testsuite/gdb.threads/auxv.exp
>
>
I tested the series on ppc64le and I've got a failure for
gdb.threads/auxv.exp:

Running target unix
Using /usr/share/dejagnu/baseboards/unix.exp as board description file for
target.
Using /usr/share/dejagnu/config/unix.exp as generic interface file for
target.
Using
/root/build_upstream/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/config/unix.exp
as tool-and-target-specific interface file.
Running
/root/build_upstream/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.threads/auxv.exp
...
WARNING: Unrecognized tag value: 27   ???
              0x1c

WARNING: Unrecognized tag value: 28   ???
              0x20

FAIL: gdb.threads/auxv.exp: Get auxv after main thread exits

=== gdb Summary ===

# of expected passes 1
# of unexpected failures 1

Regards,
Alexandra

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter
  2023-03-31  3:44 [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Thiago Jung Bauermann
                   ` (4 preceding siblings ...)
  2023-03-31  3:44 ` [PATCH 5/5] gdb/testsuite: Add test that reads auxv in a multi-threaded inferior Thiago Jung Bauermann
@ 2023-04-04 15:19 ` Simon Marchi
  5 siblings, 0 replies; 8+ messages in thread
From: Simon Marchi @ 2023-04-04 15:19 UTC (permalink / raw)
  To: Thiago Jung Bauermann, gdb-patches

On 3/30/23 23:44, Thiago Jung Bauermann via Gdb-patches wrote:
> Hello,
> 
> Back in February, I pushed commit 43e5fbd8b788 ("gdbserver: Add PID
> parameter to linux_get_auxv and linux_get_hwcap"), and mentioned that
> there was no change in gdbserver behaviour. Pedro corrected me,
> mentioning that before the patch gdbserver would read the auxv from
> /proc/<current_thread's LWP>/auxv, while after the patch it would read
> it from /proc/<current_thread's PID>/auxv. This causes trouble in case
> the inferior's main thread exits.
> 
> I created a testcase exercising this scenario (the last patch in this
> series) and confirmed that both GDB and gdbserver have this problem (GDB
> wasn't changed by my patch). The problem is largely mitigated by GDB's
> auxv cache though, and is very hard to hit because one of the first
> things that GDB does when starting/attaching to an inferior is read the
> auxv and cache it. It will only be a problem if the cache is invalidated
> by one of the cache-clearing events ("inferior_exit",
> "inferior_appeared", "executable_changed").

If it helps, you can add maintenance commands to force clearing and
fetching the auxv.

> 
> In the discussion about my patch there were also some questions about
> other race conditions in this area. I created a test program to
> experiment and these are the results:
> 
> Q1: What happens exactly if GDB/gdbserver tries to read
>     /proc/<thread's PID>/auxv after the main thread exits?
> A1: If GDB/gdbserver is root, then open() will succeed but read() will
>     return 0, indicating an empty file. If GDB/gdbserver isn't root,
>     then open() will fail with errno = EACCES ("Permission denied") even
>     if it's running as the same user as the inferior.

Crazy!

> Q2: What happens if an inferior thread exits after GDB/gdbserver opens its
>     /proc/<thread's LWP>/auxv file but before it has a chance to read it?
> A2: The read() call will return 0, indicating an empty file.

Even if the thread is ptraced (which is the case, we are debugging it)
and we haven't consumed the thread exit event from the kernel?

I suppose an even more corner case is: a first partial read succeeds,
the thread exits, the following read returns 0.  Is it even possible to
detect this happened?

Simon

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-04-04 15:19 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-31  3:44 [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Thiago Jung Bauermann
2023-03-31  3:44 ` [PATCH 1/5] gdbserver: Use current_process in handle_qxfer_auxv Thiago Jung Bauermann
2023-03-31  3:44 ` [PATCH 2/5] gdbserver: Use the PID of the current process instead of the current thread Thiago Jung Bauermann
2023-03-31  3:44 ` [PATCH 3/5] gdbserver/linux: Read auxv from any thread of the process Thiago Jung Bauermann
2023-03-31  3:44 ` [PATCH 4/5] gdb/linux-nat: " Thiago Jung Bauermann
2023-03-31  3:44 ` [PATCH 5/5] gdb/testsuite: Add test that reads auxv in a multi-threaded inferior Thiago Jung Bauermann
2023-04-04 14:00   ` Alexandra Petlanova Hajkova
2023-04-04 15:19 ` [PATCH 0/5] gdbserver: Follow-up on linux_get_auxv using PID parameter Simon Marchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).