Re: [PATCH v2 5/10] gdb/solib-rocm: move per-inferior data to rocm_solib_ops

[Lancelot SIX <lancelot.six@amd.com>]

On Mon, Jun 08, 2026 at 04:00:29PM -0400, Simon Marchi wrote:
> Move the data currently stored in an inferior registry directly into
> rocm_solib_ops.  This changes the storage of this data from "per
> inferior" to "per program-space", but it should be equivalent, since
> there is usually a 1:1 mapping between those two.  The only time there
> isn't a 1:1 mapping is during a vfork, before the exec/exit, but no ROCm
> activity happens during that slice of time.
> 
> Function rocm_update_solib_list becomes
> rocm_solib_ops::update_solib_list.
> 
> Function rocm_bfd_iovec_open becomes rocm_solib_ops::bfd_iovec_open.
> 
> Most of the changes consist of accessing the fields directly, instead of
> going through the solib_info object.
> 
> The rocm_solib_ops constructor has to take an inferior as a parameter,
> rather than a program_space, because of the fd cache.
> 

Hi Simon,

It seems that this patch introduces a use-after-free issue when handling
the "detach-on-fork on", "follow-fork-mode child" case (I see this with
the gdb.rocm/fork-exec-gpu-to-non-gpu.exp testcase with ASAN enabled).

When doing the case where we follow the child, we reach
rocm_solib_target_inferior_forked:

  if (detach_on_fork && follow_child && fork_kind == TARGET_WAITKIND_FORKED)
    {
      auto rocm_ops_holder = child_inf->pspace->release_solib_ops ();
      auto rocm_ops
        = gdb::checked_static_cast<rocm_solib_ops *> (rocm_ops_holder.get ());
      child_inf->pspace->set_solib_ops (rocm_ops->release_host_ops ());
    }

On exit here of this block, the rocm_ops is deleted (calls
rocm_solib_ops::~rocm_solib_ops).

However, at this stage, if there are still solibs which were opened by
rocm_solib_ops, the next call to update_solib_list will lead to deleting
the solib which do not exist anymore (so far so good).  Any file
rocm-solib still open at this point will eventually be closed when we
close the associated BFD.  This calls into rocm_solib_fd_cache::close,
which tries to access the rocm_solib_fd_cache::m_cache field.  However,
this is a reference to a rocm_solib_ops::m_fd_cache, which is now gone,
leading to a use after free.

This was not an issue until this patch because the fd_cache was owned by
the per-inferior rocm_solib_data. which is not going anywhere during the fork.

Best,
Lancelot.

