Hi Aaron, On Tue, 2026-10-27 at 20:34 -0400, Aaron Merey wrote: > Concurrent calls to dwarf_cfi_addrframe are unsynchronized and can > result in a CIE's initial state being initialized multiple times across > threads or a thread reading a partially initialized initial state. > > Fix this by checking cie->initial_state atomically and if it isn't > initialized, acquire cache->lock before performing the initialization.
OK, so the struct dwarf_cie defined in libdw/cfi.h is a separate (internal only) data type from the public Dwarf_CIE defined in libdw/libdw. The only function creating struct dwarf_cies is intern_new_cie which initializes the initial_state field to NULL (called from __libdw_intern_cie and __libdw_find_cie). There is the Dummy CIE for backend's instructions in cie_cache_initial_state which is never fully initialized (hopefully fine?) before calling cfi_execute on it. The initial_state field is a pointer to a Dwarf_Frame, an opaque structure which holds instructions for unwinding frame at a particular PC location. It is set only in cie_cache_initial_state (after running execute_cfi on it). It is destroyed by __libdw_destroy_frame_cache called from either dwarf_end (for the Dwarf_CFI gotten through dwarf_getcfi) or dwarf_cfi_end (for Dwarf_CFI gotten through dwarf_getcfi_elf). The cie->initial_state is directly used in execute_cfi called by __libdw_frame_at_address and cie_cache_initial_state itself. __libdw_frame_at_address makes sure to call cie_cache_initial_state. Some of the above might be good to include in a comment for initial_state in cfi.h ? > Signed-off-by: Aaron Merey <[email protected]> > --- > libdw/cfi.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/libdw/cfi.c b/libdw/cfi.c > index a9607246..4710b474 100644 > --- a/libdw/cfi.c > +++ b/libdw/cfi.c > @@ -456,9 +456,14 @@ cie_cache_initial_state (Dwarf_CFI *cache, struct > dwarf_cie *cie) > { > int result = DWARF_E_NOERROR; > > - if (likely (cie->initial_state != NULL)) > + if (likely (atomic_load_acquire (&cie->initial_state) != NULL)) > return result; OK, if not NULL then fully initialized already and we can use it. > + mutex_lock (cache->lock); > + > + if (cie->initial_state != NULL) > + goto out; OK, take the lock, recheck if nobody raced passed us, goto out immediately if already initialized. > /* This CIE has not been used before. Play out its initial > instructions and cache the initial state that results. > First we'll let the backend fill in the default initial > @@ -477,11 +482,17 @@ cie_cache_initial_state (Dwarf_CFI *cache, struct > dwarf_cie *cie) > /* Fetch the ABI's default CFI program. */ > if (likely (cache->ebl != (void *) -1l) > && unlikely (ebl_abi_cfi (cache->ebl, &abi_info) < 0)) > - return DWARF_E_UNKNOWN_ERROR; > + { > + result = DWARF_E_UNKNOWN_ERROR; > + goto out; > + } OK, on error goto out to drop the lock (and return result). > Dwarf_Frame *cie_fs = calloc (1, sizeof (Dwarf_Frame)); > if (unlikely (cie_fs == NULL)) > - return DWARF_E_NOMEM; > + { > + result = DWARF_E_NOMEM; > + goto out; > + } OK, likewise. > /* If the default state of any register is not "undefined" > (i.e. call-clobbered), then the backend supplies instructions > @@ -513,9 +524,11 @@ cie_cache_initial_state (Dwarf_CFI *cache, struct > dwarf_cie *cie) > /* Now we have the initial state of things that all > FDEs using this CIE will start from. */ > cie_fs->cache = cache; > - cie->initial_state = cie_fs; > + atomic_store_release (&cie->initial_state, cie_fs); > } Right use atomic store so any code outside the lock sees it atomically. > +out: > + mutex_unlock (cache->lock); > return result; > } > OK, and release the lock. My only concern would be that the lock could be held for the entire duration of (multiple) execute_cfi calls. So maybe a lock on the dwarf_cie itself instead of the whole cache, might be better for less contention. But that would make the dwarf_cie bigger (and introduce a lot of locks). So maybe the cache lock is a better choice. Cheers, Mark
