dev3: two first-ever-allocation bootstrap bugs on Android (arena_reserve_lock reentrancy risk, recurse-guard emutls recursion) -- patch included

[CatPieLeaf]

Summary

Two real bugs found while building a from-scratch Android ELF loader that runs a real, statically-linked-mimalloc .so (MI_OVERRIDE=ON, the CMake default) on Android NDK r28c / x86_64 / API 26, targeting the dev3 branch. Both are first-ever-allocation bootstrap issues, both reproduce with a minimal (6-line) .so — not specific to our own loader, and not specific to Android's dynamic linker path either (a real dlopen()-based load should hit the same code, since the trigger is what mimalloc itself does on a thread's first allocation, not how the .so got mapped).

Verified against dev3 as of this report (checked live via raw.githubusercontent.com, not a stale local clone) — src/options.c's recurse guard is byte-identical to what was originally found and fixed; src/arena.c's arena_reserve_lock/mi_arenas_try_alloc are present and structurally the same, though the exact metadata-allocation recursion path wasn't re-traced against the very latest dev3 this round (see the caveat under Bug 1). Patch attached below fixes both, verified working against the version of dev3 used during the original investigation.

Bug 1: possible self-deadlock risk in mi_arenas_try_alloc's arena_reserve_lock on first-ever arena creation

src/arena.c, mi_arenas_try_alloc:

mi_subproc_t* const subproc = heap->subproc;
const size_t arena_count = mi_arenas_get_count(subproc);
mi_lock(&subproc->arena_reserve_lock) {
  if (arena_count == mi_arenas_get_count(subproc)) {
    mi_arena_id_t arena_id = _mi_arena_id_none();
    mi_arena_reserve(subproc, mi_size_of_slices(slice_count), allow_large, &arena_id);
  }
  ...
}

In the version of dev3 used for the original investigation, mi_arena_reserve() (called while holding arena_reserve_lock) needed metadata space for the new arena's own bookkeeping struct, obtained via _mi_meta_zalloc() -> mi_meta_page_zalloc() -> _mi_arenas_alloc_aligned(). On the very first arena a subprocess ever creates, no arena/meta-page exists yet to satisfy that inner request, so _mi_arenas_alloc_aligned() called mi_arenas_try_alloc() again — same thread, same non-recursive lock, still held from the outer call — a genuine, reproducible self-deadlock, confirmed via gdb (single thread, pthread_mutex_lock -> __lll_lock_wait, static across repeated samples, not a slow computation).

Confirmed the exact recursive call site by reading the return address directly off the stack at both pthread_mutex_lock entries (registers deep in glibc's optimized mutex path are scratch by that point, unreliable to read directly) — identical both times, resolved via llvm-addr2line: mi_lock_acquire, atomic.h:456, called from mi_arenas_try_alloc, arena.c:534 both times.

_mi_preloading() (checked a few lines above the lock) doesn't cover this: _mi_auto_process_init() sets os_preloading = false as its own first statement, long before this later, separate "a thread's first real allocation lazily triggers first-arena-creation" path runs.

Caveat, stated plainly: re-checked dev3 live just before filing this — arena_reserve_lock/mi_arenas_try_alloc are present, structurally unchanged (same lock pattern, same surrounding logic, arena.c:534 still mi_lock(&subproc->arena_reserve_lock)), but I did not re-trace whether mi_arena_reserve()'s current metadata-allocation path (it now goes through mi_reserve_os_memory_ex2/mi_manage_os_memory_ex2 rather than the _mi_meta_zalloc chain the original investigation cited by name) still recurses back into this same lock, or whether that's changed since. Flagging honestly rather than claiming a fresh repro against the exact current HEAD — the lock's own non-reentrancy is real and current either way; whether today's code still reaches it recursively is worth a maintainer's own check against the live metadata-allocation path.

Reproduction (minimal, no thread_local needed):

#include <stdlib.h>
__attribute__((constructor))
static void ctor_mallocs(void) {
    void* p = malloc(256);
    if (p == NULL) abort();
    free(p);
}

Compiled as an Android shared library (x86_64-linux-android26-clang -shared -fPIC), statically linked against libmimalloc.a (MI_BUILD_SHARED=ON -DMI_BUILD_STATIC=ON, MI_OVERRIDE default-on), loaded via anything that runs DT_INIT_ARRAY (a real dlopen, or the from-scratch ELF loader used here). First-ever malloc() call deadlocked, on the dev3 snapshot used at the time.

Fix (see attached patch, src/arena.c hunk): a thread-local reentrancy guard around the mi_lock(&subproc->arena_reserve_lock) block, mirroring this same file's own existing recurse/mi_recurse_enter pattern (see Bug 2) for the analogous mi_vfprintf hazard. If the guard is already set (meaning we're already inside this critical section on this thread), the inner call returns NULL immediately instead of re-acquiring the lock; the existing fallback (raw OS allocation) then correctly takes over for the metadata request.

