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<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: https://github.com/nodejs/node/issues/65858
Assisted-by: Claude Opus 5
Signed-off-by: Trevor Burnham <trevorburnham@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65860
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

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> 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<Object> js_channel) {
 void Channel::Publish(Environment* env, Local<Value> message) {
   if (!HasSubscribers()) return;
 
-  if (binding_data_ == nullptr) return;
-
   if (js_channel_.IsEmpty()) return;
 
   // Publishing is not possible during shutdown or GC.

src/node_diagnostics_channel.h

@@ -31,6 +31,7 @@ class BindingData : public SnapshotableObject {
   BindingData(Realm* realm,
               v8::Local<v8::Object> 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<uint32_t, ChannelStatusCallback> channel_status_callbacks_;
 };

src/node_sqlite.cc

@@ -3424,8 +3424,11 @@ int Database::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;
   }
 

src/node_sqlite.h

@@ -354,7 +354,9 @@ class Database : public BaseObject {
   std::set<BackupJob*> backups_;
   std::unordered_set<Session*> sessions_;
   std::unordered_set<Statement*> statements_;
-  BaseObjectPtr<diagnostics_channel::Channel> 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<diagnostics_channel::Channel> trace_channel_;
 
   friend class UserDefinedFunction;
   friend class CustomAggregate;

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<v8::Object> obj,
+                BaseObjectPtr<Channel> 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> 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<v8::Context> context = (*env)->context();
+    v8::Local<v8::Object> 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<ChannelHolder>(*env, obj, channel);
+  }
+
+  EXPECT_TRUE(ChannelHolder::destroyed);
+  EXPECT_FALSE(ChannelHolder::had_subscribers);
+}

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 { Database } = require('node:sqlite');
+
+  dc.subscribe('sqlite.db.query', () => {});
+
+  const db = new Database(':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']);
+}