[v3,2/8] gdbserver: Add PID parameter to linux_get_auxv and linux_get_hwcap
Commit Message
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.
Changes are propagated to allow passing the new parameter through the call
chain.
Approved-By: Simon Marchi <simon.marchi@efficios.com>
---
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 | 3 ++-
gdbserver/target.cc | 4 ++--
gdbserver/target.h | 4 ++--
11 files changed, 30 insertions(+), 31 deletions(-)
Comments
On 1/30/23 04:45, Thiago Jung Bauermann via Gdb-patches 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.
>
> Changes are propagated to allow passing the new parameter through the call
> chain.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> ---
> 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 | 3 ++-
> gdbserver/target.cc | 4 ++--
> gdbserver/target.h | 4 ++--
> 11 files changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 3c09e086afee..2ed6e95562c5 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -846,12 +846,13 @@ aarch64_target::low_arch_setup ()
> if (is_elf64)
> {
> struct aarch64_features features;
> + int pid = current_thread->id.pid ();
>
> 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 = aarch64_tls_register_count (tid);
>
> current_process ()->tdesc = aarch64_linux_read_description (features);
> @@ -3322,7 +3323,7 @@ aarch64_target::supports_memory_tagging ()
> #endif
> }
>
> - return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
> + return (linux_get_hwcap2 (current_thread->id.pid (), 8) & HWCAP2_MTE) != 0;
> }
>
> bool
> diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
> index 98ba0e02524b..5975b44af0ae 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 (current_thread->id.pid (), 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 7e1de3978933..5cd22824e470 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5483,12 +5483,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);
>
> @@ -6982,14 +6981,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)
> {
> @@ -7019,20 +7019,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 aebfe05707e5..221de85aa2ee 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 fdf74727e392..f8dd770b8eb3 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 (current_thread->id.pid (), features.wordsize);
> + ppc_hwcap2 = linux_get_hwcap2 (current_thread->id.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 (wordsize, AT_PHDR, &phdr))
> + if (!linux_get_auxv (current_thread->id.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/linux-s390-low.cc b/gdbserver/linux-s390-low.cc
> index b3ccdff3e57f..48f64ef7bb3b 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 15afa36d1152..4defd79eee47 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 daefc302785e..050b43fc54f4 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 d802e8b4a341..21fb51a45d16 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -1443,7 +1443,8 @@ 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 (current_thread->id.pid (), offset, readbuf,
> + len);
> }
>
> /* Handle qXfer:exec-file:read. */
> diff --git a/gdbserver/target.cc b/gdbserver/target.cc
> index 24b8e2160138..b4f8e9f1efb6 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 eea651c30b4b..1b0caa98c56a 100644
> --- a/gdbserver/target.h
> +++ b/gdbserver/target.h
> @@ -172,10 +172,10 @@ class process_stratum_target
> /* Return true if the read_auxv target op is supported. */
> virtual bool supports_read_auxv ();
>
> - /* Read auxiliary vector data from the inferior process.
> + /* Read auxiliary vector data from the process with pid PID.
>
> 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
I don't have comments on this patch. Look sane to me.
Thiago Jung Bauermann via Gdb-patches <gdb-patches@sourceware.org>
writes:
> 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.
>
> Changes are propagated to allow passing the new parameter through the call
> chain.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> ---
> 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 | 3 ++-
> gdbserver/target.cc | 4 ++--
> gdbserver/target.h | 4 ++--
> 11 files changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 3c09e086afee..2ed6e95562c5 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -846,12 +846,13 @@ aarch64_target::low_arch_setup ()
> if (is_elf64)
> {
> struct aarch64_features features;
> + int pid = current_thread->id.pid ();
>
> 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 = aarch64_tls_register_count (tid);
>
> current_process ()->tdesc = aarch64_linux_read_description (features);
> @@ -3322,7 +3323,7 @@ aarch64_target::supports_memory_tagging ()
> #endif
> }
>
> - return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
> + return (linux_get_hwcap2 (current_thread->id.pid (), 8) & HWCAP2_MTE) != 0;
> }
>
> bool
> diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
> index 98ba0e02524b..5975b44af0ae 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 (current_thread->id.pid (), 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 7e1de3978933..5cd22824e470 100644
> --- a/gdbserver/linux-low.cc
> +++ b/gdbserver/linux-low.cc
> @@ -5483,12 +5483,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);
>
> @@ -6982,14 +6981,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)
> {
> @@ -7019,20 +7019,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 aebfe05707e5..221de85aa2ee 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);
Ideally the comment for these three functions would be updated to
mention the PID argument.
Thanks,
Andrew
>> @@ -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);
>
> Ideally the comment for these three functions would be updated to
> mention the PID argument.
I think the patch is OK to push after addressing this comment.
Simon
Simon Marchi <simon.marchi@efficios.com> writes:
>>> @@ -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);
>>
>> Ideally the comment for these three functions would be updated to
>> mention the PID argument.
This is how I changed the comments. What do you think?
diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
index 221de85aa2ee..4f95a27808ba 100644
--- a/gdbserver/linux-low.h
+++ b/gdbserver/linux-low.h
@@ -941,20 +941,19 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
extern int have_ptrace_getregset;
-/* Search for the value with type MATCH in the auxv vector with
- entries of length WORDSIZE bytes. If found, store the value in
- *VALP and return 1. If not found or if there is an error, return
- 0. */
+/* Search for the value with type MATCH in the auxv vector, with entries of
+ length WORDSIZE bytes, of process with PID. If found, store the value
+ in *VALP and return 1. If not found or if there is an error, return 0. */
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. */
+ WORDSIZE, of process with PID. If no entry was found, return zero. */
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. */
+ WORDSIZE, of process with PID. If no entry was found, return zero. */
CORE_ADDR linux_get_hwcap2 (int pid, int wordsize);
> I think the patch is OK to push after addressing this comment.
Thanks!
On 2/1/23 14:33, Thiago Jung Bauermann via Gdb-patches wrote:
>
> Simon Marchi <simon.marchi@efficios.com> writes:
>
>>>> @@ -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);
>>>
>>> Ideally the comment for these three functions would be updated to
>>> mention the PID argument.
>
> This is how I changed the comments. What do you think?
>
> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
> index 221de85aa2ee..4f95a27808ba 100644
> --- a/gdbserver/linux-low.h
> +++ b/gdbserver/linux-low.h
> @@ -941,20 +941,19 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
>
> extern int have_ptrace_getregset;
>
> -/* Search for the value with type MATCH in the auxv vector with
> - entries of length WORDSIZE bytes. If found, store the value in
> - *VALP and return 1. If not found or if there is an error, return
> - 0. */
> +/* Search for the value with type MATCH in the auxv vector, with entries of
> + length WORDSIZE bytes, of process with PID. If found, store the value
"of process with PID" sounds weird, syntactically. I would say "of process with pid
PID" (same in the other comments). Otherwise, this is OK.
Simon
Simon Marchi <simark@simark.ca> writes:
> On 2/1/23 14:33, Thiago Jung Bauermann via Gdb-patches wrote:
>>
>> Simon Marchi <simon.marchi@efficios.com> writes:
>>
>>>>> @@ -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);
>>>>
>>>> Ideally the comment for these three functions would be updated to
>>>> mention the PID argument.
>>
>> This is how I changed the comments. What do you think?
>>
>> diff --git a/gdbserver/linux-low.h b/gdbserver/linux-low.h
>> index 221de85aa2ee..4f95a27808ba 100644
>> --- a/gdbserver/linux-low.h
>> +++ b/gdbserver/linux-low.h
>> @@ -941,20 +941,19 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int
>> *handle_len);
>>
>> extern int have_ptrace_getregset;
>>
>> -/* Search for the value with type MATCH in the auxv vector with
>> - entries of length WORDSIZE bytes. If found, store the value in
>> - *VALP and return 1. If not found or if there is an error, return
>> - 0. */
>> +/* Search for the value with type MATCH in the auxv vector, with entries of
>> + length WORDSIZE bytes, of process with PID. If found, store the value
>
> "of process with PID" sounds weird, syntactically. I would say "of process with pid
> PID" (same in the other comments). Otherwise, this is OK.
I adopted this wording and pushed this patch as well as patch 1 in this
series.
Thank you again for your, Andrew's and Luis' reviews on these patches.
On 2023-01-30 4:45 a.m., Thiago Jung Bauermann via Gdb-patches wrote:
> This patch doesn't change gdbserver behaviour,
That's not actually true -- before the patch, we use the lwpid
of the current thread, while after the patch we always use the pid of the
process.
Here:
> 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);
>
AFAICT by playing with debugging the gdb.threads/leader-exit.exp program,
you can't read /proc/PID/auxv if the leader thread has exited (is zombie).
Maybe that ends up not mattering in practice, not sure, but it is a
behavior change.
> 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.
>
> Changes are propagated to allow passing the new parameter through the call
> chain.
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>
> ---
> 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 | 3 ++-
> gdbserver/target.cc | 4 ++--
> gdbserver/target.h | 4 ++--
> 11 files changed, 30 insertions(+), 31 deletions(-)
>
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 3c09e086afee..2ed6e95562c5 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -846,12 +846,13 @@ aarch64_target::low_arch_setup ()
> if (is_elf64)
> {
> struct aarch64_features features;
> + int pid = current_thread->id.pid ();
You can also write:
current_process ()->pid
which is a more to the point. Also, sometimes there's a current
process, but not a current thread, so when we don't actually need a thread,
current_process() is better.
Pedro Alves
On 2/6/23 14:54, Pedro Alves wrote:
> AFAICT by playing with debugging the gdb.threads/leader-exit.exp program,
> you can't read /proc/PID/auxv if the leader thread has exited (is zombie).
>
> Maybe that ends up not mattering in practice, not sure, but it is a
> behavior change.
Even if we can't read /proc/PID/auxv if the leader thread has exited,
auxv is still a process-specific resource, not a thread-specific one,
right? So, I suppose that the target interface could still accept a
pid, but linux_process_target could pick an arbitrary non-exited thread
from that pid to read auxv from? Although I don't know what would
happen if that chose thread has just exited and we haven't consumed the
event yet.
Simon
On 2023-02-06 8:16 p.m., Simon Marchi wrote:
> On 2/6/23 14:54, Pedro Alves wrote:
>> AFAICT by playing with debugging the gdb.threads/leader-exit.exp program,
>> you can't read /proc/PID/auxv if the leader thread has exited (is zombie).
>>
>> Maybe that ends up not mattering in practice, not sure, but it is a
>> behavior change.
>
> Even if we can't read /proc/PID/auxv if the leader thread has exited,
> auxv is still a process-specific resource, not a thread-specific one,
> right?
Right.
> So, I suppose that the target interface could still accept a
> pid, but linux_process_target could pick an arbitrary non-exited thread
> from that pid to read auxv from?
Right, I think we can do that.
> Although I don't know what would
> happen if that chose thread has just exited and we haven't consumed the
> event yet.
Not sure either. And I don't know what happens if the thread hasn't exited
when when we open /proc/TID/auxv and then the thread exits before we read
from the file. These cases should be confirmed. We may need to keep looking
for a thread until we manage to open & read the file or run out of threads.
BTW, I just noticed this "current_thread == NULL" check here:
/* Handle qXfer:auxv:read. */
static int
handle_qxfer_auxv (const char *annex,
gdb_byte *readbuf, const gdb_byte *writebuf,
ULONGEST offset, LONGEST len)
{
if (!the_target->supports_read_auxv () || writebuf != NULL)
return -2;
if (annex[0] != '\0' || current_thread == NULL)
return -1;
Since we're removing the thread dependency, I think that check should be replaced by
current_process() == NULL instead.
Pedro Alves <pedro@palves.net> writes:
> On 2023-02-06 8:16 p.m., Simon Marchi wrote:
>> On 2/6/23 14:54, Pedro Alves wrote:
>>> AFAICT by playing with debugging the gdb.threads/leader-exit.exp program,
>>> you can't read /proc/PID/auxv if the leader thread has exited (is zombie).
>>>
>>> Maybe that ends up not mattering in practice, not sure, but it is a
>>> behavior change.
>>
>> Even if we can't read /proc/PID/auxv if the leader thread has exited,
>> auxv is still a process-specific resource, not a thread-specific one,
>> right?
>
> Right.
>
>> So, I suppose that the target interface could still accept a
>> pid, but linux_process_target could pick an arbitrary non-exited thread
>> from that pid to read auxv from?
>
> Right, I think we can do that.
>
>> Although I don't know what would
>> happen if that chose thread has just exited and we haven't consumed the
>> event yet.
>
> Not sure either. And I don't know what happens if the thread hasn't
> exited when when we open /proc/TID/auxv and then the thread exits
> before we read from the file. These cases should be confirmed. We may
> need to keep looking for a thread until we manage to open & read the
> file or run out of threads.
Ok, I will look into that. A simpler approach though would be to fetch
auxv at process starts and cache it for the duration of the debugging
session (since auxv doesn't change at runtime, AFAIK). Would that be a
good solution?
> BTW, I just noticed this "current_thread == NULL" check here:
>
> /* Handle qXfer:auxv:read. */
>
> static int
> handle_qxfer_auxv (const char *annex,
> gdb_byte *readbuf, const gdb_byte *writebuf,
> ULONGEST offset, LONGEST len)
> {
> if (!the_target->supports_read_auxv () || writebuf != NULL)
> return -2;
>
> if (annex[0] != '\0' || current_thread == NULL)
> return -1;
>
>
> Since we're removing the thread dependency, I think that check should
> be replaced by current_process() == NULL instead.
Ok, I'll submit a patch with this change.
Pedro Alves <pedro@palves.net> writes:
> On 2023-01-30 4:45 a.m., Thiago Jung Bauermann via Gdb-patches wrote:
>> This patch doesn't change gdbserver behaviour,
>
> That's not actually true -- before the patch, we use the lwpid
> of the current thread, while after the patch we always use the pid of the
> process.
Ah, indeed that is true. Thanks for pointing it out.
> Here:
>
>> 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);
>>
>
> AFAICT by playing with debugging the gdb.threads/leader-exit.exp program,
> you can't read /proc/PID/auxv if the leader thread has exited (is zombie).
>
> Maybe that ends up not mattering in practice, not sure, but it is a
> behavior change.
We are talking about this in the other branch of this email thread so I
won't respond here, but I'll leave the quote above because I reference
it below.
>> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
>> index 3c09e086afee..2ed6e95562c5 100644
>> --- a/gdbserver/linux-aarch64-low.cc
>> +++ b/gdbserver/linux-aarch64-low.cc
>> @@ -846,12 +846,13 @@ aarch64_target::low_arch_setup ()
>> if (is_elf64)
>> {
>> struct aarch64_features features;
>> + int pid = current_thread->id.pid ();
>
> You can also write:
>
> current_process ()->pid
>
> which is a more to the point. Also, sometimes there's a current
> process, but not a current thread, so when we don't actually need a thread,
> current_process() is better.
In the case of the spots changed by this patch, we know that
current_thread is always set because, as can be seen in the first code
hunk you quoted, before this patch was applied
linux_process_target::read_auxv dereferenced it.
But I agree that it makes more sense to use current_process (). I'll
post a new patch with the change.
> Ok, I will look into that. A simpler approach though would be to fetch
> auxv at process starts and cache it for the duration of the debugging
> session (since auxv doesn't change at runtime, AFAIK). Would that be a
> good solution?
Not sure, you could be attaching, and at the moment you attach, the
leader has already exited.
Simon
Simon Marchi <simon.marchi@efficios.com> writes:
>> Ok, I will look into that. A simpler approach though would be to fetch
>> auxv at process starts and cache it for the duration of the debugging
>> session (since auxv doesn't change at runtime, AFAIK). Would that be a
>> good solution?
>
> Not sure, you could be attaching, and at the moment you attach, the
> leader has already exited.
Ah, that is true. I'll work on a solution following what Pedro
described, then.
@@ -846,12 +846,13 @@ aarch64_target::low_arch_setup ()
if (is_elf64)
{
struct aarch64_features features;
+ int pid = current_thread->id.pid ();
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 = aarch64_tls_register_count (tid);
current_process ()->tdesc = aarch64_linux_read_description (features);
@@ -3322,7 +3323,7 @@ aarch64_target::supports_memory_tagging ()
#endif
}
- return (linux_get_hwcap2 (8) & HWCAP2_MTE) != 0;
+ return (linux_get_hwcap2 (current_thread->id.pid (), 8) & HWCAP2_MTE) != 0;
}
bool
@@ -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 (current_thread->id.pid (), 4);
if (arm_hwcap & HWCAP_IWMMXT)
return arm_linux_read_description (ARM_FP_TYPE_IWMMXT);
@@ -5483,12 +5483,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);
@@ -6982,14 +6981,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)
{
@@ -7019,20 +7019,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;
}
@@ -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 */
@@ -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 (current_thread->id.pid (), features.wordsize);
+ ppc_hwcap2 = linux_get_hwcap2 (current_thread->id.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 (wordsize, AT_PHDR, &phdr))
+ if (!linux_get_auxv (current_thread->id.pid (), wordsize, AT_PHDR, &phdr))
return def_res;
/* Assume ELF header is at the beginning of the page where program headers
@@ -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
@@ -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);
}
@@ -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;
@@ -1443,7 +1443,8 @@ 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 (current_thread->id.pid (), offset, readbuf,
+ len);
}
/* Handle qXfer:exec-file:read. */
@@ -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");
}
@@ -172,10 +172,10 @@ class process_stratum_target
/* Return true if the read_auxv target op is supported. */
virtual bool supports_read_auxv ();
- /* Read auxiliary vector data from the inferior process.
+ /* Read auxiliary vector data from the process with pid PID.
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