diagnostics_channel: fix dangling binding pointer - #65860
Draft
TrevorBurnham wants to merge 1 commit into
Draft
Conversation
Collaborator
|
Review requested:
|
TrevorBurnham
force-pushed
the
dc-clear-channel-binding-data
branch
3 times, most recently
from
September 6, 2026 18:14
ff5512d to
7db0a64
Compare
`Channel` reads its subscriber count through a raw `BindingData*` that was never cleared, so any native holder that outlives environment cleanup reads a destroyed object. The null check in `HasSubscribers()` could not fire, because the pointer was only ever assigned in the constructor. `node:sqlite` holds a strong `BaseObjectPtr<Channel>` for the lifetime of a `DatabaseSync`, which made this reachable from ordinary JavaScript. A statement left mid-step at exit is finalized by the destructor chain after `Environment::RunCleanup()` has destroyed the binding, and `sqlite3_finalize()` invokes the profile callback for such a statement. The result was a segfault at normal process exit; inside a worker it took down the whole process. Clear `binding_data_` on every `Channel` the binding owns whenever it gives up that ownership, both in the destructor and in `PrepareForSerialization()`, so that the existing null check in `HasSubscribers()` does its job. The second check in `Publish()` is now unreachable and is dropped. This protects any holder that is itself a `BaseObject`, and so is destroyed later in the same cleanup. A holder that is not a `BaseObject` still needs a cleanup hook or a weak reference, because `Realm::~Realm()` checks that no `BaseObject`s remain. On the `node:sqlite` side, switch `DatabaseSync::trace_channel_` to a `BaseObjectWeakPtr`, so that it follows the same convention `permission` documents, where `BindingData` is the sole owner of channels. `TraceCallback` already null-checks, so this needs no other change there. Also check `AreTraceEventsSuppressed()` before the channel in `TraceCallback()`, so that a suppressed callback does not dereference it at all. `StatementSync::Finalize()` already suppresses trace events, so the reported path was meant to be a no-op; only the order of the `||` operands took it through the channel first. Fixes: nodejs#65858 Assisted-by: Claude Opus 5 Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
TrevorBurnham
force-pushed
the
dc-clear-channel-binding-data
branch
from
September 6, 2026 21:49
7db0a64 to
4d9852c
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.
Channelreads its subscriber count through a rawBindingData*that is never cleared, so any native holder that outlives environment cleanup reads a destroyed object. The null check inHasSubscribers()cannot fire, becausebinding_data_is only ever assigned in the constructor.node:sqliteholds a strongBaseObjectPtr<Channel>for the lifetime of aDatabaseSync, which makes this reachable from ordinary JavaScript: a statement left mid-step at exit is finalized by the destructor chain afterEnvironment::RunCleanup()has destroyed the binding, andsqlite3_finalize()invokes the profile callback for such a statement. A release build segfaults at normal process exit; a debug build hits theDCHECK(is_valid())inAliasedBufferBase::GetValue(). Inside a worker it takes down the whole process.This clears
binding_data_on everyChannelthe binding owns whenever it gives up that ownership, both in the destructor and inPrepareForSerialization(), so the existing null check inHasSubscribers()does its job. The second check inPublish()becomes unreachable and is dropped. That protects any holder which is itself aBaseObject, and so is destroyed later in the same cleanup; a holder that is not aBaseObjectstill needs a cleanup hook or a weak reference, becauseRealm::~Realm()checks that noBaseObjects remain.The alternative of giving
node:sqlitea cleanup hook, ascrypto's FIPS indicator does, leaves the same trap set for the next native holder that forgets it, so the general fix goes indiagnostics_channel.Two changes go to
node:sqliteas well.DatabaseSync::trace_channel_becomes aBaseObjectWeakPtr, matching the conventionpermissiondocuments and follows, whereBindingDatais the sole owner of channels;TraceCallbackalready null-checks, so that needs no other change. AndTraceCallbacknow testsAreTraceEventsSuppressed()before the channel, so a suppressed callback does not dereference it at all:StatementSync::Finalize()already suppresses trace events, so the reported path was meant to be a no-op, and only the order of the||operands took it through the channel first.The three changes are independent, and each was checked on its own. With only the weak reference, or with only the reordered condition, the
node:sqlitetests pass but theccteststill segfaults, because its holder keeps a strong reference and publishes unsuppressed. With only thediagnostics_channelchange, all of them pass. Thecctestis therefore what guards that change, and it does so in builds configured--without-sqlite; thenode:sqlitetest is the user-visible reproduction of the original report, covering the main-thread and worker teardown paths.Fixes: #65858