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

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

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>