> Change-Id: I386139008b6f0eca1d897fc35be18faa9d5a104e
> ---
>  gdb/solib-rocm.c | 90 ++++++++++++++++++------------------------------
>  1 file changed, 33 insertions(+), 57 deletions(-)
> 
> diff --git a/gdb/solib-rocm.c b/gdb/solib-rocm.c
> index 4032ada28a0f..618f4f5e7905 100644
> --- a/gdb/solib-rocm.c
> +++ b/gdb/solib-rocm.c
> @@ -181,30 +181,15 @@ struct rocm_so
>    lm_info_svr4_up lm_info;
>  };
>  
> -struct solib_info
> -{
> -  explicit solib_info (inferior *inf)
> -    : fd_cache (inf)
> -  {};
> -
> -  /* List of code objects loaded into the inferior.  */
> -  std::vector<rocm_so> solib_list;
> -
> -  /* Cache of opened FD in the inferior.  */
> -  rocm_solib_fd_cache fd_cache;
> -};
> -
> -/* Per-inferior data key.  */
> -static const registry<inferior>::key<solib_info> rocm_solib_data;
> -
>  /* solib_ops for ROCm systems.  */
>  
>  struct rocm_solib_ops : public solib_ops
>  {
>    /* HOST_OPS is the host solib_ops that rocm_solib_ops hijacks / wraps,
>       in order to provide support for ROCm code objects.  */
> -  explicit rocm_solib_ops (program_space *pspace, solib_ops_up host_ops)
> -    : solib_ops (pspace), m_host_ops (std::move (host_ops))
> +  explicit rocm_solib_ops (inferior *inf, solib_ops_up host_ops)
> +    : solib_ops (inf->pspace), m_host_ops (std::move (host_ops)),
> +      m_fd_cache (inf)
>    {
>      gdb_assert (m_host_ops != nullptr);
>      gdb_assert (dynamic_cast<rocm_solib_ops *> (m_host_ops.get ()) == nullptr);
> @@ -221,6 +206,10 @@ struct rocm_solib_ops : public solib_ops
>    void relocate_section_addresses (solib &so, target_section *) const override;
>    void handle_event () override;
>  
> +  /* This needs to be public because it is called from
> +     rocm_solib_target_inferior_created.  */
> +  void update_solib_list ();
> +
>    /* Implement the following methods just to forward the calls to the host
>       solib_ops.  We currently need to implement all the methods that
>       svr4_solib_ops implements.  */
> @@ -271,18 +260,17 @@ struct rocm_solib_ops : public solib_ops
>  private:
>    owning_intrusive_list<solib>
>    solibs_from_rocm_sos (const std::vector<rocm_so> &sos);
> +  gdb_bfd_iovec_base *bfd_iovec_open (bfd *abfd, inferior *inferior);
>  
>    solib_ops_up m_host_ops;
> +
> +  /* List of code objects loaded into the inferior.  */
> +  std::vector<rocm_so> m_solib_list;
> +
> +  /* Cache of opened FD in the inferior.  */
> +  rocm_solib_fd_cache m_fd_cache;
>  };
>  
> -/* Fetch the solib_info data for INF.  */
> -
> -static struct solib_info *
> -get_solib_info (inferior *inf)
> -{
> -  return &rocm_solib_data.try_emplace (inf, inf);
> -}
> -
>  /* Relocate section addresses.  */
>  
>  void
> @@ -300,8 +288,6 @@ rocm_solib_ops::relocate_section_addresses (solib &so,
>    sec->endaddr = sec->endaddr + li->l_addr;
>  }
>  
> -static void rocm_update_solib_list ();
> -
>  void
>  rocm_solib_ops::handle_event ()
>  {
> @@ -313,7 +299,7 @@ rocm_solib_ops::handle_event ()
>       previously loaded).  */
>    m_host_ops->handle_event ();
>  
> -  rocm_update_solib_list ();
> +  this->update_solib_list ();
>  }
>  
>  /* Create solib objects from rocm_so objects in SOS.  */
> @@ -340,12 +326,10 @@ rocm_solib_ops::current_sos ()
>    owning_intrusive_list<solib> sos = m_host_ops->current_sos ();
>  
>    /* Then, the device-side shared library list.  */
> -  std::vector<rocm_so> &dev_sos = get_solib_info (current_inferior ())->solib_list;
> -
> -  if (dev_sos.empty ())
> +  if (m_solib_list.empty ())
>      return sos;
>  
> -  owning_intrusive_list<solib> dev_solibs = solibs_from_rocm_sos (dev_sos);
> +  owning_intrusive_list<solib> dev_solibs = solibs_from_rocm_sos (m_solib_list);
>  
>    if (sos.empty ())
>      return dev_solibs;
> @@ -525,8 +509,8 @@ rocm_code_object_stream_memory::read (bfd *, void *buf, file_ptr size,
>  
>  } /* anonymous namespace */
>  
> -static gdb_bfd_iovec_base *
> -rocm_bfd_iovec_open (bfd *abfd, inferior *inferior)
> +gdb_bfd_iovec_base *
> +rocm_solib_ops::bfd_iovec_open (bfd *abfd, inferior *inferior)
>  {
>    std::string_view uri (bfd_get_filename (abfd));
>    std::string_view protocol_delim = "://";
> @@ -635,10 +619,9 @@ rocm_bfd_iovec_open (bfd *abfd, inferior *inferior)
>  	      && decoded_path[3] == '/')
>  	    decoded_path.erase (0, 1);
>  
> -	  auto info = get_solib_info (inferior);
>  	  fileio_error target_errno;
>  	  rocm_solib_fd_cache::cached_target_fd fd
> -	    = info->fd_cache.open (decoded_path, &target_errno);
> +	    = m_fd_cache.open (decoded_path, &target_errno);
>  
>  	  if (fd.fd () == target_fd::INVALID)
>  	    {
> @@ -698,9 +681,9 @@ rocm_solib_ops::bfd_open (const char *pathname)
>    if (strstr (pathname, "://") == nullptr)
>      return m_host_ops->bfd_open (pathname);
>  
> -  auto open = [] (bfd *nbfd) -> gdb_bfd_iovec_base *
> +  auto open = [this] (bfd *nbfd)
>    {
> -    return rocm_bfd_iovec_open (nbfd, current_inferior ());
> +    return this->bfd_iovec_open (nbfd, current_inferior ());
>    };
>  
>    gdb_bfd_ref_ptr abfd = gdb_bfd_openr_iovec (pathname, "elf64-amdgcn", open);
> @@ -782,13 +765,12 @@ rocm_solib_ops::bfd_open (const char *pathname)
>  void
>  rocm_solib_ops::create_inferior_hook (int from_tty)
>  {
> -  get_solib_info (current_inferior ())->solib_list.clear ();
> -
> +  m_solib_list.clear ();
>    m_host_ops->create_inferior_hook (from_tty);
>  }
>  
> -static void
> -rocm_update_solib_list ()
> +void
> +rocm_solib_ops::update_solib_list ()
>  {
>    inferior *inf = current_inferior ();
>  
> @@ -796,10 +778,7 @@ rocm_update_solib_list ()
>    if (process_id.handle == AMD_DBGAPI_PROCESS_NONE.handle)
>      return;
>  
> -  solib_info *info = get_solib_info (inf);
> -
> -  info->solib_list.clear ();
> -  std::vector<rocm_so> &sos = info->solib_list;
> +  m_solib_list.clear ();
>  
>    amd_dbgapi_code_object_id_t *code_object_list;
>    size_t count;
> @@ -846,7 +825,8 @@ rocm_update_solib_list ()
>        std::string unique_name
>  	= string_printf ("code_object_%ld", code_object_list[i].handle);
>  
> -      sos.emplace_back (uri_bytes, std::move (unique_name), std::move (li));
> +      m_solib_list.emplace_back (uri_bytes, std::move (unique_name),
> +				 std::move (li));
>      }
>  }
>  
> @@ -857,17 +837,15 @@ rocm_solib_target_inferior_created (inferior *inf)
>    if (inf->vfork_parent != nullptr)
>      return;
>  
> -  get_solib_info (inf)->solib_list.clear ();
> -
>    auto prev_ops = inf->pspace->release_solib_ops ();
> -  auto rocm_ops
> -    = std::make_unique<rocm_solib_ops> (inf->pspace, std::move (prev_ops));
> +  auto rocm_ops = std::make_unique<rocm_solib_ops> (inf, std::move (prev_ops));
>    inf->pspace->set_solib_ops (std::move (rocm_ops));
>  
> -  rocm_update_solib_list ();
> +  gdb::checked_static_cast<rocm_solib_ops *>
> +    (inf->pspace->solib_ops ())->update_solib_list ();
>  
>    /* Force GDB to reload the solibs.  */
> -  current_inferior ()->pspace->clear_solib_cache ();
> +  inf->pspace->clear_solib_cache ();
>    solib_add (nullptr, 0, auto_solib_add);
>  }
>  
> @@ -882,10 +860,8 @@ rocm_solib_target_inferior_execd (inferior *exec_inf, inferior *follow_inf)
>    auto pspace = follow_inf->pspace;
>    auto prev_ops = pspace->release_solib_ops ();
>    auto rocm_ops
> -    = std::make_unique<rocm_solib_ops> (pspace, std::move (prev_ops));
> +    = std::make_unique<rocm_solib_ops> (follow_inf, std::move (prev_ops));
>    pspace->set_solib_ops (std::move (rocm_ops));
> -
> -  get_solib_info (exec_inf)->solib_list.clear ();
>  }
>  
>  static void
> -- 
> 2.54.0