Skip to content

Commit

Permalink
Don't deadlock in dl_iterate_phdr for heaptrack_inject
Browse files Browse the repository at this point in the history 
When we call dl_iterate_phdr we need to have the heaptrack mutex
locked, otherwise we can get a AB/BA deadlock like this:

One thread is trying to update the module cache:
```
Thread 4 (Thread 0xffffa8e1ef40 (LWP 10016) "system-ui-execu"):
#0  futex_wait (private=0, expected=2, futex_word=0xffffb13bcad0 <_rtld_global+2744>) at ../sysdeps/nptl/futex-internal.h:146
#1  __GI___lll_lock_wait (futex=futex@entry=0xffffb13bcad0 <_rtld_global+2744>, private=private@entry=0) at lowlevellock.c:49
#2  0x0000ffffae7437b0 in lll_mutex_lock_optimized (mutex=0xffffb13bcad0 <_rtld_global+2744>) at pthread_mutex_lock.c:48
#3  ___pthread_mutex_lock (mutex=mutex@entry=0xffffb13bcad0 <_rtld_global+2744>) at pthread_mutex_lock.c:128
#4  0x0000ffffae7ed6ec in __GI___dl_iterate_phdr (callback=0xffffb1366d10 <(anonymous namespace)::HeapTrack::dl_iterate_phdr_callback(dl_phdr_info*, size_t, void*)>, data=0xffffa8e1e2d0) at dl-iteratephdr.c:39
#5  0x0000ffffb136982c in (anonymous namespace)::HeapTrack::updateModuleCache (this=0xffffa8e1e2d0) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:636
#6  (anonymous namespace)::HeapTrack::handleMalloc (this=this@entry=0xffffa8e1e2d0, ptr=ptr@entry=0xffff9c000b70, size=size@entry=16, trace=...) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:527
#7  0x0000ffffb13699f8 in (anonymous namespace)::HeapTrack::handleMalloc (trace=..., size=16, ptr=0xffff9c000b70, this=0xffffa8e1e2d0) at /home/milian/projects/kde/src/heaptrack/src/util/linewriter.h:271
#8  operator() (heaptrack=..., __closure=<optimized out>) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:880
#9  (anonymous namespace)::HeapTrack::op<heaptrack_malloc(void*, size_t)::<lambda((anonymous namespace)::HeapTrack&)> > (op=...) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:273
#10 heaptrack_malloc (ptr=0xffff9c000b70, size=16) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:880
#11 0x0000ffffb1364484 in (anonymous namespace)::hooks::malloc::hook (size=<optimized out>) at /home/milian/projects/kde/src/heaptrack/src/track/heaptrack_inject.cpp:120
#12 0x0000ffffae942a50 in operator new(unsigned long) () from /usr/lib/libstdc++.so.6
```

The other one is trying to overwrite symbols:
```
Thread 1 (Thread 0xffffb0a8d020 (LWP 10011) "system-ui-execu"):
#0  0x0000ffffae774560 in __GI___clock_nanosleep (clock_id=<optimized out>, clock_id@entry=0, flags=flags@entry=0, req=0xfffff3152150, rem=0xfffff3152150) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:48
#1  0x0000ffffae779520 in __GI___nanosleep (req=<optimized out>, rem=<optimized out>) at ../sysdeps/unix/sysv/linux/nanosleep.c:25
#2  0x0000ffffb13699b4 in std::this_thread::sleep_for<long, std::ratio<1l, 1000000l> > (__rtime=..., __rtime=...) at /home/milian/projects/kdab/vorwerk/sdk/sysroots/cortexa53-crypto-veld-linux/usr/include/c++/11.4.0/bits/this_thread_sleep.h:82
#3  (anonymous namespace)::HeapTrack::tryLock<(anonymous namespace)::HeapTrack::op<heaptrack_malloc(void*, size_t)::<lambda((anonymous namespace)::HeapTrack&)> >(const (anonymous namespace)::RecursionGuard&, const heaptrack_malloc(void*, size_t)::<lambda((anonymous namespace)::HeapTrack&)>&)::<lambda()> > (stopLockCheck=...) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:676
#4  (anonymous namespace)::HeapTrack::op<heaptrack_malloc(void*, size_t)::<lambda((anonymous namespace)::HeapTrack&)> > (op=...) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:265
#5  heaptrack_malloc (ptr=0xaaaafeb07b10, size=55) at /home/milian/projects/kde/src/heaptrack/src/track/libheaptrack.cpp:880
#6  0x0000ffffb1364484 in (anonymous namespace)::hooks::malloc::hook (size=<optimized out>) at /home/milian/projects/kde/src/heaptrack/src/track/heaptrack_inject.cpp:120
#7  0x0000ffffae942a50 in operator new(unsigned long) () from /usr/lib/libstdc++.so.6
#8  0x0000ffffb1365464 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*> (__end=0xffffb13b5856 "", __beg=0xffffb13b5820 "/mnt/appfs/nwot-appfs1/usr/lib/libutility-framework.so", this=0xfffff3152468) at /home/milian/projects/kdab/vorwerk/sdk/sysroots/cortexa53-crypto-veld-linux/usr/include/c++/11.4.0/bits/basic_string.tcc:219
#9  std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string (__a=..., __s=0xffffb13b5820 "/mnt/appfs/nwot-appfs1/usr/lib/libutility-framework.so", this=0xfffff3152468) at /home/milian/projects/kdab/vorwerk/sdk/sysroots/cortexa53-crypto-veld-linux/usr/include/c++/11.4.0/bits/basic_string.h:539
#10 (anonymous namespace)::cachedSymtabSize (path=<optimized out>) at /home/milian/projects/kde/src/heaptrack/src/track/heaptrack_inject--Type <RET> for more, q to quit, c to continue without paging--
.cpp:555
#11 (anonymous namespace)::iterate_phdrs (info=0xfffff31525d0, data=0x0) at /home/milian/projects/kde/src/heaptrack/src/track/heaptrack_inject.cpp:577
#12 0x0000ffffae7ed7f0 in __GI___dl_iterate_phdr (callback=0xffffb1364ea0 <(anonymous namespace)::iterate_phdrs(dl_phdr_info*, size_t, void*)>, data=0x0) at dl-iteratephdr.c:74
#13 0x0000ffffb1364870 in (anonymous namespace)::overwrite_symbols () at /home/milian/projects/kde/src/heaptrack/src/track/heaptrack_inject.cpp:589
```
  • Loading branch information
