unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
nodejs/node
mid9/10
error-handlingsecuritymemory-managementtesting-strategy

sqlite: re-validate database state after reading options

Project: nodejs/node | Language: C++/JavaScript | Concepts: error-handling, security, memory-management, testing-strategy | Difficulty: mid
Link: https://github.com/nodejs/node/pull/65595

The problem

Several methods in the DatabaseSync class checked if the SQLite database was open before reading options from a user-supplied object. However, JavaScript getters on that options object could execute arbitrary code, such as closing the database, after the check but before the database was actually used. This could lead to use-after-close bugs, including segmentation faults or incorrect errors. Additionally, some methods did not properly validate the type of certain user-supplied values (e.g., function arity), and there were edge cases where buffer resizing could result in uninitialized memory being handed to SQLite.

Why it's non-trivial

A naive fix might only check the database state once, before reading user-supplied options. However, because JavaScript getters can execute code with side effects (like closing the database or resizing buffers), code must defensively re-validate state after all user code has run, but before using any native resources. Failing to do so can lead to serious bugs, including memory corruption, security vulnerabilities, or misleading errors.

The approach

1. Re-check database state after reading options

After reading options (and thus potentially running arbitrary user code), the code now re-validates that the database is still open:

+  // Reading the options bag above can run user JavaScript through a property
+  // getter, which may have closed the database since it was checked.
+  THROW_AND_RETURN_ON_BAD_STATE(env, !db->IsOpen(), "database is not open");

This pattern is applied to all affected methods: Prepare, CustomFunction, Deserialize, AggregateFunction, CreateSession, Backup, and ApplyChangeset. The check is always placed after any options or property getters are accessed, but before any SQLite API is called.

2. Validate function arity types

Previously, the code assumed that function .length properties were always integers, but this is not guaranteed in JavaScript. Now, the code explicitly checks the type:

+    if (!js_len->IsInt32()) {
+      THROW_ERR_INVALID_ARG_TYPE(
+          env->isolate(),
+          "The \"function.length\" property must be an integer.");
+      return;
+    }

This prevents silent misbehavior if a user redefines .length to something non-integer.

3. Defensive buffer handling in deserialize()

If a getter shrinks or detaches an ArrayBuffer during options parsing, the previous code could copy uninitialized memory to SQLite. Now, the copy is checked for completeness:

-  input->CopyContents(buf, byte_length);
+  // The same user JavaScript may also have shrunk or detached the backing
+  // store, in which case byte_length is stale and CopyContents() leaves the
+  // remainder of buf uninitialized. Handing that to SQLite would disclose it
+  // through serialize().
+  if (input->CopyContents(buf, byte_length) != byte_length) {
+    sqlite3_free(buf);
+    THROW_ERR_INVALID_STATE(
+        env, "The \"buffer\" argument was resized while reading \"options\"");
+    return;
+  }

This ensures no uninitialized memory is exposed or used.

4. Test coverage for all edge cases

A new test file was added to cover all scenarios, including:

  • Closing the database in an options getter for every affected method.
  • Resizing/detaching buffers in a getter for deserialize().
  • Supplying non-integer .length properties to function/aggregate registration.

From the review thread

[araujogui] These are not a reentry call so I think these aren't in the correct test file.

[TrevorBurnham] Good call-out. I've created a follow-up PR: https://github.com/nodejs/node/pull/65769

Takeaways

  • Always re-validate critical state after running user-supplied code, especially if user code can execute via property getters or callbacks.
  • Never assume JavaScript properties (even built-ins like .length) are well-typed; always validate types before using them in native code.
  • When copying buffers or handling memory, check that the operation completed as expected, especially if user code could mutate the buffer mid-operation.
  • Comprehensive tests should cover not just typical usage, but also edge cases involving user code with side effects during API calls.