diagnostics_channel: fix dangling binding pointer - #65860
Open
TrevorBurnham wants to merge 1 commit into
Open
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
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
TrevorBurnham
marked this pull request as ready for review
September 6, 2026 22:39
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65860 +/- ##
==========================================
+ Coverage 90.18% 90.19% +0.01%
==========================================
Files 771 771
Lines 264619 264626 +7
Branches 50224 50231 +7
==========================================
+ Hits 238643 238682 +39
+ Misses 16959 16949 -10
+ Partials 9017 8995 -22
🚀 New features to boost your workflow:
|
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
2 participants
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.
Fixes: #65858
diagnostics_channel's
Channelreads its subscriber count through a rawBindingData*that's never cleared, so any native holder that outlives environment cleanup reads a destroyed object. That impacts node:sqlite'sDatabaseSync, which holds a strongBaseObjectPtr<Channel>for its lifetime, leading to a potential segfault.This PR clears
binding_data_on everyChannelthe binding owns whenever it gives up that ownership, so the existing null check inHasSubscribers()can do its job.Two changes go to
node:sqliteas well:DatabaseSync::trace_channel_becomes aBaseObjectWeakPtr, matching the conventionpermissiondocuments and follows, whereBindingDatais the sole owner of channels. AndTraceCallbacknow testsAreTraceEventsSuppressed()before the channel, so a suppressed callback doesn't dereference it at all.