[PATCH] gdb/aarch64: prevent crash from in process agent

[PATCH] gdb/aarch64: prevent crash from in process agent

[Andrew Burgess]

Since this commit:

  commit 0ee6b1c511c0e2a6793568692d2e5418cd6bc10d
  Date:   Wed May 18 13:32:04 2022 -0700

      Use aarch64_features to describe register features in target descriptions.

There has been an issue with how aarch64 target descriptions are
cached within gdbserver, and specifically, how this caching impacts
the in process agent (IPA).

The function initialize_tracepoint_ftlib (gdbserver/tracepoint.cc) is
part of the IPA, this function is a constructor function, i.e. is
called as part of the global initialisation process.  We can't
guarantee the ordering of when this function is called vs when other
global state is initialised.

Now initialize_tracepoint_ftlib calls initialize_tracepoint, which
calls initialize_low_tracepoint, which for aarch64 calls
aarch64_linux_read_description.

The aarch64_linux_read_description function lives in
linux-aarch64-tdesc.cc and after the above commit, depends on a
std::unordered_map having been initialized.

Prior to the above commit aarch64_linux_read_description used a global
C style array, which obviously requires no runtime initialization.

The consequence of the above is that any inferior linked with the IPA
(for aarch64) will experience undefined behaviour (access to an
uninitialized std::unordered_map) during startup, which for me
manifests as a segfault.

I propose fixing this by moving the std::unordered_map into the
function body, but leaving it static.  The map will now be initialized
the first time the function is called, which removes the undefiend
behaviour.

The same problem exists for the expedited_registers global, however
this global can just be made into a function local instead.  The
expedited_registers variable is used to build a pointer list which is
then passed to init_target_desc, however init_target_desc copies the
values it is given so expedited_registers does not need to live longer
than its containing function.

On most of the AArch64 machines I have access too tracing is not
supported, and so the gdb.trace/*.exp tests that use the IPA just exit
early reporting unsupported.  I've added a test which links an
inferior with the IPA and just starts the inferior.  No tracing is
performed.  This exposes the current issue even on hosts that don't
support tracing.  After this patch the test passes.
---
 gdb/testsuite/gdb.trace/basic-libipa.c   | 22 ++++++++++++
 gdb/testsuite/gdb.trace/basic-libipa.exp | 45 ++++++++++++++++++++++++
 gdbserver/linux-aarch64-tdesc.cc         | 22 +++++++-----
 3 files changed, 81 insertions(+), 8 deletions(-)
 create mode 100644 gdb/testsuite/gdb.trace/basic-libipa.c
 create mode 100644 gdb/testsuite/gdb.trace/basic-libipa.exp

diff --git a/gdb/testsuite/gdb.trace/basic-libipa.c b/gdb/testsuite/gdb.trace/basic-libipa.c
new file mode 100644
index 00000000000..bbcfb01316e
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/basic-libipa.c
@@ -0,0 +1,22 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2024 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/>.  */
+
+int
+main ()
+{
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.trace/basic-libipa.exp b/gdb/testsuite/gdb.trace/basic-libipa.exp
new file mode 100644
index 00000000000..c457d6afc36
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/basic-libipa.exp
@@ -0,0 +1,45 @@
+# Copyright 2024 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/>.
+
+# Very simple test that links with libinproctrace.so and then starts
+# the resulting inferior.
+#
+# This test should run on targets that don't support tracing, but for
+# which libinproctrace.so is built and helps catch some issues where
+# libinproctrace.so is so broken inferiors wont even start.
+
+load_lib "trace-support.exp"
+
+require allow_shlib_tests
+
+standard_testfile
+
+set libipa [get_in_proc_agent]
+gdb_download_shlib $libipa
+
+if { ![file exists $libipa] } {
+    unsupported "missing libinproctrace.so"
+    return -1
+}
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
+	  [list debug shlib=$libipa]] } {
+    return -1
+}
+
+if {![runto_main]} {
+    return -1
+}
+
+pass "inferior with libinproctrace.so started"
diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
index 0ed9a42cbff..5d3b6ddffff 100644
--- a/gdbserver/linux-aarch64-tdesc.cc
+++ b/gdbserver/linux-aarch64-tdesc.cc
@@ -26,16 +26,17 @@
 #include <inttypes.h>
 #include <unordered_map>
 
