diagnostics_channel: fix dangling binding pointer - #65860
Open
TrevorBurnham wants to merge 1 commit into
Open
TrevorBurnham wants to merge 1 commit into
TrevorBurnham wants to merge 1 commit into
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
c++
TrevorBurnham
force-pushed
the
dc-clear-channel-binding-data
branch
4 times, most recently
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.36% 90.37%
========================================
Files 790 790
Lines 274657 274816 +159
Branches 52587 52638 +51
========================================
+ Hits 248207 248373 +166
+ Misses 16935 16923 -12
- Partials 9515 9520 +5
🚀 New features to boost your workflow:
|
Flarna
added
the
diagnostics_channel
bengl
approved these changes
Sep 23, 2026
bengl
added
the
request-ci
panva
added
the
author ready
github-actions
Bot
removed
the
request-ci
This comment was marked as outdated.
This comment was marked as outdated.
panva
added
the
resume-ci
github-actions
Bot
removed
the
resume-ci
This comment was marked as outdated.
This comment was marked as outdated.
panva
added
the
resume-ci
github-actions
Bot
removed
the
resume-ci
Collaborator
Flarna
approved these changes
Sep 25, 2026
github-actions
Bot
removed
the
author ready
Contributor
|
This pull request has conflicts with its base branch, removing the |
`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 `Database`, 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 `Database::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. `Statement::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 26, 2026 21:04
4d9852c to
35ed41b
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
5 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'sDatabase, 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:Database::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.