android / android/ndk

[BUG] pthread_create() segfaults when called from a constructor in statically linked executables

Open
#2,244 1 comment 0 reactions 0 assignees View on GitHub
bug
Dominant language
No language data
Stars
2.3k
Forks
310
PR merge metrics
No merged PRs in 30d

Description

### Description

bionic segfaults when running `pthread_create()` from an ELF static initializer (`__attribute__((constructor))`) in a statically linked binary.

After analyzing the issue (see below), I expect it also affects `pthread_join()` and contended paths in the `pthread_mutex_*` functions. `pthread_create()` appears to be the only one currently reachable from constructors (the others both require other threads to already exist), but the underlying problem is that any libc call using `ScopedTrace` is unsafe in this context.

## Version

I ran the below test case on both the latest r29 stable and the latest r27d LTS (both downloaded today):

```
$ cat "$NDK/source.properties"
Pkg.Desc = Android NDK
Pkg.Revision = 29.0.14206865
Pkg.BaseRevision = 29.0.14206865
Pkg.ReleaseName = r29
```

```
$ cat "$NDK/source.properties"
Pkg.Desc = Android NDK
Pkg.Revision = 27.3.13750724
Pkg.BaseRevision = 27.3.13750724
Pkg.ReleaseName = r27d
```

## Test case

```c
#include
#include

static void *thread_fn(void *arg) {
(void)arg;
return NULL;
}

__attribute__((constructor)) static void ctor(void) {
pthread_t t;
write(1, "ctor: before\n", 13);
pthread_create(&t, NULL, thread_fn, NULL);
write(1, "ctor: after\n", 12);
}

int main(void) {
write(1, "main\n", 5);
return 0;
}
```