-/* All possible aarch64 target descriptors.  */
-static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
-
-static std::vector<const char *> expedited_registers;
-
 /* Create the aarch64 target description.  */
 
 const target_desc *
 aarch64_linux_read_description (const aarch64_features &features)
 {
+  /* All possible aarch64 target descriptors.  This map must live within
+     this function as the in-process-agent calls this function from a
+     constructor function, when globals might not yet have been
+     initialised.  */
+  static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
+
   if (features.vq > AARCH64_MAX_SVE_VQ)
     error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
 	   AARCH64_MAX_SVE_VQ);
@@ -50,10 +51,15 @@ aarch64_linux_read_description (const aarch64_features &features)
   if (tdesc == NULL)
     {
       tdesc = aarch64_create_target_description (features);
-      expedited_registers.clear ();
 
-      /* Configure the expedited registers.  By default we include x29, sp and
-	 pc.  */
+      /* Configure the expedited registers.  By default we include x29, sp
+	 and pc, but we allow for up to 6 pointers as this is (currently)
+	 the most that we push.
+
+	 Calling init_target_desc takes a copy of all the strings pointed
+	 to by expedited_registers so this vector only needs to live for
+	 the scope of this function.  */
+      std::vector<const char *> expedited_registers (6);
       expedited_registers.push_back ("x29");
       expedited_registers.push_back ("sp");
       expedited_registers.push_back ("pc");

base-commit: 6dfd07222c02edc792447049ba94518ae982f362
-- 
2.25.4

Re: [PATCH] gdb/aarch64: prevent crash from in process agent

[Luis Machado]

On 6/13/24 10:42, Andrew Burgess wrote:
> Since this commit:
> 
>   commit 0ee6b1c511c0e2a6793568692d2e5418cd6bc10d
>   Date:   Wed May 18 13:32:04 2022 -0700
> 
>       Use aarch64_features to describe register features in target descriptions.
> 
> There has been an issue with how aarch64 target descriptions are
> cached within gdbserver, and specifically, how this caching impacts
> the in process agent (IPA).
> 
> The function initialize_tracepoint_ftlib (gdbserver/tracepoint.cc) is
> part of the IPA, this function is a constructor function, i.e. is
> called as part of the global initialisation process.  We can't
> guarantee the ordering of when this function is called vs when other
> global state is initialised.
> 
> Now initialize_tracepoint_ftlib calls initialize_tracepoint, which
> calls initialize_low_tracepoint, which for aarch64 calls
> aarch64_linux_read_description.
> 
> The aarch64_linux_read_description function lives in
> linux-aarch64-tdesc.cc and after the above commit, depends on a
> std::unordered_map having been initialized.
> 
> Prior to the above commit aarch64_linux_read_description used a global
> C style array, which obviously requires no runtime initialization.
> 
> The consequence of the above is that any inferior linked with the IPA
> (for aarch64) will experience undefined behaviour (access to an
> uninitialized std::unordered_map) during startup, which for me
> manifests as a segfault.
> 
> I propose fixing this by moving the std::unordered_map into the
> function body, but leaving it static.  The map will now be initialized
> the first time the function is called, which removes the undefiend
> behaviour.
> 
> The same problem exists for the expedited_registers global, however
> this global can just be made into a function local instead.  The
> expedited_registers variable is used to build a pointer list which is
> then passed to init_target_desc, however init_target_desc copies the
> values it is given so expedited_registers does not need to live longer
> than its containing function.
> 
> On most of the AArch64 machines I have access too tracing is not
> supported, and so the gdb.trace/*.exp tests that use the IPA just exit
> early reporting unsupported.  I've added a test which links an
> inferior with the IPA and just starts the inferior.  No tracing is
> performed.  This exposes the current issue even on hosts that don't
> support tracing.  After this patch the test passes.
> ---
>  gdb/testsuite/gdb.trace/basic-libipa.c   | 22 ++++++++++++
>  gdb/testsuite/gdb.trace/basic-libipa.exp | 45 ++++++++++++++++++++++++
>  gdbserver/linux-aarch64-tdesc.cc         | 22 +++++++-----
>  3 files changed, 81 insertions(+), 8 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.trace/basic-libipa.c
>  create mode 100644 gdb/testsuite/gdb.trace/basic-libipa.exp
> 
> diff --git a/gdb/testsuite/gdb.trace/basic-libipa.c b/gdb/testsuite/gdb.trace/basic-libipa.c
> new file mode 100644
> index 00000000000..bbcfb01316e
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/basic-libipa.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2024 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/>.  */
> +
> +int
> +main ()
> +{
> +  return 0;
> +}
> diff --git a/gdb/testsuite/gdb.trace/basic-libipa.exp b/gdb/testsuite/gdb.trace/basic-libipa.exp
> new file mode 100644
> index 00000000000..c457d6afc36
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/basic-libipa.exp
> @@ -0,0 +1,45 @@
> +# Copyright 2024 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/>.
> +
> +# Very simple test that links with libinproctrace.so and then starts
> +# the resulting inferior.
> +#
> +# This test should run on targets that don't support tracing, but for
> +# which libinproctrace.so is built and helps catch some issues where
> +# libinproctrace.so is so broken inferiors wont even start.
> +
> +load_lib "trace-support.exp"
> +
> +require allow_shlib_tests
> +
> +standard_testfile
> +
> +set libipa [get_in_proc_agent]
> +gdb_download_shlib $libipa
> +
> +if { ![file exists $libipa] } {
> +    unsupported "missing libinproctrace.so"
> +    return -1
> +}
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
> +	  [list debug shlib=$libipa]] } {
> +    return -1
> +}
> +
> +if {![runto_main]} {
> +    return -1
> +}
> +
> +pass "inferior with libinproctrace.so started"
> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
> index 0ed9a42cbff..5d3b6ddffff 100644
> --- a/gdbserver/linux-aarch64-tdesc.cc
> +++ b/gdbserver/linux-aarch64-tdesc.cc
> @@ -26,16 +26,17 @@
>  #include <inttypes.h>
>  #include <unordered_map>
>  
> -/* All possible aarch64 target descriptors.  */
> -static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
> -
> -static std::vector<const char *> expedited_registers;
> -
>  /* Create the aarch64 target description.  */
>  
>  const target_desc *
>  aarch64_linux_read_description (const aarch64_features &features)
>  {
> +  /* All possible aarch64 target descriptors.  This map must live within
> +     this function as the in-process-agent calls this function from a
> +     constructor function, when globals might not yet have been
> +     initialised.  */
> +  static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
> +
>    if (features.vq > AARCH64_MAX_SVE_VQ)
>      error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>  	   AARCH64_MAX_SVE_VQ);
> @@ -50,10 +51,15 @@ aarch64_linux_read_description (const aarch64_features &features)
>    if (tdesc == NULL)
>      {
>        tdesc = aarch64_create_target_description (features);
> -      expedited_registers.clear ();
>  
> -      /* Configure the expedited registers.  By default we include x29, sp and
> -	 pc.  */
> +      /* Configure the expedited registers.  By default we include x29, sp
> +	 and pc, but we allow for up to 6 pointers as this is (currently)
> +	 the most that we push.
> +
> +	 Calling init_target_desc takes a copy of all the strings pointed
> +	 to by expedited_registers so this vector only needs to live for
> +	 the scope of this function.  */
> +      std::vector<const char *> expedited_registers (6);
>        expedited_registers.push_back ("x29");
>        expedited_registers.push_back ("sp");
>        expedited_registers.push_back ("pc");
> 
> base-commit: 6dfd07222c02edc792447049ba94518ae982f362

Thanks. This is OK. Though, as I mentioned in previous threads touching
tracepoints/IPA, I think we should just rip out the IPA. Nobody seems
to be using it, and it is just a maintenance burden at this point.

Approved-By: Luis Machado <luis.machado@arm.com>

Re: [PATCH] gdb/aarch64: prevent crash from in process agent

[Andrew Burgess]

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

> On 6/13/24 10:42, Andrew Burgess wrote:
>> Since this commit:
>> 
>>   commit 0ee6b1c511c0e2a6793568692d2e5418cd6bc10d
>>   Date:   Wed May 18 13:32:04 2022 -0700
>> 
>>       Use aarch64_features to describe register features in target descriptions.
>> 
>> There has been an issue with how aarch64 target descriptions are
>> cached within gdbserver, and specifically, how this caching impacts
>> the in process agent (IPA).
>> 
>> The function initialize_tracepoint_ftlib (gdbserver/tracepoint.cc) is
>> part of the IPA, this function is a constructor function, i.e. is
>> called as part of the global initialisation process.  We can't
>> guarantee the ordering of when this function is called vs when other
>> global state is initialised.
>> 
>> Now initialize_tracepoint_ftlib calls initialize_tracepoint, which
>> calls initialize_low_tracepoint, which for aarch64 calls
>> aarch64_linux_read_description.
>> 
>> The aarch64_linux_read_description function lives in
>> linux-aarch64-tdesc.cc and after the above commit, depends on a
>> std::unordered_map having been initialized.
>> 
>> Prior to the above commit aarch64_linux_read_description used a global
>> C style array, which obviously requires no runtime initialization.
>> 
>> The consequence of the above is that any inferior linked with the IPA
>> (for aarch64) will experience undefined behaviour (access to an
>> uninitialized std::unordered_map) during startup, which for me
>> manifests as a segfault.
>> 
>> I propose fixing this by moving the std::unordered_map into the
>> function body, but leaving it static.  The map will now be initialized
>> the first time the function is called, which removes the undefiend
>> behaviour.
>> 
>> The same problem exists for the expedited_registers global, however
>> this global can just be made into a function local instead.  The
>> expedited_registers variable is used to build a pointer list which is
>> then passed to init_target_desc, however init_target_desc copies the
>> values it is given so expedited_registers does not need to live longer
>> than its containing function.
>> 
>> On most of the AArch64 machines I have access too tracing is not
>> supported, and so the gdb.trace/*.exp tests that use the IPA just exit
>> early reporting unsupported.  I've added a test which links an
>> inferior with the IPA and just starts the inferior.  No tracing is
>> performed.  This exposes the current issue even on hosts that don't
>> support tracing.  After this patch the test passes.
>> ---
>>  gdb/testsuite/gdb.trace/basic-libipa.c   | 22 ++++++++++++
>>  gdb/testsuite/gdb.trace/basic-libipa.exp | 45 ++++++++++++++++++++++++
>>  gdbserver/linux-aarch64-tdesc.cc         | 22 +++++++-----
>>  3 files changed, 81 insertions(+), 8 deletions(-)
>>  create mode 100644 gdb/testsuite/gdb.trace/basic-libipa.c
>>  create mode 100644 gdb/testsuite/gdb.trace/basic-libipa.exp
>> 
>> diff --git a/gdb/testsuite/gdb.trace/basic-libipa.c b/gdb/testsuite/gdb.trace/basic-libipa.c
>> new file mode 100644
>> index 00000000000..bbcfb01316e
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.trace/basic-libipa.c
>> @@ -0,0 +1,22 @@
>> +/* This testcase is part of GDB, the GNU debugger.
>> +
>> +   Copyright 2024 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/>.  */
>> +
>> +int
>> +main ()
>> +{
>> +  return 0;
>> +}
>> diff --git a/gdb/testsuite/gdb.trace/basic-libipa.exp b/gdb/testsuite/gdb.trace/basic-libipa.exp
>> new file mode 100644
>> index 00000000000..c457d6afc36
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.trace/basic-libipa.exp
>> @@ -0,0 +1,45 @@
>> +# Copyright 2024 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/>.
>> +
>> +# Very simple test that links with libinproctrace.so and then starts
>> +# the resulting inferior.
>> +#
>> +# This test should run on targets that don't support tracing, but for
>> +# which libinproctrace.so is built and helps catch some issues where
>> +# libinproctrace.so is so broken inferiors wont even start.
>> +
>> +load_lib "trace-support.exp"
>> +
>> +require allow_shlib_tests
>> +
>> +standard_testfile
>> +
>> +set libipa [get_in_proc_agent]
>> +gdb_download_shlib $libipa
>> +
>> +if { ![file exists $libipa] } {
>> +    unsupported "missing libinproctrace.so"
>> +    return -1
>> +}
>> +
>> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile \
>> +	  [list debug shlib=$libipa]] } {
>> +    return -1
>> +}
>> +
>> +if {![runto_main]} {
>> +    return -1
>> +}
>> +
>> +pass "inferior with libinproctrace.so started"
>> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
>> index 0ed9a42cbff..5d3b6ddffff 100644
>> --- a/gdbserver/linux-aarch64-tdesc.cc
>> +++ b/gdbserver/linux-aarch64-tdesc.cc
>> @@ -26,16 +26,17 @@
>>  #include <inttypes.h>
>>  #include <unordered_map>
>>  
>> -/* All possible aarch64 target descriptors.  */
>> -static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>> -
>> -static std::vector<const char *> expedited_registers;
>> -
>>  /* Create the aarch64 target description.  */
>>  
>>  const target_desc *
>>  aarch64_linux_read_description (const aarch64_features &features)
>>  {
>> +  /* All possible aarch64 target descriptors.  This map must live within
>> +     this function as the in-process-agent calls this function from a
>> +     constructor function, when globals might not yet have been
>> +     initialised.  */
>> +  static std::unordered_map<aarch64_features, target_desc *> tdesc_aarch64_map;
>> +
>>    if (features.vq > AARCH64_MAX_SVE_VQ)
>>      error (_("VQ is %" PRIu64 ", maximum supported value is %d"), features.vq,
>>  	   AARCH64_MAX_SVE_VQ);
>> @@ -50,10 +51,15 @@ aarch64_linux_read_description (const aarch64_features &features)
>>    if (tdesc == NULL)
>>      {
>>        tdesc = aarch64_create_target_description (features);
>> -      expedited_registers.clear ();
>>  
>> -      /* Configure the expedited registers.  By default we include x29, sp and
>> -	 pc.  */
>> +      /* Configure the expedited registers.  By default we include x29, sp
>> +	 and pc, but we allow for up to 6 pointers as this is (currently)
>> +	 the most that we push.
>> +
>> +	 Calling init_target_desc takes a copy of all the strings pointed
>> +	 to by expedited_registers so this vector only needs to live for
>> +	 the scope of this function.  */
>> +      std::vector<const char *> expedited_registers (6);
>>        expedited_registers.push_back ("x29");
>>        expedited_registers.push_back ("sp");
>>        expedited_registers.push_back ("pc");
>> 
>> base-commit: 6dfd07222c02edc792447049ba94518ae982f362
>
> Thanks. This is OK. Though, as I mentioned in previous threads touching
> tracepoints/IPA, I think we should just rip out the IPA. Nobody seems
> to be using it, and it is just a maintenance burden at this point.
>
> Approved-By: Luis Machado <luis.machado@arm.com>

Pushed.

Thanks,
Andrew