@milianw
milianw committed Jun 19, 2024
1 parent 87ae476 commit f3571b5
Show file tree
Hide file tree
Showing 5 changed files with 18 additions and 10 deletions.
5 changes: 2 additions & 3 deletions src/track/heaptrack_inject.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,7 @@ struct dlopen
{
auto ret = original(filename, flag);
if (ret) {
heaptrack_invalidate_module_cache();
overwrite_symbols();
heaptrack_invalidate_module_cache(&overwrite_symbols);
}
return ret;
}
Expand All @@ -201,7 +200,7 @@ struct dlclose
{
auto ret = original(handle);
if (!ret) {
heaptrack_invalidate_module_cache();
heaptrack_invalidate_module_cache(nullptr);
}
return ret;
}
Expand Down
4 changes: 2 additions & 2 deletions src/track/heaptrack_preload.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ void* dlopen(const char* filename, int flag) LIBC_FUN_ATTRS
void* ret = hooks::dlopen(filename, flag);

if (ret) {
heaptrack_invalidate_module_cache();
heaptrack_invalidate_module_cache(nullptr);
}

return ret;
Expand All @@ -381,7 +381,7 @@ int dlclose(void* handle) LIBC_FUN_ATTRS
int ret = hooks::dlclose(handle);

if (!ret) {
heaptrack_invalidate_module_cache();
heaptrack_invalidate_module_cache(nullptr);
}

return ret;
Expand Down
8 changes: 6 additions & 2 deletions src/track/libheaptrack.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -902,13 +902,17 @@ void heaptrack_realloc2(uintptr_t ptr_in, size_t size, uintptr_t ptr_out)
heaptrack_realloc_impl(reinterpret_cast<void*>(ptr_in), size, reinterpret_cast<void*>(ptr_out));
}

void heaptrack_invalidate_module_cache()
void heaptrack_invalidate_module_cache(heaptrack_invalidate_module_cache_callback callback)
{
RecursionGuard guard;

debugLog<VerboseOutput>("%s", "heaptrack_invalidate_module_cache()");

HeapTrack::op(guard, [&](HeapTrack& heaptrack) { heaptrack.invalidateModuleCache(); });
HeapTrack::op(guard, [&](HeapTrack& heaptrack) {
heaptrack.invalidateModuleCache();
if (callback)
callback();
});
}

void heaptrack_warning(heaptrack_warning_callback_t callback)
Expand Down
3 changes: 2 additions & 1 deletion src/track/libheaptrack.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ void heaptrack_free(void* ptr);
void heaptrack_realloc(void* ptr_in, size_t size, void* ptr_out);
void heaptrack_realloc2(uintptr_t ptr_in, size_t size, uintptr_t ptr_out);

void heaptrack_invalidate_module_cache();
typedef void (*heaptrack_invalidate_module_cache_callback)();
void heaptrack_invalidate_module_cache(heaptrack_invalidate_module_cache_callback callback);

typedef void (*heaptrack_warning_callback_t)(FILE*);
void heaptrack_warning(heaptrack_warning_callback_t callback);
Expand Down
8 changes: 6 additions & 2 deletions tests/auto/tst_libheaptrack.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,11 @@ TEST_CASE ("api") {

SUBCASE("invalidate-cache")
{
heaptrack_invalidate_module_cache();
heaptrack_invalidate_module_cache(nullptr);

static bool wasCalled = false;
heaptrack_invalidate_module_cache([]() { wasCalled = true; });
REQUIRE(wasCalled);
}

SUBCASE("multi-threaded")
Expand All @@ -104,7 +108,7 @@ TEST_CASE ("api") {
heaptrack_realloc(&i, i + 1, &i);
heaptrack_free(&i);
if (i % 100 == 0) {
heaptrack_invalidate_module_cache();
heaptrack_invalidate_module_cache(nullptr);
}
}
}));
Expand Down

0 comments on commit f3571b5

Please sign in to comment.