harden(sqlite): add opcode allowlist and buffer size validation to fileControl#27498
harden(sqlite): add opcode allowlist and buffer size validation to fileControl#27498robobun wants to merge 2 commits into
Conversation
|
Updated 3:13 AM PT - Feb 28th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 27498That installs a local version of the PR into your bun-27498 --bun |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdded opcode allowlisting and precise buffer-size validation to SQLite file-control handling in C++; introduced stack-allocated storage for 64-bit results, stricter null/number argument checks, and expanded tests validating allowed/unsupported opcodes, buffer-size enforcement, null handling, and int64 numeric arguments. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 1779-1784: The marshalling for the SQLITE_FCNTL_SIZE_HINT /
SQLITE_FCNTL_MMAP_SIZE / SQLITE_FCNTL_SIZE_LIMIT opcodes currently converts JS
numbers with toInt32() and writes 4 bytes into an 8-byte sqlite3_int64 buffer;
change the conversion code in JSSQLStatement.cpp for those opcodes to use a
64-bit conversion (e.g., toInt64() or toBigInt -> cast to sqlite3_int64) and
write the full 8 bytes into the destination (use a sqlite3_int64 temp and
memcpy/store the 8-byte value) so the buffer size returned by the switch
(sizeof(sqlite3_int64)) matches the actual bytes written for
SQLITE_FCNTL_SIZE_HINT, SQLITE_FCNTL_MMAP_SIZE, and SQLITE_FCNTL_SIZE_LIMIT.
- Around line 1838-1843: The code currently only rejects unsupported opcodes but
doesn't reject a null argument when an opcode requires writable storage; update
the file-control handling (after calling fcntlMinimumBufferSize(op) and getting
minBufSize) to check if minBufSize > 0 and the argument pointer (the buffer/arg
passed to sqlite3_file_control, e.g., the variable representing the
user-supplied pointer) is null, and if so call
throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject,
"Null pointer passed for opcode requiring writable storage"_s)) and return {};
ensure you reference fcntlMinimumBufferSize, minBufSize, and
throwException/createError so the null-pointer rejection is applied for opcodes
like SQLITE_FCNTL_PERSIST_WAL, SQLITE_FCNTL_DATA_VERSION, and
SQLITE_FCNTL_SIZE_LIMIT.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 5b8b725 and 3c1db8d4861205015b2553c55866b8c827c1c551.
📒 Files selected for processing (2)
src/bun.js/bindings/sqlite/JSSQLStatement.cpptest/js/bun/sqlite/sqlite.test.js
robobun
force-pushed
the
claude/harden-sqlite-filecontrol
branch
from
3c1db8d to
cd2b4b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/sqlite/JSSQLStatement.cpp (1)
1869-1887:⚠️ Potential issue | 🔴 CriticalReject non-TypedArray objects before calling
sqlite3_file_control.A plain object (e.g.
{}) currently enters the object branch, skips all checks, and leavesresultPtr == nullptr. For opcodes that require writable storage, this can pass a null pointer to SQLite.🔧 Proposed fix
if (resultValue.isObject()) { if (auto* view = jsDynamicCast<JSC::JSArrayBufferView*>(resultValue.getObject())) { if (view->isDetached()) { throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "TypedArray is detached"_s)); return {}; } resultPtr = view->vector(); if (resultPtr == nullptr) { throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected buffer"_s)); return {}; } // Validate that the provided buffer is large enough for this opcode. if (static_cast<int>(view->byteLength()) < minBufSize) { throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, makeString("Buffer too small for this opcode; need at least "_s, minBufSize, " bytes"_s))); return {}; } + } else { + throwException(lexicalGlobalObject, scope, createError(lexicalGlobalObject, "Expected result to be a number, null or a TypedArray"_s)); + return {}; } } else if (resultValue.isNumber()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp` around lines 1869 - 1887, The object branch in JSSQLStatement.cpp allows plain objects to bypass buffer checks and leave resultPtr null, which can lead to passing a null pointer into sqlite3_file_control; update the logic in the resultValue.isObject() handling (the jsDynamicCast<JSC::JSArrayBufferView*> check) to explicitly reject non-TypedArray objects by throwing an exception (e.g., createError with a clear message like "Expected TypedArray/ArrayBufferView") when the dynamic cast returns nullptr, and keep the existing detached/byteLength/minBufSize checks so only valid, non-detached, sufficiently large JSArrayBufferView instances are accepted before calling sqlite3_file_control.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 1883-1884: The check is narrowing view->byteLength() (size_t) to
int which can overflow; update the validation in the block that references
view->byteLength(), minBufSize, throwException, createError, lexicalGlobalObject
and scope to compare using an unsigned/size type (e.g. size_t) rather than
casting to int — either change minBufSize to size_t or cast minBufSize to size_t
and compare without casting view->byteLength(); keep the same error path
(throwException(createError(...))) if the size is insufficient.
- Around line 1891-1895: The conversion of
resultValue.toNumber(lexicalGlobalObject) before casting to sqlite3_int64 must
validate finiteness and range to avoid undefined behavior; in the branch where
minBufSize == sizeof(sqlite3_int64) (around the resultBuf write), call toNumber
then check RETURN_IF_EXCEPTION(scope, {}) and ensure std::isfinite(value) is
true and value is between std::numeric_limits<sqlite3_int64>::min() and ::max()
(use appropriate cast/comparison) before doing the static_cast and memcpy; if
the checks fail, return the same error path used elsewhere (mirroring the BigInt
validation) rather than performing the cast.
In `@test/js/bun/sqlite/sqlite.test.js`:
- Around line 1361-1480: The test suite is missing regression cases for invalid
object and non-finite numeric arguments to db.fileControl; add tests in the
"fileControl" describe block that call db.fileControl with an object (e.g., {}),
NaN, and Infinity for representative opcodes (both int-sized like
constants.SQLITE_FCNTL_PERSIST_WAL and int64-sized like
constants.SQLITE_FCNTL_SIZE_LIMIT) and assert they throw the same validation
errors (e.g., "Unsupported file control opcode" or "This opcode requires a
buffer or value argument" / "Buffer too small" depending on the opcode) to
ensure db.fileControl rejects non-finite and non-buffer/value inputs.
---
Outside diff comments:
In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 1869-1887: The object branch in JSSQLStatement.cpp allows plain
objects to bypass buffer checks and leave resultPtr null, which can lead to
passing a null pointer into sqlite3_file_control; update the logic in the
resultValue.isObject() handling (the jsDynamicCast<JSC::JSArrayBufferView*>
check) to explicitly reject non-TypedArray objects by throwing an exception
(e.g., createError with a clear message like "Expected
TypedArray/ArrayBufferView") when the dynamic cast returns nullptr, and keep the
existing detached/byteLength/minBufSize checks so only valid, non-detached,
sufficiently large JSArrayBufferView instances are accepted before calling
sqlite3_file_control.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 3c1db8d4861205015b2553c55866b8c827c1c551 and cd2b4b1d3e8dcbaad5afa2e40e5ca776fc3caa52.
📒 Files selected for processing (2)
src/bun.js/bindings/sqlite/JSSQLStatement.cpptest/js/bun/sqlite/sqlite.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…leControl Add input validation to `db.fileControl()` to improve robustness: - Restrict opcodes to a known-safe allowlist. Opcodes that return internal pointers or expect callback function pointers are no longer accepted. - Validate that TypedArray buffers are large enough for the opcode's output requirements before passing them to sqlite3_file_control. - Use a properly-sized stack buffer for the integer argument path to handle opcodes that write up to 8 bytes (sqlite3_int64). - Use 64-bit conversion for number arguments when the opcode expects sqlite3_int64 (SIZE_HINT, MMAP_SIZE, SIZE_LIMIT), with finiteness and range validation. - Reject null arguments and non-TypedArray objects for opcodes that require writable storage. Co-Authored-By: Claude <noreply@anthropic.com>
robobun
force-pushed
the
claude/harden-sqlite-filecontrol
branch
from
cd2b4b1 to
46f4173
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bun.js/bindings/sqlite/JSSQLStatement.cpp (1)
1-52:⚠️ Potential issue | 🟠 MajorAdd missing
<cmath>include forstd::isfinite.Line 1894 uses
std::isfinite(raw), which requires the<cmath>header. While this may be transitively included via other headers in some configurations, explicit inclusion is required for portability and follows C++ best practices. The absence of this header can cause build failures on platforms with stricter transitive include handling, such as musl libc (which aligns with the observedaarch64-muslandx64-muslbuild failures).Proposed fix
`#include` <limits> +#include <cmath> `#include` <wtf/text/ExternalStringImpl.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp` around lines 1 - 52, Add the missing <cmath> include so std::isfinite is defined: open JSSQLStatement.cpp and add `#include` <cmath> near the top of the includes (alongside other standard headers) to ensure the call to std::isfinite(raw) (used around the code referencing variable raw at line ~1894) builds on platforms with strict transitive include rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bun.js/bindings/sqlite/JSSQLStatement.cpp`:
- Around line 1-52: Add the missing <cmath> include so std::isfinite is defined:
open JSSQLStatement.cpp and add `#include` <cmath> near the top of the includes
(alongside other standard headers) to ensure the call to std::isfinite(raw)
(used around the code referencing variable raw at line ~1894) builds on
platforms with strict transitive include rules.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between cd2b4b1d3e8dcbaad5afa2e40e5ca776fc3caa52 and 46f4173.
📒 Files selected for processing (2)
src/bun.js/bindings/sqlite/JSSQLStatement.cpptest/js/bun/sqlite/sqlite.test.js
There was a problem hiding this comment.
All previously identified bugs have been addressed. This is security-hardening C++ code with opcode allowlist design decisions that would benefit from a human maintainer verifying the categorizations are correct.
Extended reasoning...
Overview
This PR adds an opcode allowlist (fcntlMinimumBufferSize) and buffer size validation to the SQLite fileControl binding in JSSQLStatement.cpp. It categorizes SQLITE_FCNTL_* opcodes into three groups (int-sized, int64-sized, no-arg) and rejects all others. It also adds proper int64 marshalling, null rejection for buffer-requiring opcodes, a final nullptr safety check, and comprehensive tests.
Security risks
The PR is itself a security hardening measure — it prevents passing arbitrary opcodes to sqlite3_file_control and prevents segfaults from null/undersized buffers. However, incorrect opcode categorization could either block legitimate use or allow unsafe operations. The correctness of the allowlist (which opcodes are safe, and what buffer sizes they need) is a critical design decision that should be verified by someone familiar with SQLite internals.
Level of scrutiny
High. This is production-critical native C++ code that directly interfaces with SQLite file control operations. Incorrect validation could cause crashes or data corruption. The previous review rounds (CodeRabbit and my own prior review) found real bugs that have since been fixed, demonstrating the complexity of getting this right.
Other factors
- All prior review feedback (6+ issues from CodeRabbit and 2 from my previous review) has been addressed in the current version
- The PR is AI-generated (claude label), adding importance to human verification
- Comprehensive test coverage is included for all validation paths
- The build had a failure on aarch64-musl that should be investigated
Extends the non-finite number rejection (NaN, Infinity) to also cover int-sized opcodes (e.g. PERSIST_WAL), not just int64-sized ones. Adds corresponding tests. Addresses review feedback on #27498. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed the remaining review feedback: Already fixed in initial commit:
Fixed in new commit (14ac698):
|
There was a problem hiding this comment.
All previously reported bugs have been addressed and the hardening logic looks correct. However, this touches security-sensitive C++ memory operations in SQLite bindings (memcpy, pointer validation, opcode classification), so a human with SQLite domain expertise should validate the opcode categorization before merging.
Extended reasoning...
Overview
This PR adds security hardening to db.fileControl() in JSSQLStatement.cpp by:
- Adding an opcode allowlist via
fcntlMinimumBufferSize()that classifies SQLITE_FCNTL_* opcodes into int-sized (4 bytes), int64-sized (8 bytes), no-arg (0 bytes), and disallowed (-1) - Validating TypedArray buffer sizes against opcode requirements
- Proper int vs int64 numeric marshalling with finiteness checks
- Null/nullptr rejection for opcodes requiring writable storage
- Comprehensive tests covering all validation paths
Security risks
The PR is itself a security hardening measure that prevents:
- Passing undersized buffers to
sqlite3_file_control(could cause out-of-bounds writes) - Exposing internal pointers through opcodes like FILE_POINTER/VFS_POINTER
- Null pointer dereferences when opcodes expect writable storage
- Integer truncation when passing negative values to int64 opcodes
All previously identified bugs (sign extension, null crashes, narrowing, finiteness) have been addressed in the current code.
Level of scrutiny
This is production-critical C++ code in SQLite bindings that handles memory operations (memcpy into stack buffers, pointer validation). The opcode classification in fcntlMinimumBufferSize() is the core security decision - misclassifying an opcode (e.g., putting a pointer-returning opcode in the int category) could create new vulnerabilities. A human with SQLite domain knowledge should verify the opcode list.
Other factors
- The PR has the
claudelabel indicating it was AI-generated, which warrants extra human scrutiny for native code - All previous reviewer feedback (from CodeRabbit and my prior review) has been incorporated
- Test coverage is thorough, testing allowed/rejected opcodes, buffer sizes, null handling, non-finite numbers, and plain objects
- The code is well-commented and the logic flow is clear
|
Closing this PR because it has been inactive for more than 90 days. |
1 participant
Summary
db.fileControl(), restricting it to well-defined SQLITE_FCNTL_* opcodes with known argument sizes. Opcodes that return internal pointers (FILE_POINTER, VFS_POINTER, JOURNAL_POINTER), expect callback function pointers (BUSYHANDLER), or are internal/extension-specific are no longer accepted.argparameter are large enough for the opcode's output requirements before forwarding tosqlite3_file_control.sizeof(sqlite3_int64)) for the integer argument path, ensuring correct behavior for opcodes that write up to 8 bytes.Test plan
test/js/bun/sqlite/sqlite.test.jscovering:🤖 Generated with Claude Code