fix(usb): stop finalizer from dereferencing tsfn-owned callbackRef during shutdown (#790)

src/binding/database/database.cpp

@@ -1614,21 +1614,21 @@ napi_value Database::GetUserSharedBuffer(napi_env env, napi_callback_info info)
 	std::string keyStr(key + keyStart, keyEnd - keyStart);
 
 	// if we have a callback, add it as a listener
-	napi_ref callbackRef = nullptr;
+	std::shared_ptr<ListenerCallback> listener;
 	napi_valuetype type;
 	NAPI_STATUS_THROWS(::napi_typeof(env, argv[2], &type));
 	if (type != napi_undefined) {
 		if (type == napi_function) {
 			DEBUG_LOG("Database::GetUserSharedBuffer key start=%u end=%u:", keyStart, keyEnd);
 			DEBUG_LOG_KEY_LN(keyStr);
-			callbackRef = (*dbHandle)->descriptor->addListener(env, keyStr, argv[2], *dbHandle);
+			listener = (*dbHandle)->descriptor->addListener(env, keyStr, argv[2], *dbHandle);
 		} else {
 			::napi_throw_error(env, nullptr, "Callback must be a function");
 			return nullptr;
 		}
 	}
 
-	return (*dbHandle)->descriptor->getUserSharedBuffer(env, keyStr, *dbHandle, argv[1], callbackRef);
+	return (*dbHandle)->descriptor->getUserSharedBuffer(env, keyStr, *dbHandle, argv[1], std::move(listener));
 }
 
 /**

src/binding/database/db_descriptor.cpp

@@ -1569,19 +1569,16 @@ static void userSharedBufferFinalize(napi_env env, void* unusedData, void* hint)
 
 	if (auto dbHandle = finalizeData->dbHandle.lock()) {
 		DEBUG_LOG("userSharedBufferFinalize GC'd dbHandle=%p\n", dbHandle.get());
-		if (finalizeData->callbackRef) {
-			napi_value callback;
-			if (::napi_get_reference_value(env, finalizeData->callbackRef, &callback) == napi_ok) {
+		// Remove this buffer's listener by identity, never by its napi_ref: the
+		// ref is owned by the listener's tsfn (see UserSharedBufferFinalizeData),
+		// so re-resolving it here was the shutdown UAF in #790. removeListener is
+		// a no-op if the listener is already gone (close / env teardown).
+		if (auto listener = finalizeData->listener.lock()) {
+			if (dbHandle->descriptor) {
 				DEBUG_LOG("%p userSharedBufferFinalize removing listener for key:", dbHandle.get());
 				DEBUG_LOG_KEY_LN(finalizeData->key);
-				if (dbHandle->descriptor) {
-					DEBUG_LOG("%p userSharedBufferFinalize descriptor is still alive %p", dbHandle.get(), dbHandle->descriptor.get());
-					dbHandle->descriptor->removeListener(env, finalizeData->key, callback);
-				} else {
-					DEBUG_LOG("%p userSharedBufferFinalize descriptor is not alive %p", dbHandle.get(), dbHandle->descriptor.get());
-				}
+				dbHandle->descriptor->removeListener(finalizeData->key, listener);
 			}
-			finalizeData->callbackRef = nullptr;
 		}
 	} else {
 		DEBUG_LOG("userSharedBufferFinalize GC'd dbHandle was already destroyed for key:");
@@ -1611,7 +1608,7 @@ napi_value DBDescriptor::getUserSharedBuffer(
 	std::string& key,
 	std::shared_ptr<DBHandle> dbHandle,
 	napi_value defaultBuffer,
-	napi_ref callbackRef
+	std::shared_ptr<ListenerCallback> listener
 ) {
 	bool isArrayBuffer;
 	NAPI_STATUS_THROWS(::napi_is_arraybuffer(env, defaultBuffer, &isArrayBuffer));
@@ -1655,7 +1652,7 @@ napi_value DBDescriptor::getUserSharedBuffer(
 		std::weak_ptr<DBHandle>(dbHandle),
 		std::weak_ptr<ColumnFamilyDescriptor>(dbHandle->columnDescriptor),
 		userSharedBuffer,
-		callbackRef
+		std::weak_ptr<ListenerCallback>(listener)
 	);
 
 	napi_value result;
@@ -1673,7 +1670,7 @@ napi_value DBDescriptor::getUserSharedBuffer(
 /**
  * Adds an event listener to this descriptor's event emitter.
  */
