Hi Aaron, On Thu, May 29, 2025 at 05:44:32PM -0400, Aaron Merey wrote: > eu_tfind is used to facilitate lazy loading throughout libdw. > If a result is not found via eu_tfind, work is done to load > the result and cache it in an eu_search_tree. > > Some calls to eu_tfind allow for TOCTOU bugs. Multiple threads > might race to call eu_tfind on some result that hasn't yet been > cached. Multiple threads may then attempt to load the result > which might cause an unnecessary amount of memory to be allocated. > Additionally this memory may not get released when the associated > libdw data structure is freed. > > Fix this by adding additional locking to ensure that only one > thread performs lazy loading. > > One approach used in this patch is to preserve calls to eu_tfind > without additional locking, but when the result isn't found then > a lock is then used to synchronize access to the lazy loading code. > An extra eu_tfind call has been added at the start of these critical > section to synchronize verification that lazy loading should proceed. > > Another approach used is to simply synchronize entire calls to > functions where lazy loading via eu_tfind might occur (__libdw_find_fde > and __libdw_intern_expression). In this case, new _nolock variants of > the eu_t* functions are used to avoid unnecessary double locking. > > lib/ > * eu-search.c: Add eu_tsearch_nolock, eu_tfind_nolock and > eu_tdelete_nolock functions. > * eu-search.h: Ditto. > > libdw/ > * cfi.h (struct Dwarf_CFI_s): Declare new mutex. > * dwarf_begin_elf.c (valid_p): Initialize all locks for fake CUs. > * dwarf_cfi_addrframe.c (dwarf_cfi_addrframe): Place lock around > __libdw_find_fde. > * dwarf_end.c (cu_free): Deallocate all locks unconditionally, > whether or not the CU is fake. > * dwarf_frame_cfa.c (dwarf_frame_cfa): Place lock around > __libdw_intern_expression. > * dwarf_frame_register.c (dwarf_frame_register): Ditto. > * dwarf_getcfi.c (dwarf_getcfi): Initialize cfi lock. > * dwarf_getlocation.c (is_constant_offset): Synchronize access > to lazy loading section. > (getlocation): Place lock around __libdw_intern_expression. > * dwarf_getmacros.c (cache_op_table): Synchronize access to lazy > loading section. > * frame-cache.c (__libdw_destroy_frame_cache): Free Dwarf_CFI > mutex. > * libdwP.h (struct Dwarf): Update macro_lock comment. > (struct Dwarf_CU): Declare new mutex. > libdw_findcu.c (__libdw_intern_next_unit): Initialize > intern_lock. > (__libdw_findcu): Adjust locking so that the first eu_tfind > can be done without extra lock overhead. > > Signed-off-by: Aaron Merey <ame...@redhat.com> > > v3: > Implement _nolock functions as inline functions and move to eu-search.h. > Simplify locking in is_constant_offset. > In __libdw_findcu use eu_tfind instead of eu_tfind_nolock.
Thanks, this addresses all comments I had. I'll see if my suggestion to make __libdw_fde_by_offset static makes sense as a followup patch. Thanks, Mark