Verified working at the time: rebuilt mimalloc (Debug, MI_DEBUG=2) with the patch, relinked the minimal repro above, ran with no environment workarounds — deadlock gone, confirmed via gdb (pthread_mutex_lock no longer re-entered on the held lock).

A non-source workaround also avoids the trigger (not a real fix, degrades allocator behavior process-wide): setting both MIMALLOC_ARENA_RESERVE=0 and MIMALLOC_DISALLOW_ARENA_ALLOC=1 before the process starts.

Bug 2: unbounded recursion in options.c's recurse cold-path guard on Android (confirmed present, unchanged, on current dev3)

src/options.c, around mi_recurse_enter_prim/mi_recurse_exit_prim:

static mi_decl_thread bool recurse = false;

static mi_decl_noinline bool mi_recurse_enter_prim(void) {
  ...
}
static mi_decl_noinline void mi_recurse_exit_prim(void) {
  recurse = false;
}

This guards _mi_fputs/mi_vfprintf against recursively calling itself while reporting a message. On Android, a plain mi_decl_thread/__thread for a library loaded outside the initial process image compiles to LLVM/compiler-rt's emulated, allocation-backed TLS (__emutls_get_address) — so the very first read/write of recurse on a given thread needs to malloc() a per-thread backing array. If that nested malloc() itself needs to report an error (a real, reachable case — "out of memory while handling out of memory" is exactly the scenario this guard exists to make safe), it calls mi_recurse_enter() again, touching the same, still-mid-initialization recurse access again — recursing without bound until the stack is exhausted.

