Skip to content

Fix race condition in checkMailbox #25073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sbc100 merged 1 commit into from
Aug 28, 2025
Merged

Fix race condition in checkMailbox #25073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions src/lib/libcore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2094,8 +2094,13 @@ addToLibrary({
#endif
try {
#if PTHREADS
if (ENVIRONMENT_IS_PTHREAD) __emscripten_thread_exit(EXITSTATUS);
else
if (ENVIRONMENT_IS_PTHREAD) {
// exit the current thread, but only if there is one active.
// TODO(https://github.com/emscripten-core/emscripten/issues/25076):
// Unify this check with the runtimeExited check above
if (_pthread_self()) __emscripten_thread_exit(EXITSTATUS);
return;
}
#endif
_exit(EXITSTATUS);
} catch (e) {
Expand All @@ -2113,8 +2118,19 @@ addToLibrary({
},

#else // MINIMAL_RUNTIME
// MINIMAL_RUNTIME doesn't support the runtimeKeepalive stuff
$callUserCallback: (func) => func(),
$callUserCallback: (func) => {
// MINIMAL_RUNTIME doesn't support the runtimeKeepalive stuff, but under
// some circumstances it supportes `runtimeExited`
#if EXIT_RUNTIME
if (runtimeExited) {

@juj juj Aug 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen if we are building in -sEXIT_RUNTIME=0 mode, and some code calls abort(), and then this async timeout for checkMailbox() triggers? Wouldn't that result in this safeguard passing right through into calling _pthread_self() again, since in EXIT_RUNTIME=0 mode this check was compiled out altogether?

_pthread_self() is a C compiled function, so if the Worker is not hosting a pthread and hence doesn't have an active program stack, then even entering that function will not be safe (even though it might return 0;), since it could e.g. do a stack bump into corrupted space.

@sbc100 sbc100 Aug 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this could be a problem for MINIMAL_RUNTIME, since it doesn't track ABORT like the normal runtime does.

Do we have any other signal to know of the runtime is alive other than ABORT, runtimeExited and/or pthread_self() == null? i.e. could we do any better?

I've love to consolidate those 3 myself into single "is_runtime_ok_or_valid_right_now" things, so adding yet another piece of state seems like the wrong direction.

@juj juj Aug 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this could be a problem for MINIMAL_RUNTIME, since it doesn't track ABORT like the normal runtime does.

I mean just in regular runtime, if one builds with -sEXIT_RUNTIME=0, then the above if (runtimeExited) build won't be compiled in, and if a pthread calls abort, then that would set ABORT = true; in the Worker and shut down its runtime? But this code wouldn't catch it?

@sbc100 sbc100 Aug 27, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the other implementation of callUserCallback for the regular runtime. It does include that check.

@juj juj Aug 27, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, right - missed that this was the MINIMAL_RUNTIME path. That makes sense. LGTM.

#if ASSERTIONS
err('user callback triggered after runtime exited or application aborted. Ignoring.');
#endif
return;
}
#endif
func();
},
#endif // MINIMAL_RUNTIME

$asmjsMangle: (x) => {
Expand Down
19 changes: 11 additions & 8 deletions src/lib/libpthread.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,18 +1094,16 @@ var LibraryPThread = {
checkStackCookie();
#endif
function finish(result) {
#if MINIMAL_RUNTIME
#if !MINIMAL_RUNTIME
// In MINIMAL_RUNTIME the noExitRuntime concept does not apply to
// pthreads. To exit a pthread with live runtime, use the function
// emscripten_unwind_to_js_event_loop() in the pthread body.
__emscripten_thread_exit(result);
#else
if (keepRuntimeAlive()) {
EXITSTATUS = result;
} else {
__emscripten_thread_exit(result);
return;
}
#endif
__emscripten_thread_exit(result);
}
#if ASYNCIFY == 2
result = await result;
Expand Down Expand Up @@ -1215,19 +1213,24 @@ var LibraryPThread = {
'pthread_self',
'_emscripten_check_mailbox',
'_emscripten_thread_mailbox_await'],
$checkMailbox: () => {
$checkMailbox: () => callUserCallback(() => {
// Only check the mailbox if we have a live pthread runtime. We implement
// pthread_self to return 0 if there is no live runtime.
//
// TODO(https://github.com/emscripten-core/emscripten/issues/25076):
// Is this check still needed? `callUserCallback` is supposed to
// ensure the runtime is alive, and if `_pthread_self` is NULL then the
// runtime certainly is *not* alive, so this should be a redundant check.
var pthread_ptr = _pthread_self();
if (pthread_ptr) {
// If we are using Atomics.waitAsync as our notification mechanism, wait
// for a notification before processing the mailbox to avoid missing any
// work that could otherwise arrive after we've finished processing the
// mailbox and before we're ready for the next notification.
__emscripten_thread_mailbox_await(pthread_ptr);
callUserCallback(__emscripten_check_mailbox);
__emscripten_check_mailbox();
}
},
}),

_emscripten_thread_mailbox_await__deps: ['$checkMailbox'],
_emscripten_thread_mailbox_await: (pthread_ptr) => {
Expand Down
8 changes: 4 additions & 4 deletions test/code_size/test_codesize_minimal_pthreads.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.out.js": 7649,
"a.out.js.gz": 3768,
"a.out.js": 7660,
"a.out.js.gz": 3776,
"a.out.nodebug.wasm": 19588,
"a.out.nodebug.wasm.gz": 9025,
"total": 27237,
"total_gz": 12793,
"total": 27248,
"total_gz": 12801,
"sent": [
"a (memory)",
"b (emscripten_get_now)",
Expand Down
8 changes: 4 additions & 4 deletions test/code_size/test_codesize_minimal_pthreads_memgrowth.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.out.js": 8076,
"a.out.js.gz": 3974,
"a.out.js": 8087,
"a.out.js.gz": 3978,
"a.out.nodebug.wasm": 19589,
"a.out.nodebug.wasm.gz": 9025,
"total": 27665,
"total_gz": 12999,
"total": 27676,
"total_gz": 13003,
"sent": [
"a (memory)",
"b (emscripten_get_now)",
Expand Down
4 changes: 1 addition & 3 deletions tools/emscripten.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,9 +897,7 @@ def install_debug_wrapper(sym):
return False
# Likewise `__trap` can occur before the runtime is initialized since it is used in
# abort.
# pthread_self is currently called in some cases after the runtime has exited.
# TODO: Look into removing these, and improving our robustness around thread termination.
return sym not in {'__trap', 'pthread_self'}
return sym != '__trap'


def should_export(sym):
Expand Down