-napi_ref DBDescriptor::addListener(
+std::shared_ptr<ListenerCallback> DBDescriptor::addListener(
 	napi_env env,
 	std::string& key,
 	napi_value callback,
@@ -1702,6 +1699,10 @@ napi_value DBDescriptor::removeListener(napi_env env, std::string& key, napi_val
 	return this->events.removeListener(env, key, callback);
 }
 
+void DBDescriptor::removeListener(const std::string& key, const std::shared_ptr<ListenerCallback>& target) {
+	this->events.removeListener(key, target);
+}
+
 void DBDescriptor::removeListenersByOwner(DBHandle* owner) {
 	this->events.removeListenersByOwner(static_cast<void*>(owner));
 }

src/binding/database/db_descriptor.h

@@ -370,21 +370,22 @@ struct DBDescriptor final : public std::enable_shared_from_this<DBDescriptor> {
 	 * @param key The key of the user shared buffer.
 	 * @param defaultBuffer The default buffer to use if the user shared buffer does
 	 * not exist.
-	 * @param callbackRef An optional callback reference to remove the listener when
-	 * the user shared buffer is garbage collected.
+	 * @param listener An optional listener (from addListener) to remove when the
+	 * user shared buffer is garbage collected.
 	 */
 	napi_value getUserSharedBuffer(
 		napi_env env,
 		std::string& key,
 		std::shared_ptr<DBHandle> dbHandle,
 		napi_value defaultBuffer,
-		napi_ref callbackRef = nullptr
+		std::shared_ptr<ListenerCallback> listener = nullptr
 	);
 
-	napi_ref addListener(napi_env env, std::string& key, napi_value callback, std::weak_ptr<DBHandle> owner);
+	std::shared_ptr<ListenerCallback> addListener(napi_env env, std::string& key, napi_value callback, std::weak_ptr<DBHandle> owner);
 	bool notify(std::string key, ListenerData* data);
 	napi_value listeners(napi_env env, std::string& key);
 	napi_value removeListener(napi_env env, std::string& key, napi_value callback);
+	void removeListener(const std::string& key, const std::shared_ptr<ListenerCallback>& target);
 	void removeListenersByOwner(DBHandle* owner);
 	void removeListenersByEnv(napi_env env);
 
@@ -528,21 +529,27 @@ struct UserSharedBufferData final {
  * until JS releases every retained ArrayBuffer for the key. The weak pointers
  * to `DBHandle` / `ColumnFamilyDescriptor` are used for opportunistic cleanup
  * (removing listeners, erasing map entries) when those are still alive.
+ *
+ * The listener is held as a `weak_ptr` (not the raw `napi_ref`): the ref's
+ * ownership belongs to the listener's threadsafe function, which deletes it
+ * once the listener is torn down. A `weak_ptr` lets the finalizer remove the
+ * listener by identity only while it is still live, without ever dereferencing
+ * a ref it does not own (HarperFast/rocksdb-js#790).
  */
 struct UserSharedBufferFinalizeData final {
 	std::string key;
 	std::weak_ptr<DBHandle> dbHandle;
 	std::weak_ptr<ColumnFamilyDescriptor> columnDescriptor;
 	std::shared_ptr<UserSharedBufferData> sharedData;
-	napi_ref callbackRef;
+	std::weak_ptr<ListenerCallback> listener;
 
 	UserSharedBufferFinalizeData(
 		const std::string& k,
 		std::weak_ptr<DBHandle> d,
 		std::weak_ptr<ColumnFamilyDescriptor> c,
 		std::shared_ptr<UserSharedBufferData> data,
-		napi_ref callbackRef = nullptr
-	) : key(k), dbHandle(d), columnDescriptor(c), sharedData(std::move(data)), callbackRef(callbackRef) {}
+		std::weak_ptr<ListenerCallback> listener = {}
+	) : key(k), dbHandle(d), columnDescriptor(c), sharedData(std::move(data)), listener(std::move(listener)) {}
 };
 
 /**

src/binding/napi/event_emitter.cpp

@@ -163,7 +163,7 @@ ListenerData* serializeListenerArgs(napi_env env, napi_value value) {
 	return data;
 }
 
-napi_ref EventEmitter::addListener(
+std::shared_ptr<ListenerCallback> EventEmitter::addListener(
 	napi_env env,
 	const std::string& key,
 	napi_value callback,
@@ -265,7 +265,7 @@ napi_ref EventEmitter::addListener(
 	DEBUG_LOG_KEY(key);
 	DEBUG_LOG_MSG(" (listeners=%zu)\n", it->second.size());
 
-	return callbackRef;
+	return listenerCallback;
 }
 
 bool EventEmitter::notify(const std::string& key, ListenerData* data) {
@@ -443,6 +443,35 @@ napi_value EventEmitter::removeListener(napi_env env, const std::string& key, na
 	return result;
 }
 
+void EventEmitter::removeListener(const std::string& key, const std::shared_ptr<ListenerCallback>& target) {
+	if (!target) {
+		return;
+	}
+
+	std::lock_guard<std::mutex> lock(this->mutex);
+	auto it = this->callbacks.find(key);
+	if (it == this->callbacks.end()) {
+		return;
+	}
+
+	auto& listeners = it->second;
+	auto listener = std::find(listeners.begin(), listeners.end(), target);
+	if (listener != listeners.end()) {
+		// Release the tsfn first (queues napi_ref deletion on the JS thread via
+		// the tsfn finalizer), then erase. No napi_ref is dereferenced here, so
+		// this is safe from a finalizer during teardown.
+		releaseListenerResources(**listener);
+		listeners.erase(listener);
+		this->listenerCount.fetch_sub(1, std::memory_order_relaxed);
+		DEBUG_LOG("%p EventEmitter::removeListener (by identity) removed listener for key:", this);
+		DEBUG_LOG_KEY_LN(key);
+	}
+
+	if (listeners.empty()) {
+		this->callbacks.erase(it);
+	}
+}
+
 void EventEmitter::removeListenersByOwner(void* owner) {
 	std::lock_guard<std::mutex> lock(this->mutex);
 

src/binding/napi/event_emitter.h

@@ -158,9 +158,12 @@ class EventEmitter final {
 	 * @param callback The JS function to invoke when the event is emitted.
 	 * @param owner Optional weak reference to the owning object. Defaults to
 	 *              an empty weak_ptr for ownerless listeners (global emitter).
-	 * @returns The napi_ref backing the callback, suitable for later removal.
+	 * @returns The registered listener, or nullptr on failure. Callers that need
+	 *          a lifetime-safe removal handle (e.g. the user-shared-buffer
+	 *          finalizer) hold a std::weak_ptr to it; the backing napi_ref is
+	 *          reachable via `->callbackRef` while the listener is live.
 	 */
-	napi_ref addListener(
+	std::shared_ptr<ListenerCallback> addListener(
 		napi_env env,
 		const std::string& key,
 		napi_value callback,
@@ -205,6 +208,16 @@ class EventEmitter final {
 	 */
 	napi_value removeListener(napi_env env, const std::string& key, napi_value callback);
 
+	/**
+	 * Removes a specific listener by identity (the shared_ptr returned from
+	 * addListener) for the given key. Env-free and does not touch the listener's
+	 * napi_ref, so it is safe to call from a finalizer during env/process
+	 * teardown -- the tsfn (and, through it, the napi_ref) is released here, but
+	 * the ref itself is deleted later on the JS thread by the tsfn finalizer.
+	 * A no-op if `target` is null or no longer registered.
+	 */
+	void removeListener(const std::string& key, const std::shared_ptr<ListenerCallback>& target);
+
 	/**
 	 * Removes all listeners owned by the given raw pointer. The pointer must
 	 * match the address that was held by the listener's owning shared_ptr at

test/fixtures/fork-user-shared-buffer-shutdown.mts

@@ -0,0 +1,62 @@
+/**
+ * Repro for the user-shared-buffer finalize use-after-free observed during
+ * Harper shutdown (ctrl-C) on macOS. The reported crash:
+ *
+ *   userSharedBufferFinalize -> napi_get_reference_value  (EXC_BAD_ACCESS)
+ *   ... BufferFinalizer::FinalizeBufferCallback
+ *   ... Environment::RunCleanup
+ *   ... FreeEnvironment (worker / process teardown)
+ *
+ * Scenario (a graceful shutdown while a shared buffer is still referenced):
+ * 1. Open a DB and get one or more user-shared ArrayBuffers WITH a callback, so
+ *    a listener (napi_ref + threadsafe function) is registered for each.
+ * 2. Shut the DB down (`db.close()` -> removeListenersByOwner releases each
+ *    listener's tsfn, whose finalizer deletes the backing napi_ref).
+ * 3. Keep the ArrayBuffers referenced and let the process exit NATURALLY (no
+ *    `process.exit()`), so Environment::RunCleanup finalizes the still-live
+ *    external ArrayBuffers.
+ *
+ * `userSharedBufferFinalize` then reads `finalizeData->callbackRef` via
+ * `napi_get_reference_value` -- but that ref's ownership was transferred to the
+ * (now torn-down) listener tsfn in `addListener`, so it has already been
+ * deleted: a use-after-free. On a plain build the freed slot usually reads back
+ * benign; under an instrumented allocator (Guard Malloc on macOS, ASan on
+ * Linux) it faults, which is how the test surfaces it deterministically.
+ *
+ * Exit 0 = survived (expected once the finalizer stops touching the borrowed
+ * callbackRef); a crash/non-zero exit = the UAF reproduced.
+ */
+import { RocksDatabase } from '../../src/index.ts';
+import { mkdirSync } from 'node:fs';
+
+const dbPath = process.argv[2];
+
+if (!dbPath) {
+	console.error('Usage: fork-user-shared-buffer-shutdown.mts <dbPath>');
+	process.exit(1);
+}
+
+mkdirSync(dbPath, { recursive: true });
+
+const db = RocksDatabase.open(dbPath, { name: 'shutdown-cf' });
+
+// Retain several buffers-with-callbacks so they are still "in use" at shutdown.
+const held: ArrayBuffer[] = [];
+for (let i = 0; i < 16; i++) {
+	const buffer = db.getUserSharedBuffer(`shutdown-key-${i}`, new ArrayBuffer(8), {
+		callback() {
+			// presence of the listener is what matters, not its body
+		},
+	});
+	new DataView(buffer).setBigUint64(0, 1n);
+	held.push(buffer);
+}
+// Keep them reachable past `db.close()` so teardown must finalize them.
+(globalThis as unknown as { __heldBuffers?: ArrayBuffer[] }).__heldBuffers = held;
+
+// Graceful shutdown: tear the DB down while the buffers are still referenced.
+db.close();
+
+// Intentionally no `process.exit()`: allow a natural exit so the finalizers run
+// during Environment::RunCleanup -- the crashing path from the report.
+console.log('SUCCESS');

test/user-shared-buffer-shutdown.test.ts

@@ -0,0 +1,87 @@
+import { generateDBPath } from './lib/util.ts';
+import { spawn } from 'node:child_process';
+import { existsSync, rmSync } from 'node:fs';
+import { join } from 'node:path';
+import { describe, expect, it } from 'vitest';
+
+/**
+ * Regression repro for the user-shared-buffer finalize use-after-free seen on
+ * Harper shutdown (ctrl-C) on macOS:
+ *
+ *   userSharedBufferFinalize -> napi_get_reference_value  (SIGSEGV)
+ *   ... Environment::RunCleanup -> FreeEnvironment (teardown)
+ *
+ * `userSharedBufferFinalize` reads the buffer's borrowed `callbackRef`, but that
+ * napi_ref is owned by the listener's threadsafe function and has already been
+ * deleted once the DB (and its listeners) were torn down at shutdown. See
+ * `test/fixtures/fork-user-shared-buffer-shutdown.mts` for the full scenario.
+ *
+ * Runs in a child process because a regression SIGSEGVs rather than throwing a
+ * catchable error. On a plain build the freed napi_ref slot usually reads back
+ * benign, so the child is run under Apple's Guard Malloc to make the UAF fault
+ * deterministically. This is macOS-only: Guard Malloc is macOS-specific, ASan
+ * does not work locally on recent macOS (see AGENTS.md), and a plain run on any
+ * other platform is a false green (and can flake if the freed ref slot gets
+ * reused), so it is skipped there rather than run without instrumentation.
+ */
+describe('User shared buffer shutdown use-after-free repro (child process)', () => {
+	const fixturePath = join(__dirname, 'fixtures', 'fork-user-shared-buffer-shutdown.mts');
+	const GUARD_MALLOC = '/usr/lib/libgmalloc.dylib';
+
+	// macOS + Guard Malloc + Node only (Bun/Deno don't run this fixture under
+	// Node's native type stripping the same way).
+	const runnable =
+		process.platform === 'darwin' &&
+		existsSync(GUARD_MALLOC) &&
+		!process.versions.bun &&
+		!process.versions.deno;
+
+	it.runIf(runnable)(
+		'should survive shutdown while user shared buffers with callbacks are still in use',
+		async () => {
+			// Loop: teardown-order UAFs only fault on a fraction of runs even under
+			// Guard Malloc, so a single shot can mask a regression.
+			for (let i = 0; i < 5; i++) {
+				const { code, signal } = await spawnShutdownRepro(fixturePath);
+				expect(signal, `iteration=${i}`).toBeNull();
+				expect(code, `iteration=${i}`).toBe(0);
+			}
+		},
+		30_000
+	);
+});
+
+function spawnShutdownRepro(
+	fixturePath: string
+): Promise<{ code: number | null; signal: NodeJS.Signals | null }> {
+	return new Promise((resolve, reject) => {
+		const dbPath = generateDBPath();
+		// Fault immediately on the use-after-free instead of reading benign freed
+		// memory. MallocScribble poisons freed blocks so a stale read is more
+		// likely to be caught even outside the guard page.
+		const env = {
+			...process.env,
+			DYLD_INSERT_LIBRARIES: '/usr/lib/libgmalloc.dylib',
+			MallocScribble: '1',
+		};
+
+		const child = spawn(process.execPath, [fixturePath, dbPath], { env });
+
+		let stderr = '';
+		child.stderr?.on('data', (chunk) => {
+			stderr += chunk.toString();
+		});
+		child.on('close', (code, signal) => {
+			if (code !== 0 || signal) {
+				console.error(`Shutdown repro stderr:\n${stderr}`);
+			}
+			// The child creates the DB dir; clean it up so repeated runs don't
+			// accumulate databases under the temp dir (honoring KEEP_FILES).
+			if (!process.env.KEEP_FILES) {
+				rmSync(dbPath, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
+			}
+			resolve({ code, signal });
+		});
+		child.on('error', reject);
+	});
+}