Confirmed via a real reproduction (same minimal .so as above, using a _Thread_local variable to force the emulated-TLS path) and a full stack scan for the repeating pattern: a real, exact 15-function cycle (__emutls_get_address -> __libc_malloc (mimalloc's own override) -> mi_theap_malloc -> ... -> mi_page_fresh -> mi_page_fresh_alloc -> back into __emutls_get_address), each address appearing ~208-209 times in just the first 400KB of the exhausted stack scanned.

mimalloc already solves the equivalent problem correctly for the hot allocation path on this same platform: _mi_theap_default() (prim-tls.h, MI_TLS_MODEL_PTHREADS) deliberately uses a pthread_key_t instead of a raw thread_local, specifically because bare __thread is unsafe here. This cold-path guard variable never got the same treatment.

Fix (attached patch, src/options.c hunk): give recurse the same pthread_key_t treatment _mi_theap_default_key already gets, scoped to __ANDROID__ only (Apple/OpenBSD behavior in this same #if chain left untouched — not verified against those platforms by this investigation). Reuses mimalloc's own existing mi_pthread_key_get/mi_pthread_key_set helpers.

Verified working: rebuilt mimalloc (Debug, MI_DEBUG=2) with the patch, relinked the repro, ran with zero environment workarounds — stack exhaustion gone, confirmed no more of the 15-function cycle.

A related, same-shape issue was also found in mi_arena_reserve_recursing (the thread-local guard Bug 1's own fix introduces) once Bugs 1-3 (a third, unrelated Stud-side sysconf() bug, not a mimalloc issue, omitted here) were fixed and the repro ran far enough to expose it: the exact same plain-thread_local-on-Android hazard, on a different variable, causing 4362 redundant full page/arena allocations in a row instead of ever reusing one. Same fix pattern, included in the attached patch (also covers this second arena.c hunk).

Patch

Applies to the dev3 snapshot used during the original investigation. The options.c hunk (Bug 2) is confirmed unchanged against current dev3 and should apply cleanly or near-cleanly. The arena.c hunks (Bug 1 and the mi_arena_reserve_recursing follow-up) may need re-basing against whatever mi_arena_reserve's current metadata-allocation path looks like — the underlying reentrancy-guard pattern should transfer regardless of exact line drift.

diff --git a/src/arena.c b/src/arena.c
index 4077cf6..85b9a4c 100644
--- a/src/arena.c
+++ b/src/arena.c
@@ -528,6 +528,44 @@ static mi_decl_noinline void* mi_arenas_try_alloc(
   // don't create arena's if OS allocation is disallowed
   if (mi_option_is_enabled(mi_option_disallow_os_alloc)) return NULL;
 
+  // Reentrancy guard: reserving a fresh arena (below) needs meta-data space
+  // for the new arena's own bookkeeping struct, which is obtained via
+  // _mi_meta_zalloc -> mi_meta_page_zalloc -> _mi_arenas_alloc_aligned. On
+  // the very first arena a subprocess ever creates, no arena/meta-page
+  // exists yet to satisfy that request, so _mi_arenas_alloc_aligned tries
+  // mi_arenas_try_alloc() again -- i.e. THIS function, on the SAME thread,
+  // while still holding subproc->arena_reserve_lock below. That lock is a
+  // plain (non-recursive) mutex, so this is a guaranteed self-deadlock, not
+  // a race: same thread, same lock, no other thread exists to release it.
+  // _mi_preloading() (just above) does not cover this -- it only guards
+  // allocations made during the process's very first constructor
+  // (_mi_auto_process_init sets os_preloading=false as its own first
+  // statement), not this later, separate "first real allocation lazily
+  // triggers first arena creation" path. Mirrors the existing
+  // recurse/mi_recurse_enter pattern this file's sibling (options.c)
+  // already uses for the analogous mi_vfprintf recursion hazard.
+  // Same class of bug already fixed for options.c's `recurse` guard: a plain
+  // `mi_decl_thread bool` here compiles to allocation-backed emulated TLS on
+  // Android (__emutls_get_address), and on this platform/loader combination
+  // that per-thread backing array does not reliably persist across repeated
+  // accesses -- confirmed via a real, minimal (__emutls_get_address called
+  // directly, twice, through Stud's loader) and a full trace showing this
+  // exact guard's own emutls control block being the one whose per-thread
+  // array gets recreated on every single mi_arenas_try_alloc() call (4362
+  // times in one repro run, each triggering a full fresh page/arena
+  // allocation instead of ever reusing one -- the "bug #4" stack/slow-path
+  // cascade). Same fix as `recurse` (options.c): Android-gated pthread_key
+  // storage via mimalloc's own existing helpers, bypassing emutls entirely.
+#if defined(__ANDROID__)
+  static pthread_key_t mi_arena_reserve_recursing_key = MI_PTHREAD_KEY_INVALID;
+  if (mi_pthread_key_get(mi_arena_reserve_recursing_key) != NULL) return NULL;
+  mi_pthread_key_set(&mi_arena_reserve_recursing_key, (void*)1);
+#else
+  static mi_decl_thread bool mi_arena_reserve_recursing = false;
+  if (mi_arena_reserve_recursing) return NULL;
+  mi_arena_reserve_recursing = true;
+#endif
+
   // otherwise, try to reserve a new arena -- but one thread at a time.. (todo: allow 2 or 4 to reduce contention?)
   mi_subproc_t* const subproc = heap->subproc;
   const size_t arena_count = mi_arenas_get_count(subproc);
@@ -541,6 +579,11 @@ static mi_decl_noinline void* mi_arenas_try_alloc(
       // another thread already reserved a new arena
     }
   }
+#if defined(__ANDROID__)
+  mi_pthread_key_set(&mi_arena_reserve_recursing_key, NULL);
+#else
+  mi_arena_reserve_recursing = false;
+#endif
   // try once more to allocate in the new arena
   mi_assert_internal(req_arena == NULL);
   p = mi_arenas_try_find_free(heap, slice_count, alignment, commit, allow_large, req_arena, tseq, numa_node, memid);
diff --git a/src/options.c b/src/options.c
index 282121e..2fd75f7 100644
--- a/src/options.c
+++ b/src/options.c
@@ -442,6 +442,58 @@ static _Atomic(size_t) warning_count; // = 0;  // when >= max_warning_count stop
 // variables on demand. This is why we use a _mi_preloading test on such
 // platforms. However, C code generator may move the initial thread local address
 // load before the `if` and we therefore split it out in a separate function.
+//
+// On Android specifically, the `_mi_preloading()` test above is NOT
+// sufficient on its own: `_mi_auto_process_init()` sets `os_preloading`
+// to `false` as its own first statement, long before any real allocator
+// activity has happened, so by the time a thread's first real allocation
+// lazily needs to report an error (e.g. "out of memory" from a failed
+// arena reservation) and calls into `_mi_fputs`/`mi_vfprintf` for the
+// first time, `_mi_preloading()` already (correctly, for its own purpose)
+// reports `false` -- yet this specific `recurse` variable, being a plain
+// `mi_decl_thread bool`, is *also* accessed here for the very first time
+// on this thread. On Android, `thread_local`/`__thread` for a library
+// loaded outside the initial process image compiles to LLVM/compiler-rt's
+// *emulated*, allocation-backed TLS (`__emutls_get_address`) -- so this
+// first-ever read/write of `recurse` itself needs to `malloc()` a
+// per-thread backing array, and that nested `malloc()` call, if it in
+// turn needs to report *its own* error (a real, reachable case: the
+// exact "out of memory reporting an out-of-memory" scenario this guard
+// exists to prevent), calls back into `mi_recurse_enter()` again -- for
+// the SAME not-yet-finished `recurse` access -- recursing without bound
+// until the stack is exhausted. Confirmed via a real, minimal (6-line)
+// reproduction and a full stack scan identifying the exact repeating
+// 15-function cycle.
+//
+// mimalloc already solves the equivalent problem for the *hot* allocation
+// path on exactly this same platform: `_mi_theap_default()`
+// (`prim-tls.h`, `MI_TLS_MODEL_PTHREADS`, `#if ... || defined(__ANDROID__)`)
+// deliberately avoids a raw thread_local and uses a `pthread_key_t`
+// instead specifically because bare `__thread` is unsafe here. This
+// *cold*-path guard variable was never given the same treatment. Applying
+// it here, scoped to Android only (not touching the Apple/OpenBSD
+// behavior this file's own comment already describes, unverified by this
+// investigation): a lazily-created `pthread_key_t`, using mimalloc's own
+// existing `mi_pthread_key_get`/`mi_pthread_key_set` helpers (already
+// used by `_mi_theap_default_key` et al) rather than raw pthread calls,
+// for consistency with how this codebase already handles the identical
+// class of problem elsewhere. `pthread_key_create`/`pthread_getspecific`/
+// `pthread_setspecific` are real, ordinary, non-allocating (from libc's
+// perspective) bionic pthread primitives -- not vulnerable to this
+// specific recursion themselves.
+#if defined(__ANDROID__)
+static pthread_key_t mi_recurse_key = MI_PTHREAD_KEY_INVALID;
+
+static mi_decl_noinline bool mi_recurse_enter_prim(void) {
+  if (mi_pthread_key_get(mi_recurse_key) != NULL) return false;
+  mi_pthread_key_set(&mi_recurse_key, (void*)1);
+  return true;
+}
+
+static mi_decl_noinline void mi_recurse_exit_prim(void) {
+  mi_pthread_key_set(&mi_recurse_key, NULL);
+}
+#else
 static mi_decl_thread bool recurse = false;
 
 static mi_decl_noinline bool mi_recurse_enter_prim(void) {
@@ -453,6 +505,7 @@ static mi_decl_noinline void mi_recurse_exit_prim(void) {
 static mi_decl_noinline void mi_recurse_exit_prim(void) {
   recurse = false;
 }
+#endif
 
 static bool mi_recurse_enter(void) {
   #if defined(__APPLE__) || defined(__ANDROID__) || defined(MI_TLS_RECURSE_GUARD)

Environment

• mimalloc branch: dev3
• Target: Android NDK r28c, x86_64-linux-android26
• MI_OVERRIDE=ON (default), statically linked into a shared library loaded via a custom (from-scratch, but standards-conformant) ELF loader that runs DT_INIT_ARRAY the same way a real dlopen() would
• Both bugs are about mimalloc's own first-ever-allocation-on-a-thread bootstrap sequence interacting with Android's emulated-TLS (__emutls_get_address) model — not specific to our loader, should reproduce under a real dlopen()-loaded .so under the same conditions (first allocation triggers first-arena-creation while also being the first access to these specific cold-path thread-locals).

Happy to answer questions or help verify against current dev3 HEAD if useful — flagging the Bug 1 metadata-path caveat above precisely so it isn't taken as more certain than it is.