deps: uv: restore prior signal disposition on Android (DCP-4748)#4
Open
philippe-distributive wants to merge 1 commit into
Open
deps: uv: restore prior signal disposition on Android (DCP-4748)#4philippe-distributive wants to merge 1 commit into
philippe-distributive wants to merge 1 commit into
Conversation
Author
|
fixed upstream in libuv/libuv#5157 |
On Android, ART claims SIGSEGV/SIGBUS/SIGFPE/SIGILL/SIGTRAP/SIGABRT for implicit null-checks and stack-overflow detection, routed through libsigchain. libuv's uv__signal_unregister_handler() resets a signal to SIG_DFL once its last watcher is removed; on Android that clobbers ART's handler and emits an ERROR-level libsigchain stack trace on every embedded node::FreeEnvironment() (DCP-4748). Gated to __ANDROID__: uv__signal_register_handler() captures the previous disposition on the no-handler -> handler transition and uv__signal_unregister_handler() restores it instead of forcing SIG_DFL. bionic round-trips the sigaction flags correctly, so this is safe; it is deliberately NOT applied to other platforms (notably macOS, which drops SA_RESETHAND in the old action -- see upstream libuv nodejs#4299, which caused PR nodejs#4216 to be reverted). Off Android the code path and behaviour are unchanged. The saved table is accessed only under the global signal lock.
208331f to
dcededb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a Node environment is torn down inside an Android app process
(
node::FreeEnvironment()→uv_close()on aUV_SIGNALhandle), libuvresets the signal's disposition to
SIG_DFL. On Android thatsigactioncall is intercepted by ART's
libsigchain, which logs an ERROR-level stacktrace on every worker stop:
ART claims SIGSEGV/SIGBUS/SIGFPE/SIGILL/SIGTRAP/SIGABRT for implicit
null-checks, stack-overflow detection, etc., so resetting one to
SIG_DFLalso clobbers ART's handler — semantically wrong, not just noisy.
Root cause
uv__signal_unregister_handler()unconditionally installsSIG_DFLonce thelast watcher for a signal is removed. libuv never recorded what was installed
before it added its multiplexer — there's a long-standing
/* XXX save old action so we can restore it later on? */TODO at the installsite.
Fix (gated to
__ANDROID__)On Android,
uv__signal_register_handler()captures the previousstruct sigaction(into a per-signum slot) on the genuine"no libuv handler → installed" transition — gated by a
saveflag so theoneshot↔regular re-registration paths never overwrite it with libuv's own
handler.
uv__signal_unregister_handler()then restores that saveddisposition instead of forcing
SIG_DFL, falling back toSIG_DFLonly whennothing was captured.
The whole save/restore path is
#ifdef __ANDROID__; off Android the codepath and behaviour are byte-for-byte unchanged. On Android the restored
handler is ART's (kept by libsigchain), so no
SIG_DFLreset — and nolibsigchain report — occurs. Zero hot-path cost (only the first register /
last unregister per signum); the saved table is touched only under the
existing global signal lock.
Why Android-only
A cross-platform version of this was tried upstream (libuv nodejs#4216) and
reverted (nodejs#4302, refs nodejs#4299) because macOS doesn't propagate
SA_RESETHANDin the old action returned bysigaction()— so the captureddisposition is wrong there. bionic (Android) round-trips the flags correctly,
so gating to
__ANDROID__keeps the fix where it's both needed and correctand leaves macOS/Linux/Windows on stock upstream behaviour. See the upstream
discussion in libuv#5157 (this change floated cross-platform) and libuv issue
nodejs#4299.
Alternatives rejected
SA_RESETHANDbug that got libuv gripe: deprecating fs.existsSync without a replacement that returns a boolean nodejs/node#4216 reverted (fix: workaround for possible V8 bug fixes #4266 nodejs/node#4299). Hence the__ANDROID__gate.rt_sigaction(SIG_DFL)syscall to bypass libsigchain — unsafe: itoverwrites libsigchain's kernel-level handler (the one ART chains behind),
silencing the log by breaking ART's signal handling.
documented "stop watching → default behaviour restored" contract.
Testing
we_get_signals_mixed— the oneshot↔regular path thesavegate guards —signal_pending_on_close,signal_close_loop_alive); non-Android compileswarning-free with stock
SIG_DFLbehaviour preserved.libnode.sofor all four Android ABIs (arm64-v8a, x86_64,armeabi-v7a, x86); the 32-bit pair via the ia32 build-host path.
libsigchain "Setting SIGSEGV to SIG_DFL"count 1 → 0, with the cleanup path confirmedexercised (
FreeEnvironmentruns,libuv loop closed cleanly).Upstreaming
The cross-platform variant is not currently acceptable upstream — it hits
the macOS
oldactbug (nodejs#4299) that reverted nodejs#4216. Tracked in libuv#5157,where an
__ANDROID__-gated version is proposed; the door reopens if asupported macOS (≥ 11) starts round-tripping the flags.
Rollout
Cut a tag (e.g.
dcp/3.2.1) → bumpnode-build'sGIT_TAG→ rebuild theAndroid
node_apizips → bump the URL/SHA indcp-native/externals/node_api/CMakeLists.txt. (The fourandroid-workerjnilibs/*.sohave already been rebuilt from this branch and validated on theemulator.)