Skip to content

_mi_subproc(): fall back to the main subproc when tld->subproc is not yet set - #1391

Open
Antony Hosking (hosking) wants to merge 1 commit into
microsoft:dev3from
hosking:fix-subproc-null-during-init
Open

_mi_subproc(): fall back to the main subproc when tld->subproc is not yet set#1391
Antony Hosking (hosking) wants to merge 1 commit into
microsoft:dev3from
hosking:fix-subproc-null-during-init

Conversation

@hosking

Copy link
Copy Markdown

_mi_subproc() (src/subproc.c) defends against partially-initialized thread state per issue #1289:

mi_theap_t* theap = _mi_theap_default();
if (theap == NULL || theap->tld == NULL) {  // see issue #1289
  return _mi_subproc_main();
}
else {
  return theap->tld->subproc;
}

There is a third member of the same initialization progression it does not cover: a window where the theap and its tld exist but tld->subproc has not been set yet. In that window the function returns NULL instead of falling back to _mi_subproc_main(), and any caller that immediately dereferences the result for stat accounting faults.

How we hit it (context, not required for the fix): a malloc-override shared library whose constructor allocates before mimalloc's process/thread init has completed. The first OS commit then runs mi_subproc_stat_counter_increase(NULL, commit_calls, 1) and faults at address 0x700, which is exactly offsetof(mi_subproc_t, stats.commit_calls) in our build — confirmed against the faulting address register. Any embedder whose constructor ordering puts allocation ahead of subproc wiring can reach the same window; it is not specific to our fork's extensions.

Fix — extend the existing guard with the missing disjunct, so the fallback covers the whole progression it already defends:

if (theap == NULL || theap->tld == NULL || theap->tld->subproc == NULL) {  // see issue #1289
  return _mi_subproc_main();
}

This is what we run; it removed the constructor-time crash with no other change. The fallback (_mi_subproc_main()) is the same one the existing guard already selects for the earlier stages of the same window, so behavior for initialized threads is unchanged. Validated against dev3 HEAD: clean build, mimalloc-test-api and mimalloc-test-stress pass.

…et set

_mi_subproc() defends against partially-initialized thread state (issue microsoft#1289)
by checking theap and theap->tld, but not the third member of the same
initialization progression: a window where the theap and its tld exist while
tld->subproc has not been set yet.  In that window it returns NULL instead of
falling back to _mi_subproc_main(), and the first caller that dereferences the
result for stat accounting crashes (observed: a malloc-override shared library
whose constructor allocates before process init completes faults at
offsetof(mi_subproc_t, stats.commit_calls) in
mi_subproc_stat_counter_increase(NULL, commit_calls, 1)).

Extend the guard with the missing disjunct so the existing fallback covers the
whole window it already defends; behavior for initialized threads is unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@daanx

Copy link
Copy Markdown
Collaborator

Thanks! I'll merge the commit .. but I don't quite understand how it can happen that the the tld->subproc can be NULL; can you reproduce the error and show a stack trace? I wonder if we see a thread that allocates after being terminated instead?

@hosking

Copy link
Copy Markdown
Author

Thanks! Reproduced, and I can answer the mechanism question precisely — it isn't a terminated thread, and it isn't a race: the NULL is a static initializer.

The chain, all in current dev3

// src/prim/prim-tls.c:27   — where every thread starts
mi_decl_thread mi_theap_t* __mi_theap_default = (mi_theap_t*)&_mi_theap_empty;

// src/init.c — _mi_theap_empty.tld
  &mi_tld_detached,       // tld

// src/init.c — mi_tld_detached.subproc
  NULL,                   // subproc

So on any thread that allocates before mimalloc has initialized it:

  • theap = &_mi_theap_empty → non-NULL, passes check 1
  • theap->tld = &mi_tld_detached → non-NULL, passes check 2
  • theap->tld->subproc = NULL ← returned

mi_tld_init() sets tld->subproc as its first statement, so any tld it has touched is fine. The NULL comes from the statically-initialized detached tld that __mi_theap_default points at until then.

Fresh trace (x86-64, glibc, unpatched build)

We instrumented the predicate rather than inferring it:

[probe] _mi_subproc #1: theap=0x... tld=0x... subproc=(nil)   <- both existing checks pass
SIGSEGV in __mi_stat_counter_increase_mt, rdi = 0x700 = offsetof(mi_subproc_t, stats.commit_calls)
  _mi_os_commit <- mi_manage_os_memory_ex2 <- mi_manage_os_memory_ex
  <- our library's constructor
  <- call_init (dl-init.c:70) <- _dl_init (dl-init.c:117) <- _dl_start_user

With the guard restored, the identical command exits 0.

On your alternative: frames 7–10 are the dynamic loader running ELF constructors on the main thread during _dl_init — before main(), before any thread has been created. There is only one thread, so it can't be a terminated one.

Two caveats worth stating

  1. Linkage matters. The window only opens when mimalloc is linked statically into the preloading library. As a shared object, libmimalloc initializes the tld in its own constructor and closes the window first — measured 0/3 dynamic vs 3/3 static. Our LD_PRELOAD shim links it statically, which is why it hit this on every process including /bin/echo.
  2. x86-64 reproduces the fault; arm64 (glibc 2.35) reproduces only the precondition. There, mi_manage_os_memory_ex returns 0 before reaching _mi_os_commit, so the NULL subproc is returned and then handled by failing rather than dereferenced. Which member gets touched first — and therefore whether a NULL faults or merely fails — looks build/arch dependent.

We also could not reproduce on macOS, and the source predicts that: on Apple arm64 _mi_theap_default() takes the pthread-key path, which returns NULL when the key is unset and is caught by the first check. The vulnerable path is the thread-local-pointer one, whose initial value is a valid pointer to the empty theap.

Happy to add a test or adjust the patch if you'd prefer a different shape.

@res2k

Copy link
Copy Markdown
Contributor
  1. The window only opens when mimalloc is linked statically into the preloading library.

Why though? The intention that mimalloc is initialized automatically is there - see mi_process_attach in prim.c.

So why did that not get executed (first)?
It's at least worthwhile to find out why the mechanisms that supposedly initialize mimalloc automatically don't work here.
That there's no default subproc rather seems to be a side-effect of that...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants