[PATCH v3] gdb: fix owner passed to remove_target_sections in clear_solib

[PATCH v3] gdb: fix owner passed to remove_target_sections in clear_solib

[Simon Marchi]

Commit 8971d2788e7 ("gdb: link so_list using intrusive_list") introduced
a bug in clear_solib.  Instead of passing an `so_list *` to
remove_target_sections, it passed an `so_list **`.  This was not caught
by the compiler, because remove_target_sections takes a `void *` as the
"owner", so you can pass it any pointer and it won't complain.

This happened because I previously had a patch to change the type of the
disposer parameter to be a reference rather than a pointer, so had to
change `so` to `&so`.  When dropping that patch, I forgot to revert this
bit and / or it got re-introduced when handling subsequent merge
conflicts.  And I didn't properly retest.

Fix that, but try to make things less error prone.  Add a union to
represent the possible owner kinds for a target_section.  Trying to pass
a pointer to another type than those will not compile.

Change-Id: I600cab5ea0408ccc5638467b760768161ca3036c

---

New in v3:

  - add a union type to represent the owner of a target_section

---
 gdb/exec.c           | 16 ++++++++--------
 gdb/maint.c          |  2 +-
 gdb/progspace.c      |  3 ++-
 gdb/progspace.h      |  4 ++--
 gdb/solib.c          |  2 +-
 gdb/target-section.h | 29 ++++++++++++++++++++++++-----
 6 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 0f9f9d076c68..503dfd197925 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -492,8 +492,8 @@ exec_file_attach (const char *filename, int from_tty)
       /* Add the executable's sections to the current address spaces'
 	 list of sections.  This possibly pushes the exec_ops
 	 target.  */
-      current_program_space->add_target_sections (&current_program_space->ebfd,
-						  sections);
+      current_program_space->add_target_sections
+	(current_program_space->ebfd.get (), sections);
 
       /* Tell display code (if any) about the changed file name.  */
       if (deprecated_exec_file_display_hook)
@@ -599,8 +599,8 @@ build_section_table (struct bfd *some_bfd)
    current set of target sections.  */
 
 void
-program_space::add_target_sections (const void *owner,
-				    const std::vector<target_section> &sections)
+program_space::add_target_sections
+  (target_section_owner owner, const std::vector<target_section> &sections)
 {
   if (!sections.empty ())
     {
@@ -643,7 +643,7 @@ program_space::add_target_sections (struct objfile *objfile)
 	continue;
 
       m_target_sections.emplace_back (osect->addr (), osect->endaddr (),
-				      osect->the_bfd_section, (void *) objfile);
+				      osect->the_bfd_section, objfile);
     }
 }
 
@@ -651,15 +651,15 @@ program_space::add_target_sections (struct objfile *objfile)
    OWNER must be the same value passed to add_target_sections.  */
 
 void
-program_space::remove_target_sections (const void *owner)
+program_space::remove_target_sections (target_section_owner owner)
 {
-  gdb_assert (owner != NULL);
+  gdb_assert (owner.v != nullptr);
 
   auto it = std::remove_if (m_target_sections.begin (),
 			    m_target_sections.end (),
 			    [&] (target_section &sect)
 			    {
-			      return sect.owner == owner;
+			      return sect.owner.v == owner.v;
 			    });
   m_target_sections.erase (it, m_target_sections.end ());
 
diff --git a/gdb/maint.c b/gdb/maint.c
index e0dc5bc0c7aa..cec5128ef834 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -509,7 +509,7 @@ maintenance_info_target_sections (const char *arg, int from_tty)
 		  (8 + digits), "",
 		  hex_string_custom (sec.addr, addr_size),
 		  hex_string_custom (sec.endaddr, addr_size),
-		  sec.owner);
+		  sec.owner.v);
     }
 }
 
diff --git a/gdb/progspace.c b/gdb/progspace.c
index cca651fdb6f8..839707e9d71f 100644
--- a/gdb/progspace.c
+++ b/gdb/progspace.c
@@ -207,10 +207,11 @@ program_space::exec_close ()
     {
       /* Removing target sections may close the exec_ops target.
 	 Clear ebfd before doing so to prevent recursion.  */
+      bfd *saved_ebfd = ebfd.get ();
       ebfd.reset (nullptr);
       ebfd_mtime = 0;
 
-      remove_target_sections (&ebfd);
+      remove_target_sections (saved_ebfd);
 
       exec_filename.reset (nullptr);
     }
diff --git a/gdb/progspace.h b/gdb/progspace.h
index a480f560a77a..a22e427400e9 100644
--- a/gdb/progspace.h
+++ b/gdb/progspace.h
@@ -284,11 +284,11 @@ struct program_space
   bool empty ();
 
   /* Remove all target sections owned by OWNER.  */
