mirror of
https://github.com/nodejs/node.git
synced 2026-11-09 15:11:11 -05:00
Trevor Burnham
45ae12c3aa
`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>
52 lines
1.5 KiB
JavaScript
52 lines
1.5 KiB
JavaScript
'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']);
|
|
}
|