Hi Mark, On Sat, Oct 17, 2026 at 5:39 PM Mark Wielaard <[email protected]> wrote: > > 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?
I believe it works properly as is but to be safe I added the local dwp_dwarf variable you recommended. Aaron > > 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 >
