[PATCH v3] libdw: Fix eu_search_tree TOCTOU bugs

[Mark Wielaard]

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 <amerey@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