Re: [PATCH v2 2/6] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap

[Luis Machado <luis.machado@arm.com>]

Hi,

On 11/26/22 02:04, Thiago Jung Bauermann wrote:
> This patch doesn't change gdbserver behaviour, but after later changes are
> made it avoids a null pointer dereference when HWCAP needs to be obtained
> for a specific process while current_thread is nullptr.
> 
> Fixing linux_read_auxv, linux_get_hwcap and linux_get_hwcap2 to take a PID
> parameter seems more correct than setting current_thread in one particular
> code path.

I'm wondering if passing the pid is needed at all in gdbserver for the purposes of fetching the hwcap entries from auxv.

While current thread can be nullptr, I suppose current_process will never be nullptr if we have a valid inferior. And the auxv entries are per-process rather than per-thread.

There is a bit of a corner case when we have extended-remote without a live process and we need to fetch the hwcap bits to determine a particular feature to report via qSupported. But I suppose this is not what this change is trying to address, is it?

If not, it may be the case that we don't need these changes, and we can just use current_process.

Does that make sense?

> 
> Changes are propagated to allow passing the new parameter through the call
> chain.
> ---
>   gdbserver/linux-aarch64-low.cc |  7 ++++---
>   gdbserver/linux-arm-low.cc     |  2 +-
>   gdbserver/linux-low.cc         | 18 +++++++++---------
>   gdbserver/linux-low.h          |  9 ++++-----
>   gdbserver/linux-ppc-low.cc     |  6 +++---
>   gdbserver/linux-s390-low.cc    |  2 +-
>   gdbserver/netbsd-low.cc        |  4 +---
>   gdbserver/netbsd-low.h         |  2 +-
>   gdbserver/server.cc            |  2 +-
>   gdbserver/target.cc            |  4 ++--
>   gdbserver/target.h             |  2 +-
>   11 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index db5086962612..a6ed68f93029 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -823,12 +823,13 @@ aarch64_target::low_arch_setup ()
>     if (is_elf64)
>       {
>         struct aarch64_features features;
> +      int pid = pid_of (current_thread);
>   
>         features.vq = aarch64_sve_get_vq (tid);
>         /* A-profile PAC is 64-bit only.  */
> -      features.pauth = linux_get_hwcap (8) & AARCH64_HWCAP_PACA;
> +      features.pauth = (linux_get_hwcap (pid, 8) & AARCH64_HWCAP_PACA);
>         /* A-profile MTE is 64-bit only.  */
> -      features.mte = linux_get_hwcap2 (8) & HWCAP2_MTE;
> +      features.mte = linux_get_hwcap2 (pid, 8) & HWCAP2_MTE;
>         features.tls = true;
>   
>         current_process ()->tdesc = aarch64_linux_read_description (features);
> @@ -3299,7 +3300,7 @@ aarch64_target::supports_memory_tagging ()
>   #endif
>       }
>   
> -  return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
> +  return (linux_get_hwcap2 (pid_of (current_thread), 8) & HWCAP2_MTE) != 0;
>   }
>   
>   bool
> diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
> index a458b0f14a68..1420909626e8 100644
> --- a/gdbserver/linux-arm-low.cc
> +++ b/gdbserver/linux-arm-low.cc
> @@ -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 (4);
> +  unsigned long arm_hwcap = linux_get_hwcap (pid_of (current_thread), 4);
>   
>     if (arm_hwcap & HWCAP_IWMMXT)
>       return arm_linux_read_description (ARM_FP_TYPE_IWMMXT);
> diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
> index a896b37528b6..fc496275d6a3 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5480,12 +5480,11 @@ linux_process_target::supports_read_auxv ()
>      to debugger memory starting at MYADDR.  */
>   
>   int
> -linux_process_target::read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> -				 unsigned int len)
> +linux_process_target::read_auxv (int pid, CORE_ADDR offset,
> +				 unsigned char *myaddr, unsigned int len)
>   {
>     char filename[PATH_MAX];
>     int fd, n;
> -  int pid = lwpid_of (current_thread);
>   
>     xsnprintf (filename, sizeof filename, "/proc/%d/auxv", pid);
>   
> @@ -6979,14 +6978,15 @@ linux_get_pc_64bit (struct regcache *regcache)
>   /* See linux-low.h.  */
>   
>   int
> -linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
> +linux_get_auxv (int pid, int wordsize, CORE_ADDR match, CORE_ADDR *valp)
>   {
>     gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
>     int offset = 0;
>   
>     gdb_assert (wordsize == 4 || wordsize == 8);
>   
> -  while (the_target->read_auxv (offset, data, 2 * wordsize) == 2 * wordsize)
> +  while (the_target->read_auxv (pid, offset, data, 2 * wordsize)
> +	 == 2 * wordsize)
>       {
>         if (wordsize == 4)
>   	{
> @@ -7016,20 +7016,20 @@ linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
>   /* See linux-low.h.  */
>   
>   CORE_ADDR
> -linux_get_hwcap (int wordsize)
> +linux_get_hwcap (int pid, int wordsize)
>   {
>     CORE_ADDR hwcap = 0;
> -  linux_get_auxv (wordsize, AT_HWCAP, &hwcap);
> +  linux_get_auxv (pid, wordsize, AT_HWCAP, &hwcap);
>     return hwcap;
>   }
>   
>   /* See linux-low.h.  */
>   
>   CORE_ADDR
> -linux_get_hwcap2 (int wordsize)
> +linux_get_hwcap2 (int pid, int wordsize)
>   {
>     CORE_ADDR hwcap2 = 0;
> -  linux_get_auxv (wordsize, AT_HWCAP2, &hwcap2);
> +  linux_get_auxv (pid, wordsize, AT_HWCAP2, &hwcap2);
>     return hwcap2;
>   }
>   
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 1594f063f47c..182a540f3bb3 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -178,7 +178,7 @@ class linux_process_target : public process_stratum_target
>   
>     bool supports_read_auxv () override;
>   
> -  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> +  int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>   		 unsigned int len) override;
>   
>     int insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
> @@ -946,17 +946,16 @@ extern int have_ptrace_getregset;
>      *VALP and return 1.  If not found or if there is an error, return
>      0.  */
>   
> -int linux_get_auxv (int wordsize, CORE_ADDR match,
> -		    CORE_ADDR *valp);
> +int linux_get_auxv (int pid, int wordsize, CORE_ADDR match, CORE_ADDR *valp);
>   
>   /* Fetch the AT_HWCAP entry from the auxv vector, where entries are length
>      WORDSIZE.  If no entry was found, return zero.  */
>   
> -CORE_ADDR linux_get_hwcap (int wordsize);
> +CORE_ADDR linux_get_hwcap (int pid, int wordsize);
>   
>   /* Fetch the AT_HWCAP2 entry from the auxv vector, where entries are length
>      WORDSIZE.  If no entry was found, return zero.  */
>   
> -CORE_ADDR linux_get_hwcap2 (int wordsize);
> +CORE_ADDR linux_get_hwcap2 (int pid, int wordsize);
>   
>   #endif /* GDBSERVER_LINUX_LOW_H */
> diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
> index 08824887003b..1db20d998ffd 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 (features.wordsize);
> -  ppc_hwcap2 = linux_get_hwcap2 (features.wordsize);
> +  ppc_hwcap = linux_get_hwcap (pid_of (current_thread), features.wordsize);
> +  ppc_hwcap2 = linux_get_hwcap2 (pid_of (current_thread), 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 (wordsize, AT_PHDR, &phdr))
> +  if (!linux_get_auxv (pid_of (current_thread), wordsize, AT_PHDR, &phdr))
>       return def_res;
>   
>     /* Assume ELF header is at the beginning of the page where program headers
> diff --git a/gdbserver/linux-s390-low.cc b/gdbserver/linux-s390-low.cc
> index 5adc28070574..2d030a806350 100644
> --- a/gdbserver/linux-s390-low.cc
> +++ b/gdbserver/linux-s390-low.cc
> @@ -592,7 +592,7 @@ s390_target::low_arch_setup ()
>     /* Determine word size and HWCAP.  */
>     int pid = pid_of (current_thread);
>     int wordsize = s390_get_wordsize (pid);
> -  unsigned long hwcap = linux_get_hwcap (wordsize);
> +  unsigned long hwcap = linux_get_hwcap (pid, wordsize);
>   
>     /* Check whether the kernel supports extra register sets.  */
>     int have_regset_last_break
> diff --git a/gdbserver/netbsd-low.cc b/gdbserver/netbsd-low.cc
> index f05bcd4e173f..03af80049457 100644
> --- a/gdbserver/netbsd-low.cc
> +++ b/gdbserver/netbsd-low.cc
> @@ -581,11 +581,9 @@ netbsd_read_auxv(pid_t pid, void *offs, void *addr, size_t len)
>      to debugger memory starting at MYADDR.  */
>   
>   int
> -netbsd_process_target::read_auxv (CORE_ADDR offset,
> +netbsd_process_target::read_auxv (int pid, CORE_ADDR offset,
>   				  unsigned char *myaddr, unsigned int len)
>   {
> -  pid_t pid = pid_of (current_thread);
> -
>     return netbsd_read_auxv (pid, (void *) (intptr_t) offset, myaddr, len);
>   }
>   
> diff --git a/gdbserver/netbsd-low.h b/gdbserver/netbsd-low.h
> index 06b4fd613571..eced7b3dd860 100644
> --- a/gdbserver/netbsd-low.h
> +++ b/gdbserver/netbsd-low.h
> @@ -77,7 +77,7 @@ class netbsd_process_target : public process_stratum_target
>   
>     bool supports_read_auxv () override;
>   
> -  int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> +  int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>   		 unsigned int len) override;
>   
>     bool supports_hardware_single_step () override;
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index aaef38e00622..63c323071670 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1443,7 +1443,7 @@ handle_qxfer_auxv (const char *annex,
>     if (annex[0] != '\0' || current_thread == NULL)
>       return -1;
>   
> -  return the_target->read_auxv (offset, readbuf, len);
> +  return the_target->read_auxv (pid_of (current_thread), offset, readbuf, len);
>   }
>   
>   /* Handle qXfer:exec-file:read.  */
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index c06a67600b1f..3651dbb9e862 100644
> --- a/gdbserver/target.cc
> +++ b/gdbserver/target.cc
> @@ -344,8 +344,8 @@ process_stratum_target::supports_read_auxv ()
>   }
>   
>   int
> -process_stratum_target::read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> -				   unsigned int len)
> +process_stratum_target::read_auxv (int pid, CORE_ADDR offset,
> +				   unsigned char *myaddr, unsigned int len)
>   {
>     gdb_assert_not_reached ("target op read_auxv not supported");
>   }
> diff --git a/gdbserver/target.h b/gdbserver/target.h
> index 18ab969dda70..fe7a80b645bc 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -175,7 +175,7 @@ class process_stratum_target
>     /* Read auxiliary vector data from the inferior process.
>   
>        Read LEN bytes at OFFSET into a buffer at MYADDR.  */
> -  virtual int read_auxv (CORE_ADDR offset, unsigned char *myaddr,
> +  virtual int read_auxv (int pid, CORE_ADDR offset, unsigned char *myaddr,
>   			 unsigned int len);
>   
>     /* Returns true if GDB Z breakpoint type TYPE is supported, false