Hi Mark, On Wed, Oct 28, 2026 at 9:30 AM Mark Wielaard <[email protected]> wrote: > > 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.
This should be ok. The dummy's initial_state stays NULL which execute_cfi avoids dereferencing. > > 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 ? Agreed. I'll add this to v2. > > > 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. I suggest we keep using cache->lock, which also protects the initialization of cache->ebl. If this becomes a performance bottleneck in real workloads, we can take a different approach. Aaron > > Cheers, > > Mark >