-  void remove_target_sections (const void *owner);
+  void remove_target_sections (target_section_owner owner);
 
   /* Add the sections array defined by SECTIONS to the
      current set of target sections.  */
-  void add_target_sections (const void *owner,
+  void add_target_sections (target_section_owner owner,
 			    const std::vector<target_section> &sections);
 
   /* Add the sections of OBJFILE to the current set of target
diff --git a/gdb/solib.c b/gdb/solib.c
index 71970636dc58..b9fb911a8101 100644
--- a/gdb/solib.c
+++ b/gdb/solib.c
@@ -1195,7 +1195,7 @@ clear_solib (void)
   current_program_space->so_list.clear_and_dispose ([] (shobj *so)
     {
       notify_solib_unloaded (current_program_space, *so);
-      current_program_space->remove_target_sections (&so);
+      current_program_space->remove_target_sections (so);
       delete so;
     });
 
diff --git a/gdb/target-section.h b/gdb/target-section.h
index 1c902baa7778..386d044bfa9f 100644
--- a/gdb/target-section.h
+++ b/gdb/target-section.h
@@ -20,6 +20,25 @@
 #ifndef GDB_TARGET_SECTION_H
 #define GDB_TARGET_SECTION_H
 
+struct bfd;
+struct objfile;
+struct shobj;
+
+/* A union representing the possible owner types of a target_section.  */
+
+union target_section_owner
+{
+  target_section_owner () : v (nullptr) {}
+  target_section_owner (const bfd *bfd) : bfd (bfd) {}
+  target_section_owner (const objfile *objfile) : objfile (objfile) {}
+  target_section_owner (const shobj *shobj) : shobj (shobj) {}
+
+  const struct bfd *bfd;
+  const struct objfile *objfile;
+  const struct shobj *shobj;
+  const void *v;
+};
+
 /* Struct target_section maps address ranges to file sections.  It is
    mostly used with BFD files, but can be used without (e.g. for handling
    raw disks, or files not in formats handled by BFD).  */
@@ -27,7 +46,7 @@
 struct target_section
 {
   target_section (CORE_ADDR addr_, CORE_ADDR end_, struct bfd_section *sect_,
-		  void *owner_ = nullptr)
+		  target_section_owner owner_ = {})
     : addr (addr_),
       endaddr (end_),
       the_bfd_section (sect_),
@@ -44,11 +63,11 @@ struct target_section
   struct bfd_section *the_bfd_section;
 
   /* The "owner" of the section.
-     It can be any unique value.  It is set by add_target_sections
-     and used by remove_target_sections.
+     
+     It is set by add_target_sections and used by remove_target_sections.
      For example, for executables it is a pointer to exec_bfd and
-     for shlibs it is the so_list pointer.  */
-  const void *owner;
+     for shlibs it is the shobj pointer.  */
+  target_section_owner owner;
 };
 
 #endif /* GDB_TARGET_SECTION_H */

base-commit: 1fa80e4c8184d87d75ff30b552cc282f5811823a
-- 
2.42.0

Re: [PATCH v3] gdb: fix owner passed to remove_target_sections in clear_solib

[Pedro Alves]

On 2023-10-20 20:08, Simon Marchi wrote:

> @@ -651,15 +651,15 @@ program_space::add_target_sections (struct objfile *objfile)
>     OWNER must be the same value passed to add_target_sections.  */
>  
>  void
> -program_space::remove_target_sections (const void *owner)
> +program_space::remove_target_sections (target_section_owner owner)
>  {
> -  gdb_assert (owner != NULL);
> +  gdb_assert (owner.v != nullptr);
>  
>    auto it = std::remove_if (m_target_sections.begin (),
>  			    m_target_sections.end (),
>  			    [&] (target_section &sect)
>  			    {
> -			      return sect.owner == owner;
> +			      return sect.owner.v == owner.v;

Pedantically, accessing a non-active union member is undefined behavior.
A.k.a., type punning via a union is valid C, but not C++, though it is a GCC
extension, and likely won't ever go away and is probably supported by all compilers
we care about, I think.  So I'm not sure we really care.  It's easy to do the punning
via memcpy, which is valid, though, maybe we should just do it.  (This should be
optimized away.)  See patch below.

>  
>    /* The "owner" of the section.
> -     It can be any unique value.  It is set by add_target_sections
> -     and used by remove_target_sections.
> +     

"git am" complains about the spurious whitespace here.

> +     It is set by add_target_sections and used by remove_target_sections.
>       For example, for executables it is a pointer to exec_bfd and
> -     for shlibs it is the so_list pointer.  */
> -  const void *owner;
> +     for shlibs it is the shobj pointer.  */
> +  target_section_owner owner;
>  };

Otherwise, this LGTM.

Pedro Alves

From a03ce3200769f73d2f46e601ad78b75617975fd1 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Fri, 20 Oct 2023 21:51:01 +0100
Subject: [PATCH] avoid type punning via union

Change-Id: I51a8d6af525eb05a86b57a3d1ec31b1b144eeb33
---
 gdb/exec.c           |  4 ++--
 gdb/maint.c          |  2 +-
 gdb/target-section.h | 16 ++++++++++++++--
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/gdb/exec.c b/gdb/exec.c
index 503dfd19792..5956012338f 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -653,13 +653,13 @@ program_space::add_target_sections (struct objfile *objfile)
 void
 program_space::remove_target_sections (target_section_owner owner)
 {
-  gdb_assert (owner.v != nullptr);
+  gdb_assert (owner.v () != nullptr);
 
   auto it = std::remove_if (m_target_sections.begin (),
 			    m_target_sections.end (),
 			    [&] (target_section &sect)
 			    {
-			      return sect.owner.v == owner.v;
+			      return sect.owner.v () == owner.v ();
 			    });
   m_target_sections.erase (it, m_target_sections.end ());
 
diff --git a/gdb/maint.c b/gdb/maint.c
index cec5128ef83..0635af3dfc4 100644
--- a/gdb/maint.c
+++ b/gdb/maint.c
@@ -509,7 +509,7 @@ maintenance_info_target_sections (const char *arg, int from_tty)
 		  (8 + digits), "",
 		  hex_string_custom (sec.addr, addr_size),
 		  hex_string_custom (sec.endaddr, addr_size),
-		  sec.owner.v);
+		  sec.owner.v ());
     }
 }
 
diff --git a/gdb/target-section.h b/gdb/target-section.h
index 386d044bfa9..1d7846ac610 100644
--- a/gdb/target-section.h
+++ b/gdb/target-section.h
@@ -28,15 +28,27 @@ struct shobj;
 
 union target_section_owner
 {
-  target_section_owner () : v (nullptr) {}
+  target_section_owner () : m_v (nullptr) {}
   target_section_owner (const bfd *bfd) : bfd (bfd) {}
   target_section_owner (const objfile *objfile) : objfile (objfile) {}
   target_section_owner (const shobj *shobj) : shobj (shobj) {}
 
+  /* Use this to access the type-erased version of the owner, for
+     comparisons, printing, etc.  We don't access the M_V member
+     directly, because pedantically it is not valid to access a
+     non-active union member.  */
+  const void *v () const
+  {
+    void *tmp;
+    memcpy (&tmp, this, sizeof (*this));
+    return tmp;
+  }
+
   const struct bfd *bfd;
   const struct objfile *objfile;
   const struct shobj *shobj;
-  const void *v;
+private:
+  const void *m_v;
 };
 
 /* Struct target_section maps address ranges to file sections.  It is

-- 
2.34.1

Re: [PATCH v3] gdb: fix owner passed to remove_target_sections in clear_solib

[Simon Marchi]

On 2023-10-20 17:15, Pedro Alves wrote:
> On 2023-10-20 20:08, Simon Marchi wrote:
> 
>> @@ -651,15 +651,15 @@ program_space::add_target_sections (struct objfile *objfile)
>>     OWNER must be the same value passed to add_target_sections.  */
>>  
>>  void
>> -program_space::remove_target_sections (const void *owner)
>> +program_space::remove_target_sections (target_section_owner owner)
>>  {
>> -  gdb_assert (owner != NULL);
>> +  gdb_assert (owner.v != nullptr);
>>  
>>    auto it = std::remove_if (m_target_sections.begin (),
>>  			    m_target_sections.end (),
>>  			    [&] (target_section &sect)
>>  			    {
>> -			      return sect.owner == owner;
>> +			      return sect.owner.v == owner.v;
> 
> Pedantically, accessing a non-active union member is undefined behavior.
> A.k.a., type punning via a union is valid C, but not C++, though it is a GCC
> extension, and likely won't ever go away and is probably supported by all compilers
> we care about, I think.  So I'm not sure we really care.  It's easy to do the punning
> via memcpy, which is valid, though, maybe we should just do it.  (This should be
> optimized away.)  See patch below.

And, would it work to do it like this, just by casting to void
(basically what we have now, but wrapped in a struct)?

  struct target_section_owner
  {
    target_section_owner () = default;
    target_section_owner (const bfd *bfd) : m_v (bfd) {}
    target_section_owner (const objfile *objfile) : m_v (objfile) {}
    target_section_owner (const shobj *shobj) : m_v (shobj) {}

    const void *v () const
    { return m_v; }

  private:
    const void *m_v = nullptr;
  };

> 
>>  
>>    /* The "owner" of the section.
>> -     It can be any unique value.  It is set by add_target_sections
>> -     and used by remove_target_sections.
>> +     
> 
> "git am" complains about the spurious whitespace here.

Will fix.

>> +     It is set by add_target_sections and used by remove_target_sections.
>>       For example, for executables it is a pointer to exec_bfd and
>> -     for shlibs it is the so_list pointer.  */
>> -  const void *owner;
>> +     for shlibs it is the shobj pointer.  */
>> +  target_section_owner owner;
>>  };
> 
> Otherwise, this LGTM.

Thanks, I'll push the patch with your addendum right now, because GDB is
quite broken otherwise.

Thanks,

Simon

Re: [PATCH v3] gdb: fix owner passed to remove_target_sections in clear_solib

[Pedro Alves]

On 2023-10-21 02:52, Simon Marchi wrote:

> And, would it work to do it like this, just by casting to void
> (basically what we have now, but wrapped in a struct)?
> 
>   struct target_section_owner
>   {
>     target_section_owner () = default;
>     target_section_owner (const bfd *bfd) : m_v (bfd) {}
>     target_section_owner (const objfile *objfile) : m_v (objfile) {}
>     target_section_owner (const shobj *shobj) : m_v (shobj) {}
> 
>     const void *v () const
>     { return m_v; }
> 
>   private:
>     const void *m_v = nullptr;
>   };

That should work.  If you add a getter for each type, like:

     const bfd *bfd () const { return (struct bfd *) m_v; }

then we concentrate the casts here instead of having each caller
cast the result of v() which is more dangerous.

With that tweak, I like it it better than the union, even.

> Thanks, I'll push the patch with your addendum right now, because GDB is
> quite broken otherwise.

OK, but could have split the fix in two -- the fix proper, and then the
make-compiler-detect-badness thing separately, with the former being the
urgent thing.  :-)

Re: [PATCH v3] gdb: fix owner passed to remove_target_sections in clear_solib

[Simon Marchi]

On 10/23/23 07:51, Pedro Alves wrote:
> On 2023-10-21 02:52, Simon Marchi wrote:
> 
>> And, would it work to do it like this, just by casting to void
>> (basically what we have now, but wrapped in a struct)?
>>
>>   struct target_section_owner
>>   {
>>     target_section_owner () = default;
>>     target_section_owner (const bfd *bfd) : m_v (bfd) {}
>>     target_section_owner (const objfile *objfile) : m_v (objfile) {}
>>     target_section_owner (const shobj *shobj) : m_v (shobj) {}
>>
>>     const void *v () const
>>     { return m_v; }
>>
>>   private:
>>     const void *m_v = nullptr;
>>   };
> 
> That should work.  If you add a getter for each type, like:
> 
>      const bfd *bfd () const { return (struct bfd *) m_v; }
> 
> then we concentrate the casts here instead of having each caller
> cast the result of v() which is more dangerous.

We never need to read the pointer as a typed pointer, we really only
use the value of the pointer as an identity.  So these getters aren't
needed.

Simon

Re: [PATCH v3] gdb: fix owner passed to remove_target_sections in clear_solib

[Pedro Alves]

On 2023-10-23 14:35, Simon Marchi wrote:
> On 10/23/23 07:51, Pedro Alves wrote:
>> On 2023-10-21 02:52, Simon Marchi wrote:
>>
>>> And, would it work to do it like this, just by casting to void
>>> (basically what we have now, but wrapped in a struct)?
>>>
>>>   struct target_section_owner
>>>   {
>>>     target_section_owner () = default;
>>>     target_section_owner (const bfd *bfd) : m_v (bfd) {}
>>>     target_section_owner (const objfile *objfile) : m_v (objfile) {}
>>>     target_section_owner (const shobj *shobj) : m_v (shobj) {}
>>>
>>>     const void *v () const
>>>     { return m_v; }
>>>
>>>   private:
>>>     const void *m_v = nullptr;
>>>   };
>>
>> That should work.  If you add a getter for each type, like:
>>
>>      const bfd *bfd () const { return (struct bfd *) m_v; }
>>
>> then we concentrate the casts here instead of having each caller
>> cast the result of v() which is more dangerous.
> 
> We never need to read the pointer as a typed pointer, we really only
> use the value of the pointer as an identity.  So these getters aren't
> needed.

Oh.  In that case there was really no point to the union.  +1 for the above
code.