Hi Trithem, On Tue, Sep 8, 2026 at 11:24 AM toufik TOUFIK <[email protected]> wrote: > > The finder raced while retaining an Elf reference, updating ref_count > without holding Elf::lock. > > The finder also released elftab_lock before consuming the cache entry, > allowing a concurrent cache modification to invalidate the entry. > > Keep the tracker read lock while copying and retaining the cache entry, > and synchronize Elf retention with Elf::lock. > > Add a regression test covering concurrent finder/finder retention and > finder/replacer cache access when thread-safety support is enabled. > > Signed-off-by: Trithem <[email protected]> > --- > libdwfl_stacktrace/dwflst_tracker_find_elf.c | 26 +- > libelf/libelfP.h | 13 + > tests/Makefile.am | 19 +- > tests/dwflst-tracker-concurrent.c | 306 +++++++++++++++++++ > tests/run-dwflst-tracker-concurrent.sh | 24 ++ > 5 files changed, 374 insertions(+), 14 deletions(-) > create mode 100644 tests/dwflst-tracker-concurrent.c > create mode 100755 tests/run-dwflst-tracker-concurrent.sh > > diff --git a/libdwfl_stacktrace/dwflst_tracker_find_elf.c > b/libdwfl_stacktrace/dwflst_tracker_find_elf.c > index 56e87787..cc156cfc 100644 > --- a/libdwfl_stacktrace/dwflst_tracker_find_elf.c > +++ b/libdwfl_stacktrace/dwflst_tracker_find_elf.c > @@ -33,8 +33,6 @@ > > #include <sys/stat.h> > #include "../libelf/libelfP.h" > -/* XXX: Private header needed for Elf * ref_count field. */ > -/* TODO: Consider dup_elf() rather than direct ref_count access. */ > > #include "libdwfl_stacktraceP.h" > > @@ -83,7 +81,6 @@ dwflst_tracker_find_cached_elf (Dwflst_Process_Tracker > *tracker, > > rwlock_rdlock(tracker->elftab_lock); > ent = dwflst_tracker_elftab_find(&tracker->elftab, hval); > - rwlock_unlock(tracker->elftab_lock); > > /* Guard against collisions. > TODO: Need proper chaining, dynamicsizehash_concurrent isn't really > @@ -92,18 +89,23 @@ dwflst_tracker_find_cached_elf (Dwflst_Process_Tracker > *tracker, > rc = fstat(ent->fd, &sb); > if (rc < 0 || strcmp (module_name, ent->module_name) != 0 > || ent->dev != sb.st_dev || ent->ino != sb.st_ino) > - return -1; > + { > + rwlock_unlock(tracker->elftab_lock); > + return -1; > + } > > /* Verify that ent->fd has not been updated: */ > if (rc < 0 || ent->dev != sb.st_dev || ent->ino != sb.st_ino > || ent->last_mtime != sb.st_mtime) > - return -1; > - > - if (ent->elf != NULL) > - ent->elf->ref_count++; > - *elfp = ent->elf; > - *file_name = strdup(ent->module_name); > - return ent->fd; > + { > + rwlock_unlock(tracker->elftab_lock); > + return -1; > + } > + *elfp = __libelf_keep (ent->elf); > + *file_name = strdup (ent->module_name); > + int fd = ent->fd; > + rwlock_unlock(tracker->elftab_lock); > + return fd; > } > INTDEF(dwflst_tracker_find_cached_elf) > > @@ -170,7 +172,7 @@ dwflst_tracker_cache_elf (Dwflst_Process_Tracker *tracker, > elf_end(ent->elf); > } > if (elf != NULL && ent->elf != elf) > - elf->ref_count++; > + __libelf_keep (elf); > ent->elf = elf; > ent->fd = fd; > if (rc == 0) > diff --git a/libelf/libelfP.h b/libelf/libelfP.h > index 11ef5989..2403d796 100644 > --- a/libelf/libelfP.h > +++ b/libelf/libelfP.h > @@ -486,6 +486,19 @@ extern int __elf64_updatefile (Elf *elf, int change_bo, > size_t shnum) > internal_function; > > > +static inline Elf * > +__libelf_keep (Elf *elf) > +{ > + if (elf == NULL) > + return NULL; > + > + rwlock_wrlock (elf->lock); > + elf->ref_count++; > + rwlock_unlock (elf->lock); > + > + return elf; > +} > + > /* Alias for exported functions to avoid PLT entries, and > rdlock/wrlock variants of these functions. */ > extern int __elf_end_internal (Elf *__elf) attribute_hidden; > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 137e9616..ef0f473d 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -748,8 +748,8 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \ > run-eu-search-cfi.sh run-eu-search-macros.sh \ > run-eu-search-lines.sh run-eu-search-die.sh \ > run-dwelf-dwarf-debug-sup.sh \ > - testfile-dwarf5-ref-sup.bz2 testfile-dwarf5.sup.bz2 > - > + testfile-dwarf5-ref-sup.bz2 testfile-dwarf5.sup.bz2 \ > + run-dwflst-tracker-concurrent.sh > > if USE_HELGRIND > valgrind_cmd=valgrind -q --tool=helgrind --error-exitcode=1 --track-fds=yes \ > @@ -808,6 +808,21 @@ endif > libebl = ../libebl/libebl.a ../backends/libebl_backends.a ../libcpu/libcpu.a > libeu = ../lib/libeu.a > > +if USE_LOCKS > + > +check_PROGRAMS += dwflst-tracker-concurrent > + > +TESTS += run-dwflst-tracker-concurrent.sh > + > +dwflst_tracker_concurrent_CPPFLAGS = $(AM_CPPFLAGS) \ > + -I$(top_srcdir)/libdwfl_stacktrace > + > +dwflst_tracker_concurrent_LDFLAGS = -pthread $(AM_LDFLAGS) > + > +dwflst_tracker_concurrent_LDADD = $(libdw) $(libelf) > + > +endif
Thanks for the patch, overall it looks good. Instead of gating the new tests on USE_LOCKS can you copy the approach used by the run-eu-search-*.sh tests? These test binaries are bulit and run unconditionally and use the check_thread_safety_enabled procedure (from thread-safety-subr.sh) to check at test runtime whether thread safety is enabled and trigger a SKIP result if not. As written here the new test will not appear in the make check results. Aaron > + > arextract_LDADD = $(libelf) > arsymtest_LDADD = $(libelf) > ar_extract_ar_LDADD = $(libelf) > diff --git a/tests/dwflst-tracker-concurrent.c > b/tests/dwflst-tracker-concurrent.c > new file mode 100644 > index 00000000..c1d0b9df > --- /dev/null > +++ b/tests/dwflst-tracker-concurrent.c > @@ -0,0 +1,306 @@ > +/* Copyright (C) 2026 Trithem. > + > + Test concurrent libdwfl_stacktrace ELF tracker/cache handling. > + This file is part of elfutils. > + > + This file is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as > + published by the Free Software Foundation; either version 3 of > + the License, or (at your option) any later version. > + > + elfutils is distributed in the hope that it will be useful, but > + WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > + <http://www.gnu.org/licenses/>. */ > + > +#include <config.h> > + > +#include <errno.h> > +#include <fcntl.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <unistd.h> > +#include <pthread.h> > + > +#include <elf.h> > +#include <libelf.h> > +#include <libdw.h> > +#include <libdwfl.h> > +#include <libdwfl_stacktrace.h> > + > +static const Dwfl_Callbacks callbacks = { > + .find_elf = NULL, > + .find_debuginfo = NULL, > + .section_address = NULL, > + .debuginfo_path = NULL > +}; > + > +struct thread_context > +{ > + Dwflst_Process_Tracker *tracker; > + const char *module_name; > + int successes; > +}; > + > +static void * > +thread_work (void *arg) > +{ > + struct thread_context *ctx = arg; > + > + for (int i = 0; i < 2000; i++) > + { > + char *file_name = NULL; > + Elf *elf = NULL; > + > + int fd = dwflst_tracker_find_cached_elf > + (ctx->tracker, ctx->module_name, ctx->module_name, > + &file_name, &elf); > + > + if (fd < 0 || elf == NULL) > + return NULL; > + > + free (file_name); > + elf_end (elf); > + ctx->successes++; > + } > + > + return NULL; > +} > + > +struct replacer_context > +{ > + Dwflst_Process_Tracker *tracker; > + const char *module_name; > + int fd; > + int successes; > +}; > + > +static void * > +thread_replace (void *arg) > +{ > + struct replacer_context *ctx = arg; > + > + for (int i = 0; i < 1000; i++) > + { > + Elf *new_elf = elf_begin (ctx->fd, ELF_C_READ, NULL); > + > + if (new_elf == NULL) > + return NULL; > + > + if (!dwflst_tracker_cache_elf (ctx->tracker, > + ctx->module_name, > + ctx->module_name, > + new_elf, > + ctx->fd)) > + { > + elf_end (new_elf); > + return NULL; > + } > + > + elf_end (new_elf); > + ctx->successes++; > + } > + > + return NULL; > +} > + > +int > +main (int argc, char **argv) > +{ > + if (argc != 2) > + { > + fprintf (stderr, "Usage: %s ELF\n", argv[0]); > + return 1; > + } > + > + if (elf_version (EV_CURRENT) == EV_NONE) > + { > + fprintf (stderr, "elf_version: %s\n", elf_errmsg (-1)); > + return 1; > + } > + > + int fd = open (argv[1], O_RDONLY); > + if (fd < 0) > + { > + fprintf (stderr, "open: %s\n", strerror (errno)); > + return 1; > + } > + > + Elf *elf = elf_begin (fd, ELF_C_READ, NULL); > + if (elf == NULL) > + { > + fprintf (stderr, "elf_begin: %s\n", elf_errmsg (-1)); > + close (fd); > + return 1; > + } > + > + Dwflst_Process_Tracker *tracker = dwflst_tracker_begin (&callbacks); > + if (tracker == NULL) > + { > + fprintf (stderr, "dwflst_tracker_begin failed\n"); > + elf_end (elf); > + close (fd); > + return 1; > + } > + > + if (!dwflst_tracker_cache_elf (tracker, argv[1], argv[1], elf, fd)) > + { > + fprintf (stderr, "dwflst_tracker_cache_elf failed\n"); > + dwflst_tracker_end (tracker); > + elf_end (elf); > + close (fd); > + return 1; > + } > + > + elf_end (elf); > + > + char *found_file_name = NULL; > + Elf *found_elf = NULL; > + > + int found_fd = dwflst_tracker_find_cached_elf > + (tracker, argv[1], argv[1], &found_file_name, &found_elf); > + > + if (found_fd < 0 || found_elf == NULL) > + { > + fprintf (stderr, "dwflst_tracker_find_cached_elf failed\n"); > + free (found_file_name); > + if (found_elf != NULL) > + elf_end (found_elf); > + dwflst_tracker_end (tracker); > + return 1; > + } > + > + free (found_file_name); > + elf_end (found_elf); > + > + pthread_t threads[2]; > + struct thread_context contexts[2]; > + int num_created = 0; > + int test_failed = 0; > + > + /* Finder/finder: exercise concurrent retention of the cached Elf. */ > + for (int i = 0; i < 2; i++) > + { > + contexts[i].tracker = tracker; > + contexts[i].module_name = argv[1]; > + contexts[i].successes = 0; > + } > + > + for (int i = 0; i < 2; i++) > + { > + int ret = pthread_create (&threads[i], NULL, thread_work, > &contexts[i]); > + if (ret != 0) > + { > + fprintf (stderr, "Failed to create thread: %s\n", strerror (ret)); > + test_failed = 1; > + break; > + } > + > + num_created++; > + } > + > + for (int i = 0; i < num_created; i++) > + { > + int ret = pthread_join (threads[i], NULL); > + if (ret != 0) > + { > + fprintf (stderr, "Failed to join thread: %s\n", strerror (ret)); > + return 1; > + } > + } > + > + if (test_failed) > + { > + > + dwflst_tracker_end (tracker); > + return 1; > + } > + > + if (contexts[0].successes != 2000 > + || contexts[1].successes != 2000) > + { > + fprintf (stderr, "thread test failed: %d %d\n", > + contexts[0].successes, contexts[1].successes); > + > + dwflst_tracker_end (tracker); > + return 1; > + } > + > + pthread_t finder; > + pthread_t replacer; > + > + struct thread_context finder_context; > + struct replacer_context replacer_context; > + > + finder_context.tracker = tracker; > + finder_context.module_name = argv[1]; > + finder_context.successes = 0; > + > + replacer_context.tracker = tracker; > + replacer_context.module_name = argv[1]; > + replacer_context.fd = fd; > + replacer_context.successes = 0; > + > + /* Finder/replacer: exercise concurrent access to the cache entry. */ > + int ret = pthread_create (&finder, NULL, thread_work, &finder_context); > + if (ret != 0) > + { > + fprintf (stderr, "Failed to create finder thread: %s\n", strerror > (ret)); > + > + dwflst_tracker_end (tracker); > + return 1; > + } > + > + ret = pthread_create (&replacer, NULL, thread_replace, &replacer_context); > + if (ret != 0) > + { > + fprintf (stderr, "Failed to create replacer thread: %s\n", > + strerror (ret)); > + > + ret = pthread_join (finder, NULL); > + if (ret != 0) > + { > + fprintf (stderr, "Failed to join finder thread: %s\n", > + strerror (ret)); > + return 1; > + } > + > + > + dwflst_tracker_end (tracker); > + return 1; > + } > + > + ret = pthread_join (finder, NULL); > + if (ret != 0) > + { > + fprintf (stderr, "Failed to join finder thread: %s\n", strerror (ret)); > + return 1; > + } > + > + ret = pthread_join (replacer, NULL); > + if (ret != 0) > + { > + fprintf (stderr, "Failed to join replacer thread: %s\n", strerror > (ret)); > + return 1; > + } > + > + if (finder_context.successes != 2000 > + || replacer_context.successes != 1000) > + { > + fprintf (stderr, "finder/replacer test failed: %d %d\n", > + finder_context.successes, replacer_context.successes); > + > + dwflst_tracker_end (tracker); > + return 1; > + } > + > + > + dwflst_tracker_end (tracker); > + > + return 0; > +} > diff --git a/tests/run-dwflst-tracker-concurrent.sh > b/tests/run-dwflst-tracker-concurrent.sh > new file mode 100755 > index 00000000..35614a63 > --- /dev/null > +++ b/tests/run-dwflst-tracker-concurrent.sh > @@ -0,0 +1,24 @@ > +#!/bin/sh > +# Copyright (C) 2026 Trithem > +# This file is part of elfutils. > +# > +# This file is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as > +# published by the Free Software Foundation; either version 3 of > +# the License, or (at your option) any later version. > +# > +# elfutils is distributed in the hope that it will be useful, but > +# WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see > +# <http://www.gnu.org/licenses/>. > + > +. $srcdir/test-subr.sh > + > +testrun ${abs_builddir}/dwflst-tracker-concurrent \ > + ${abs_builddir}/dwflst-tracker-concurrent > + > +exit 0 > -- > 2.53.0 > > >
