fix: prevent SIGSEGV on worker exit when wrappers survive into final GC#1477
Open
tstone-1 wants to merge 1 commit into
Open
fix: prevent SIGSEGV on worker exit when wrappers survive into final GC#1477tstone-1 wants to merge 1 commit into
tstone-1 wants to merge 1 commit into
Conversation
Workers that opened a Database and exited abruptly (uncaught throw or
process.exit) could segfault during isolate teardown when the addon
was loaded only in the worker thread. The crash happened inside
v8::internal::GlobalHandles::InvokeFirstPassWeakCallbacks during the
final GC that runs as part of Isolate::Deinit -> CppHeap::
StartDetachingIsolate, jumping into unmapped memory below libnode.
Root cause: every wrapper (Database, Statement, Backup,
StatementIterator) inherits from node::ObjectWrap, which arms a V8
weak callback whose function pointer is inlined into the addon's own
code segment (ObjectWrap::WeakCallback in node_object_wrap.h). When a
worker isolate is the only loader of the addon, Node may unload
(dlclose) the .node file between AddEnvironmentCleanupHook firing and
the final teardown GC. If any wrapper is still weak-armed at that
point, V8 invokes a function pointer into an already-unmapped page.
The crash report confirms this: better_sqlite3.node is absent from the
loaded-image list at crash time, and the faulting address lies in
unmapped memory just below libnode.
Fix:
1. Track every live ObjectWrap-derived instance in Addon::wrappers.
Each wrapper class registers itself on construction and removes
itself on destruction. Statement, Backup, and StatementIterator
now also store an Addon* directly so their destructors can erase
from the set without dereferencing a possibly-freed parent.
2. In Addon::Cleanup (the env cleanup hook):
- Close sqlite resources of any open databases via CloseHandles(),
which cascades into child Statement and Backup handles.
- For every wrapper still alive in Addon::wrappers, manually call
ClearWeak() + Reset() on its persistent handle. Because every
constructed-but-not-destructed wrapper is in this set, this
disarms every armed callback in the addon - reachable or not.
After Cleanup returns no bsqlite weak callback is armed, so V8's
later teardown GC has nothing to invoke into the about-to-be-unloaded
binary.
Verified on macOS arm64, Node v26, against the issue's exact repro:
5/5 SIGSEGV before patch, 30/30 clean exits after across 6 scenarios
(unreachable throw, unreachable process.exit, rooted Database,
rooted Statement, orphaned Statement after db.close(), natural
completion). Existing test suite still passes (311/311).
Fixes WiseLibs#1476
tstone-1
force-pushed
the
fix-worker-exit-segfault-1476
branch
from
33b149e to
7f63854
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
1 participant
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
Databaseand exited abruptly (uncaught throw orprocess.exit) would SIGSEGV during isolate teardown when better-sqlite3 was loaded only in the worker thread.Database,Statement,Backup,StatementIterator) inherits fromnode::ObjectWrap, whose V8 weak callback (ObjectWrap::WeakCallbackfromnode_object_wrap.h) is inlined into the addon's own.nodebinary. When the worker isolate is the only loader, Node unloads the binary betweenAddEnvironmentCleanupHookfiring and the final GC insideIsolate::Deinit→CppHeap::StartDetachingIsolate, leaving V8 to invoke a function pointer into now-unmapped memory. The macOS crash report confirmsbetter_sqlite3.nodeis absent from the loaded-image list and the faulting address sits in the unmapped gap just belowlibnode.ObjectWrap-derived instance in a newAddon::wrappersset. Each wrapper class registers on construction and removes itself on destruction.Statement,Backup, andStatementIteratornow also store anAddon*directly so their destructors can erase without dereferencing a possibly-freed parent.Addon::Cleanup, close sqlite resources of any open databases (Database::CloseHandles()cascades into childStatementandBackuphandles), then manuallyClearWeak()+Reset()every wrapper still alive inAddon::wrappers. Because the set contains every constructed-but-not-destructed wrapper, this disarms every armed callback in the addon — reachable or not.After
Cleanupreturns, no bsqlite weak callback is armed, so V8's later teardown GC has nothing to invoke into the about-to-be-unloaded binary.Test plan
better-sqlite3@12.10.0,typeorm@1.0.0: 5/5 SIGSEGV before patch, 30/30 clean worker exits after across the variants below (5 iterations × 6 scenarios).new Database(':memory:')followed by sync throw.new Database(':memory:')followed byprocess.exit(0).Databasethen throw.Database+Statementthen throw.Statement(db.close()then throw, statement still rooted onglobalThis).exit).npm test→ 311/311 passing on macOS arm64.