From 4d9852ca83e95f97d804ce397ed7f896138cfef9 Mon Sep 17 00:00:00 2001 From: Trevor Burnham Date: Sun, 6 Sep 2026 11:43:15 -0400 Subject: [PATCH] diagnostics_channel: fix dangling binding pointer `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` 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: https://github.com/nodejs/node/issues/65858 Assisted-by: Claude Opus 5 Signed-off-by: Trevor Burnham --- src/node_diagnostics_channel.cc | 18 ++++- src/node_diagnostics_channel.h | 5 ++ src/node_sqlite.cc | 7 +- src/node_sqlite.h | 4 +- test/cctest/test_diagnostics_channel.cc | 73 +++++++++++++++++++ .../test-sqlite-diagnostic-channel-exit.js | 51 +++++++++++++ 6 files changed, 153 insertions(+), 5 deletions(-) create mode 100644 test/parallel/test-sqlite-diagnostic-channel-exit.js diff --git a/src/node_diagnostics_channel.cc b/src/node_diagnostics_channel.cc index 2593f6eab90f..2cc3309542ff 100644 --- a/src/node_diagnostics_channel.cc +++ b/src/node_diagnostics_channel.cc @@ -44,6 +44,21 @@ BindingData::BindingData(Realm* realm, subscribers_.MakeWeak(); } +void BindingData::DetachChannels() { + // A native holder may keep a strong reference to a Channel that outlives this + // binding's ownership of it, and the binding is destroyed during environment + // cleanup while BaseObject destructors still run after it. Drop the + // back-pointer so that the null check in HasSubscribers() stops those holders + // from reading the subscribers_ array of a binding that no longer owns them. + for (auto& channel : channels_) { + if (channel) channel->binding_data_ = nullptr; + } +} + +BindingData::~BindingData() { + DetachChannels(); +} + void BindingData::MemoryInfo(MemoryTracker* tracker) const { tracker->TrackField("subscribers", subscribers_); } @@ -104,6 +119,7 @@ bool BindingData::PrepareForSerialization(Local context, internal_field_info_->subscribers_capacity = subscribers_.Length(); link_callback_.Reset(); channel_wrap_template_.Reset(); + DetachChannels(); channels_.clear(); return true; } @@ -281,8 +297,6 @@ void Channel::CachePublishFn(Isolate* isolate, Local js_channel) { void Channel::Publish(Environment* env, Local message) { if (!HasSubscribers()) return; - if (binding_data_ == nullptr) return; - if (js_channel_.IsEmpty()) return; // Publishing is not possible during shutdown or GC. diff --git a/src/node_diagnostics_channel.h b/src/node_diagnostics_channel.h index c8c1a79994b2..b1da5fcee3da 100644 --- a/src/node_diagnostics_channel.h +++ b/src/node_diagnostics_channel.h @@ -31,6 +31,7 @@ class BindingData : public SnapshotableObject { BindingData(Realm* realm, v8::Local wrap, InternalFieldInfo* info = nullptr); + ~BindingData() override; SERIALIZABLE_OBJECT_METHODS() SET_BINDING_ID(diagnostics_channel_binding_data) @@ -70,6 +71,10 @@ class BindingData : public SnapshotableObject { static void RegisterExternalReferences(ExternalReferenceRegistry* registry); private: + // Clears the back-pointer of every Channel in channels_, so that a Channel + // this binding no longer owns cannot read through it. + void DetachChannels(); + InternalFieldInfo* internal_field_info_ = nullptr; std::unordered_map channel_status_callbacks_; }; diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc index 76accf1c2731..1545d0f24a80 100644 --- a/src/node_sqlite.cc +++ b/src/node_sqlite.cc @@ -2935,8 +2935,11 @@ int DatabaseSync::TraceCallback(unsigned int type, Environment* env = db->env(); diagnostics_channel::Channel* ch = db->trace_channel_.get(); - if (ch == nullptr || !ch->HasSubscribers() || - db->AreTraceEventsSuppressed()) { + // Checked before the channel, so that a suppressed callback does not + // dereference it at all. Statement finalization suppresses trace events, and + // that is the path SQLite takes during environment teardown. + if (db->AreTraceEventsSuppressed() || ch == nullptr || + !ch->HasSubscribers()) { return 0; } diff --git a/src/node_sqlite.h b/src/node_sqlite.h index 306a47f6c47f..6880197b329a 100644 --- a/src/node_sqlite.h +++ b/src/node_sqlite.h @@ -343,7 +343,9 @@ class DatabaseSync : public BaseObject { std::set backups_; std::unordered_set sessions_; std::unordered_set statements_; - BaseObjectPtr trace_channel_; + // Weak ref: BindingData is the sole owner of Channels, so a strong ref here + // would outlive the binding that backs it during environment cleanup. + BaseObjectWeakPtr trace_channel_; friend class DatabaseSyncLimits; friend class Session; diff --git a/test/cctest/test_diagnostics_channel.cc b/test/cctest/test_diagnostics_channel.cc index d4d1fd1c9fac..6932a27640e6 100644 --- a/test/cctest/test_diagnostics_channel.cc +++ b/test/cctest/test_diagnostics_channel.cc @@ -1,5 +1,6 @@ #include "node_diagnostics_channel.h" +#include "base_object-inl.h" #include "gtest/gtest.h" #include "node_test_fixture.h" @@ -302,3 +303,75 @@ TEST_F(DiagnosticsChannelTest, NativeChannelsGrowSubscriberStorage) { " globalThis.__lastSubscriber);"); EXPECT_TRUE(last->HasSubscribers()); } + +// Mirrors how node:sqlite holds a Channel: a strong reference kept for the +// lifetime of a BaseObject, which is destroyed during environment cleanup +// after the BindingData that owns the channel is already gone. +class ChannelHolder : public node::BaseObject { + public: + ChannelHolder(node::Environment* env, + v8::Local obj, + BaseObjectPtr channel) + : BaseObject(env, obj), channel_(std::move(channel)) {} + + ~ChannelHolder() override { + destroyed = true; + // Would read the destroyed BindingData if the back-pointer were stale. + had_subscribers = channel_->HasSubscribers(); + } + + static bool destroyed; + static bool had_subscribers; + + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(ChannelHolder) + SET_SELF_SIZE(ChannelHolder) + + private: + BaseObjectPtr channel_; +}; + +bool ChannelHolder::destroyed = false; +bool ChannelHolder::had_subscribers = false; + +// A Channel whose holder outlives the BindingData must report no subscribers +// rather than reading the destroyed binding's subscriber array. +TEST_F(DiagnosticsChannelTest, ChannelOutlivingBindingHasNoSubscribers) { + const v8::HandleScope handle_scope(isolate_); + Argv argv; + + ChannelHolder::destroyed = false; + ChannelHolder::had_subscribers = false; + + { + Env env{handle_scope, argv}; + + SetProcessExitHandler(*env, [&](node::Environment* env_, int exit_code) { + EXPECT_EQ(exit_code, 0); + node::Stop(*env); + }); + + node::LoadEnvironment( + *env, + "const dc = require('diagnostics_channel');" + "dc.subscribe('test:cctest:outlives-binding', () => {});"); + + auto channel = Channel::Get(*env, "test:cctest:outlives-binding"); + ASSERT_TRUE(channel); + ASSERT_TRUE(channel->HasSubscribers()); + + v8::Local context = (*env)->context(); + v8::Local obj = + node::BaseObject::MakeLazilyInitializedJSTemplate(*env) + ->GetFunction(context) + .ToLocalChecked() + ->NewInstance(context) + .ToLocalChecked(); + // MakeBaseObject leaves the holder a strong root, so it survives until the + // environment is torn down, like a DatabaseSync still reachable at exit. + node::MakeBaseObject(*env, obj, channel); + } + + EXPECT_TRUE(ChannelHolder::destroyed); + EXPECT_FALSE(ChannelHolder::had_subscribers); +} diff --git a/test/parallel/test-sqlite-diagnostic-channel-exit.js b/test/parallel/test-sqlite-diagnostic-channel-exit.js new file mode 100644 index 000000000000..0fc51597d5cc --- /dev/null +++ b/test/parallel/test-sqlite-diagnostic-channel-exit.js @@ -0,0 +1,51 @@ +'use strict'; + +// A statement left unfinished at exit is finalized by the destructor chain, +// which runs after the diagnostics channel binding has been destroyed. SQLite +// invokes the profile callback for such a statement, so the channel must not +// read through the destroyed binding. + +const common = require('../common'); +common.skipIfSQLiteMissing(); + +const { spawnSyncAndExitWithoutError } = require('../common/child_process'); + +// Stays reachable for the rest of the process so the statement is finalized +// during teardown rather than by the garbage collector. +const keepAlive = []; + +function leaveStatementUnfinished() { + const dc = require('node:diagnostics_channel'); + const { DatabaseSync } = require('node:sqlite'); + + dc.subscribe('sqlite.db.query', () => {}); + + const db = new DatabaseSync(':memory:'); + db.exec('CREATE TABLE t (x INTEGER)'); + const insert = db.prepare('INSERT INTO t VALUES (?)'); + for (let i = 0; i < 200; i++) insert.run(i); + + const iterator = db.prepare('SELECT x FROM t').iterate(); + iterator.next(); + keepAlive.push({ db, iterator }); +} + +switch (process.argv[2]) { + case 'main': + leaveStatementUnfinished(); + break; + + case 'worker': { + const { Worker, isMainThread } = require('node:worker_threads'); + if (isMainThread) { + new Worker(__filename, { argv: ['worker'] }); + } else { + leaveStatementUnfinished(); + } + break; + } + + default: + spawnSyncAndExitWithoutError(process.execPath, [__filename, 'main']); + spawnSyncAndExitWithoutError(process.execPath, [__filename, 'worker']); +}