Built with `"$NDK/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android30-clang" -static -o ctortest ctortest.c`, and tested on Android 14 in the Cuttlefish emulator (though the exact environment shouldn't matter much due to the static linking).

**Expected:** prints both messages in the constructor, then the one in `main()`, and exits cleanly

**Actual:** prints "`ctor: before`" and then segfaults

## Control cases

**No `-static`**: The same code now works as expected.

**Moving the `pthread_create()` call to `main()`**: Removing the constructor and changing `main` to this:

```c
int main(void) {
pthread_t t;
write(1, "main: before\n", 13);
pthread_create(&t, NULL, thread_fn, NULL);
write(1, "main: after\n", 12);
return 0;
}
```

...also works fine.

## Debugging

I added a sigaction SEGV handler to retrieve the crash address:

Modified test case including the SEGV handler

```c
#include
#include
#include
#include
#include
#include

static void segv(int sig, siginfo_t* info, void* uc_v);

static void *thread_fn(void *arg) {
(void)arg;
return NULL;
}

__attribute__((constructor)) static void ctor(void) {
struct sigaction sa;

memset(&sa, 0, sizeof(sa));
sa.sa_sigaction = segv;
sa.sa_flags = SA_SIGINFO;
sigemptyset(&sa.sa_mask);

if (sigaction(SIGSEGV, &sa, NULL) == -1) {
perror("sigaction");
exit(EXIT_FAILURE);
}

pthread_t t;
write(1, "ctor: before\n", 13);
pthread_create(&t, NULL, thread_fn, NULL);
write(1, "ctor: after\n", 12);
}

static void segv(int sig, siginfo_t* info, void* uc_v) {
ucontext_t* uc = uc_v;
char buf[256];
int n = snprintf(buf, sizeof(buf),
"SEGV addr=%p pc=%p lr=%p | ctor=%p pthread_create=%p thread_fn=%p\n",
info->si_addr, (void*)uc->uc_mcontext.pc,
(void*)uc->uc_mcontext.regs[30],
(void*)&ctor, (void*)&pthread_create, (void*)&thread_fn);
write(1, buf, n);
_exit(139);
}

int main(void) {
write(1, "main\n", 5);
return 0;
}
```

```
SEGV addr=0x0 pc=0x266970 lr=0x23f1c8 | ctor=0x21b18c pthread_create=0x232d60 thread_fn=0x21b2dc
```

Then I used Ghidra to find the PC and LR addresses in the test binary:

- PC landed in `__strchr_aarch64`
- LR landed in `PropertyInfoArea::GetPropertyInfoIndexes()` at its `strchr(name, '.')` call

Finally, I asked Claude Opus 5 to figure out the root cause based on that:

> bionic's `pthread_create` traces itself through a file-scope `CachedProperty` object. That object's C++ constructor is a dynamic initializer in libc's `.init_array`. In a static link, libc's `.init_array` entries are merged with the executable's, and their ordering depends on link order — and in this binary, `ctor` lands ahead of `bionic_systrace.cpp's` initializer, so the executable's constructors run before libc has initialised itself. The object is then still zero-filled BSS, `property_name_` is null, and `__system_property_find(nullptr)` walks down to `strchr(NULL, '.')`. Dynamic links are immune because libc.so's initializers always run at load time, long before the executable's constructors.

It also notes that not just `pthread_create`, but "***any* traced libc call is unsafe from a static executable's constructors.** The same `ScopedTrace` path is used by `pthread_join()` and the contended paths in `pthread_mutex` ([`libc/bionic/pthread_mutex.cpp:190`](https://android.googlesource.com/platform/bionic/+/731631f300090436d7f5df80d50b6275c8c60a93/libc/bionic/pthread_mutex.cpp#190), [`:597`](https://android.googlesource.com/platform/bionic/+/731631f300090436d7f5df80d50b6275c8c60a93/libc/bionic/pthread_mutex.cpp#597), [`:752`](https://android.googlesource.com/platform/bionic/+/731631f300090436d7f5df80d50b6275c8c60a93/libc/bionic/pthread_mutex.cpp#752))."

I can see in Ghidra that the custom "`ctor`" function did indeed happen to come before bionic_systrace.cpp's:

```
__init_array_start
0026c180 a4 11 22 00 00 00 00 00 addr init_have_lse_atomics
0026c188 4c 14 22 00 00 00 00 00 addr __init_cpu_features
0026c190 8c b1 21 00 00 00 00 00 addr ctor
0026c198 48 10 23 00 00 00 00 00 addr _GLOBAL__sub_I_release.cpp
0026c1a0 48 3c 23 00 00 00 00 00 addr _GLOBAL__sub_I_bionic_systrace.cpp
```

## Originating commit

[Commit `2cb5f7f`](https://android.googlesource.com/platform/bionic/+/2cb5f7f578ec682c2bf628f29ffe21de8ccbc917%5E%21/#F0) appears to be the regression point:

```diff
commit 2cb5f7f578ec682c2bf628f29ffe21de8ccbc917
Author: Wei Li
Date: Fri Jan 26 15:00:32 2018 +0800

Move static variable out of should_trace().

[...]

diff --git a/libc/bionic/bionic_systrace.cpp b/libc/bionic/bionic_systrace.cpp
index 970a92ba1..bac3d8802 100644
--- a/libc/bionic/bionic_systrace.cpp
+++ b/libc/bionic/bionic_systrace.cpp
@@ -29,12 +29,11 @@
#define WRITE_OFFSET 32

static Lock g_lock;
+static CachedProperty g_debug_atrace_tags_enableflags("debug.atrace.tags.enableflags");
+static uint64_t g_tags;
static int g_trace_marker_fd = -1;

static bool should_trace() {
- static CachedProperty g_debug_atrace_tags_enableflags("debug.atrace.tags.enableflags");
- static uint64_t g_tags;
-
g_lock.lock();
if (g_debug_atrace_tags_enableflags.DidChange()) {
g_tags = strtoull(g_debug_atrace_tags_enableflags.Get(), nullptr, 0);
```

That commit was to fix a deadlock, so reverting it wouldn't be the right fix. Claude suggests that "any fix needs to avoid reintroducing the `__cxa_guard_acquire` recursion from 2cb5f7f; constant-initialising the object would be one way."

### I am using a supported NDK

- [x] I have checked and the NDK I'm using is currently supported

### Affected versions

r29, r27

Contributor guide

Open the contributing guide

Research direction

Reproduce the failure with the static constructor test case, then inspect libc/bionic/bionic_systrace.cpp and the referenced pthread_mutex.cpp paths, along with pthread_create() and pthread_join(). Trace how CachedProperty and ScopedTrace are initialized before executable constructors. Done means pthread_create() from a static constructor no longer segfaults and the related traced paths remain safe without reintroducing the reported deadlock.

Written by the indexing model from the issue text.

Assessment

Tech stack
android, c, cpp
Domain
operating-systems
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.