Hi Aaron, On Wed, Sep 30, 2026 at 03:06:30PM -0400, Aaron Merey wrote: > dwp_dwarf and dwp_fd are set in libdw_find_split_unit.c:try_dwp_file. > This function is called with a CU's split_lock held, but a data race and > memory leak can occur if two separate CUs attempt to set dwp_dwarf and > dwp_fd concurrently. > > Fix this by adding a new Dwarf mutex dwp_lock that protects the lazy > loading of dwp_dwarf and dwp_fd. Distinct CUs will now synchronize on > the same Dwarf-wide lock when attempting to set dwp_dwarf and dwp_fd. > Reads of dwp_dwarf and dwp_fd are not protected by a lock since all > reads (besides those in dwarf_end) occur only after the fields are > confirmed to have been lazy loaded. > > This patch adds dwp_lock instead of using the existing dwarf_lock > in order to avoid creating conditions for a lock ordering problem > where this code path acquires split_lock then dwarf_lock but other > code paths might reach this one with dwarf_lock already held.
dwp_fd and dwp_dwarf initialized at creation time in dwarf_begin_elf (to -1 and NULL). The are only read and set in libdw_find_split_unit. And then possibly used to close and end in dwarf_end. No locking needed in begin and end since those cannot/shouldn't be called concurrently with other functions on the Dwarf. > Signed-off-by: Aaron Merey <[email protected]> > --- > libdw/dwarf_begin_elf.c | 1 + > libdw/dwarf_end.c | 1 + > libdw/libdwP.h | 3 +++ > libdw/libdw_find_split_unit.c | 5 +++++ > 4 files changed, 10 insertions(+) > > diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c > index bc236bf9..e157d103 100644 > --- a/libdw/dwarf_begin_elf.c > +++ b/libdw/dwarf_begin_elf.c > @@ -448,6 +448,7 @@ valid_p (Dwarf *result) > /* Initialize locks and search_trees. */ > mutex_init (result->dwarf_lock); > mutex_init (result->macro_lock); > + mutex_init (result->dwp_lock); > eu_search_tree_init (&result->cu_tree); > eu_search_tree_init (&result->tu_tree); > eu_search_tree_init (&result->split_tree); > diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c > index fa27b19e..4a65a35c 100644 > --- a/libdw/dwarf_end.c > +++ b/libdw/dwarf_end.c > @@ -135,6 +135,7 @@ dwarf_end (Dwarf *dwarf) > pthread_rwlock_destroy (&dwarf->mem_rwl); > mutex_fini (dwarf->dwarf_lock); > mutex_fini (dwarf->macro_lock); > + mutex_fini (dwarf->dwp_lock); > > /* Free the pubnames helper structure. */ > free (dwarf->pubnames_sets); > diff --git a/libdw/libdwP.h b/libdw/libdwP.h > index 9e7e2339..3e0c2edc 100644 > --- a/libdw/libdwP.h > +++ b/libdw/libdwP.h > @@ -265,6 +265,9 @@ struct Dwarf > /* Synchronize access to dwarf_macro_getsrcfiles and cache_op_table. */ > mutex_define(, macro_lock); > > + /* Synchronize lazy-loading of dwp_dwarf and dwp_fd in try_dwp_file. */ > + mutex_define(, dwp_lock); > + > /* Internal memory handling. This is basically a simplified thread-local > reimplementation of obstacks. Unfortunately the standard obstack > implementation is not usable in libraries. */ OK, define, init and fini new lock. > diff --git a/libdw/libdw_find_split_unit.c b/libdw/libdw_find_split_unit.c > index 3ccf8441..9a20ae25 100644 > --- a/libdw/libdw_find_split_unit.c > +++ b/libdw/libdw_find_split_unit.c > @@ -90,6 +90,8 @@ try_split_file (Dwarf_CU *cu, const char *dwo_path) > static void > try_dwp_file (Dwarf_CU *cu) > { > + mutex_lock (cu->dbg->dwp_lock); > + > if (cu->dbg->dwp_dwarf == NULL) > { > uint8_t is_dwp; > @@ -122,6 +124,7 @@ try_dwp_file (Dwarf_CU *cu) > if (dwp_path == NULL) > { > __libdw_seterrno (DWARF_E_NOMEM); > + mutex_unlock (cu->dbg->dwp_lock); > return; > } > memcpy (dwp_path, cu->dbg->elfpath, elfpath_len); > @@ -150,6 +153,8 @@ try_dwp_file (Dwarf_CU *cu) > cu->dbg->dwp_dwarf = (Dwarf *) -1; > } > > + mutex_unlock (cu->dbg->dwp_lock); > + > if (cu->dbg->dwp_dwarf != (Dwarf *) -1) > { > Dwarf_CU *split = __libdw_dwp_findcu_id (cu->dbg->dwp_dwarf, I would probably be paranoid and don't drop the dwp_lock early. But this is probably correct given the like just before the dropping of the lock is: if (cu->dbg->dwp_dwarf == NULL) cu->dbg->dwp_dwarf = (Dwarf *) -1; So a) the only possible values are a valid pointer or -1. And b) even when another thread now takes the lock it will never see dwp_dwarf as NULL so it will never change the field again? Is that guaranteed? Even when the thread switches cpus right after the unlock? Or should the mutux lock be extended? Or should we use a local variable set inside the lock and checked after the unlock? aka Dwarf *dwp_dwarf = cu->dbg->dwp_dwarf; mutex_unlock (cu->dbg->dwp_lock); if (dwp_dwarf != (Dwarf *) -1) Dwarf_CU *split = __libdw_dwp_findcu_id (dwp_dwarf, Thinking about these memory models does make my head hurt :) And I might be too paranoid. Cheers, Mark
