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

[Simon Marchi <simark@simark.ca>]

On 7/6/26 1:09 PM, Lancelot SIX wrote:
> 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.

Ok, thanks for the analysis.  I didn't spot this because I didn't fully
test each commit.  The issue goes away at the main multiple solib ops
patch, because it adds the behavior of removing all the solibs owned by
a given solib_ops when removing that solib_ops.  Having known this, I
would perhaps have kept this cleanup for later.  But now, the subsequent
patches are based on this one, and removing it is not trivial and risks
introducing more issues.  So the fix I propose is:

 - Move the remove_solib patch before this one, this is
   trivial.  Make remove_solib public from the start (it would become
   public in the last patch anyway).
 - Change this patch to call remove_solib on the solibs owned by the
   rocm_solib_ops.  This mimics what the last patch does.

The end result of the series is the same, it just changes the
intermediary states.  I would send a new version of the series with
those changes.

Simon