ffi: regression test for JS number passed as 'ptr' argument (#29346)#29350
ffi: regression test for JS number passed as 'ptr' argument (#29346)#29350robobun wants to merge 8 commits into
Conversation
|
Updated 5:40 AM PT - Apr 17th, 2026
❌ @robobun, your commit e14f8af has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29350That installs a local version of the PR into your bun-29350 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a Linux-only regression test that compiles a C shared library at runtime, loads it via Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/ffi/addr32.c`:
- Around line 31-43: In addr32_out, add a null-pointer guard for the out
parameter at the start of the function (before dereferencing *out/mapping return
handling); if out is NULL, return -1 (and optionally set errno) instead of
proceeding—this prevents a segfault when *out is assigned later in the function
(the assignment to *out after the mmap loop and mapping == MAP_FAILED check).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97afb764-caa0-4d76-974d-f713362f0682
📒 Files selected for processing (2)
test/js/bun/ffi/addr32.ctest/js/bun/ffi/addr32.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29346.test.ts`:
- Around line 60-63: The test currently asserts an unconditional empty stderr
via expect(cErr).toBe("") which is fragile; modify the assertion to only check
stderr on failure by wrapping it in a conditional: after obtaining cErr and
cExit, do if (cExit !== 0) { expect(cErr).toBe(""); } and then keep
expect(cExit).toBe(0) as the final pass/fail check (references: variables cErr,
cExit, compiler).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30258495-c4d2-4400-be6c-3dcb4c8306e7
📒 Files selected for processing (1)
test/regression/issue/29346.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/29346.test.ts`:
- Around line 9-17: Reduce the long bug-history comment block to a concise
test-design rationale: remove the historical narrative around the issue and PR,
and keep only a short note that this test targets the Int32-path behavior
(mentioning ffiRead.ptr and the Int32 encoding/JSVALUE_TO_PTR/JSVALUE_IS_INT32
interaction) and that the test is Linux-gated; retain the minimal explanation
"open → read handle via ffiRead.ptr → pass back as ptr arg" so readers
understand the scenario being exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95708a47-cb54-4247-9540-54c1a58323a3
📒 Files selected for processing (1)
test/regression/issue/29346.test.ts
There was a problem hiding this comment.
LGTM — all previous concerns addressed; the unused #include <string.h> nit doesn't block merging.
Extended reasoning...
Overview
The PR adds a single new file: test/regression/issue/29346.test.ts. It is a self-contained Linux-only regression test for the FFI Int32 pointer marshaling bug from issue #29346. The test compiles a small C shared library at runtime inside a tempDir, loads it via bun:ffi, maps a pointer into the low 2 GiB, round-trips it through ffiRead.ptr, and verifies the value via read_handle. No production code is touched.
Security risks
None. This is a test-only addition that uses mmap in a sandboxed temp directory. There are no auth, crypto, or permission-sensitive code paths involved.
Level of scrutiny
Low. The change is a pure test addition in the regression directory, following the established convention. All previously flagged issues have been resolved: the test is correctly placed, Linux-gated via test.skipIf(!isLinux), self-contained (no beforeAll without a platform guard), and uses the conditional stderr pattern. The only remaining finding is an unused #include <string.h> — a cosmetic nit with no behavioral impact.
Other factors
The inline bug comment (unused include) is a genuine nit but does not affect correctness, compilation, or test results. Prior reviewer concerns from coderabbitai were also all addressed in commits 57bfccb and 185823a. Approving.
|
So what happens now??? 56 successful checks, 5 failing... ??? |
|
Can this be retried??? |
robobun
force-pushed
the
farm/816bb47a/ffi-number-ptr-crash
branch
from
45c1290 to
2cd5741
Compare
There was a problem hiding this comment.
LGTM — all prior concerns addressed; clean self-contained regression test in the right directory.
Extended reasoning...
Overview
This PR adds a single regression test file at test/regression/issue/29346.test.ts for the FFI JSVALUE_TO_PTR Int32 path. The test is entirely new test-only code — no production code is changed.
Security risks
None. The test compiles a trivial C shared library at runtime using mmap and dlopen. The C code has no security-sensitive surface (no user input, no network, no privilege escalation).
Level of scrutiny
Low. This is a self-contained regression test with no effect on production code paths. The test correctly exercises the specific JSVALUE_IS_INT32 branch via jscDescribe assertions, and the cross-platform fragility concerns I raised in my prior review (unguarded beforeAll, wrong directory, unused #include <string.h>) have all been resolved in subsequent commits.
Other factors
Previous review flagged three issues; all three are addressed in the current diff: the test lives in test/regression/issue/29346.test.ts, compilation runs inside the test.skipIf(!isLinux) body (no unguarded beforeAll), and <string.h> is absent. The stderr assertion is now correctly guarded on failure only.
There was a problem hiding this comment.
LGTM — all previous feedback addressed.
Extended reasoning...
Overview
The PR adds a regression test for issue 29346 (FFI Int32 pointer marshaling) in test/regression/issue/29346.test.ts and bumps the mimalloc hash in process.test.js.
Previous feedback resolved
All issues raised in prior review rounds have been addressed: the test is now in the correct test/regression/issue/ directory per convention, the C library compilation is inside the test.skipIf(!isLinux) body (no unguarded beforeAll), the unused string.h include was removed, and the compiler stderr assertion is now conditional on cExit !== 0.
Security risks
None. This is a test-only change with no production code modifications.
Level of scrutiny
Low. Both changed files are test infrastructure — one is a mechanical hash pin, the other is a well-scoped regression test that only runs on Linux and exercises a specific FFI marshaling code path.
Other factors
The test design is sound: it uses jscDescribe(handle) to assert Int32 encoding, ensuring the test actually exercises the intended branch and cannot silently pass on a broken build.
|
How can we get this testsed again... 60 successful checks and 3 failed??? |
robobun
force-pushed
the
farm/816bb47a/ffi-number-ptr-crash
branch
from
5792552 to
d18c9c3
Compare
There was a problem hiding this comment.
LGTM — all previously raised concerns have been addressed.
Extended reasoning...
Overview
This PR adds a single regression test file at test/regression/issue/29346.test.ts covering FFI pointer marshaling when a JS number (Int32-encoded JSValue) is passed as a ptr argument.
Security risks
No security risks. This is a test-only change with no production code modifications.
Level of scrutiny
Low. The change is a self-contained regression test: it embeds a small C shared library as a string, compiles it at runtime into a temp directory, and exercises the specific Int32 marshaling path. The test is Linux-only (correctly gated with test.skipIf(!isLinux)), uses tempDir for isolation, and applies the conditional stderr assertion pattern.
Other factors
All four issues from prior review rounds have been resolved: (1) test placed in the correct test/regression/issue/ directory, (2) no top-level beforeAll without platform guard — compilation is inside the test body, (3) stderr assertion is conditional on exit code, (4) unused #include <string.h> removed. The bug hunting system found no new issues. The test logic correctly validates the Int32 encoding path via jscDescribe before the round-trip read_handle call.
There was a problem hiding this comment.
LGTM — all previous concerns addressed.
Extended reasoning...
Overview
This PR adds a single new file: test/regression/issue/29346.test.ts, a Linux-only regression test for the FFI pointer-marshaling crash described in issue #29346. The test compiles a small C library at runtime, loads it via dlopen, exercises the Int32 branch of JSVALUE_TO_PTR, and verifies that the pointer round-trips correctly (returning 0xDEADBEEF).
Security risks
None. This is a test-only change with no production code modifications. The C code uses mmap with standard flags and operates entirely within a temporary directory.
Level of scrutiny
Low. The change is self-contained test infrastructure. The test logic is straightforward, correctly guarded with test.skipIf(!isLinux), and uses tempDir / await using for proper resource management.
Other factors
All previous review concerns have been addressed: the beforeAll platform-guard issue (resolved by embedding compilation inside the test.skipIf callback), the wrong-directory placement (now correctly in test/regression/issue/), the unused #include <string.h>, the fragile unconditional stderr assertion, and the verbose comment block. The only remaining finding is a nit about the dlopen handle not being explicitly closed — this is consistent with existing patterns in the test suite and has no practical impact in a single-shot test process.
| const { symbols } = dlopen(libPath, { | ||
| open_handle: { args: [FFIType.ptr], returns: FFIType.i32 }, | ||
| read_handle: { args: [FFIType.ptr], returns: FFIType.u32 }, | ||
| }); |
There was a problem hiding this comment.
🟡 The dlopen handle is silently discarded: const { symbols } = dlopen(...) drops the Library object, so .close() (which invokes dlclose()) can never be called. This is a minor resource-leak inconsistency — the underlying shared library stays mapped until process exit instead of being explicitly released.
Extended reasoning...
What the bug is
dlopen in bun:ffi returns an object with both symbols and a .close() method. Line 55 destructures only symbols, immediately dropping the Library handle:
const { symbols } = dlopen(libPath, { ... });Because no reference to the Library object is retained, .close() can never be called and the underlying dlclose() is never invoked. The shared library's memory mapping and file descriptor remain open until the process exits.
The specific code path
bun:ffi's dlopen() returns { symbols, close() { ... } }. The close() method calls the native dlclose() to release the library handle. Without storing the return value, there is simply no way to reach that method from anywhere in the test.
Why existing code does not prevent it
JavaScript destructuring silently discards any properties not listed in the pattern — no warning, no error. Bun's test runner does not enforce that dlopen handles must be closed, and the process-exit finalizer eventually releases everything at the OS level.
Addressing the refutations
Both refutations correctly note that (1) this is a single-shot Linux-only test running in its own process, (2) the OS unconditionally reclaims all FDs and mappings on exit, and (3) addr32.test.ts in the same directory uses the identical destructuring pattern — so this is already established local convention. These points are well-taken and explain why the severity is nit, not normal. The speculative concern about multiple test-runner iterations does not apply to Bun's architecture. The practical impact is zero.
What the impact would be
No functional test failure, no correctness issue. The using dir = tempDir(...) cleanup also succeeds on Linux regardless of open mappings because Linux allows unlinking files that are still mapped. This is purely a style inconsistency with the cc.test.ts convention of closing handles in afterAll.
How to fix
Store the full return value and close the handle at the end of the test:
const lib = dlopen(libPath, {
open_handle: { args: [FFIType.ptr], returns: FFIType.i32 },
read_handle: { args: [FFIType.ptr], returns: FFIType.u32 },
});
const { symbols } = lib;
// ... test body ...
lib.close();Alternatively, add an afterAll hook as cc.test.ts does.
Step-by-step proof
- Line 55:
const { symbols } = dlopen(libPath, { ... })— the returnedLibraryobject is not assigned to any variable. - The
Libraryobject is immediately eligible for garbage collection; no reference exists. .close()is a method on theLibraryobject, not onsymbols, sosymbols.closeisundefined.- When the test completes,
dlclose()is never called; the shared library mapping persists until process exit. - On Linux, when the temp-dir cleanup runs (
using dir = tempDir(...)),lib.sois unlinked successfully — the file disappears from the directory even though it is still mapped — so no test failure occurs. The OS releases the mapping on exit.
Covers the pattern from issue #29346: an out-parameter writes a handle into the low 2 GiB, the caller reads it back with `ffiRead.ptr`, and passes it as a `ptr` FFI argument. That routes through the Int32 JSValue branch of `JSVALUE_TO_PTR` (added in #25045). Before that branch, the double path sign-extended the Int32 into 0xFFFFFFFFxxxxxxxx and the callee segfaulted at 0xFFFFFFFFFFFFFFFF.
Addresses review: regression tests for numbered issues belong in test/regression/issue/, and the previous refactor of addr32.test.ts pulled its shared-library compilation into a top-level beforeAll that would fail on non-Linux platforms before test.skipIf had a chance to run. Revert addr32.test.ts / addr32.c to their pre-PR state and make the issue #29346 regression test fully self-contained via tempDir.
robobun
force-pushed
the
farm/816bb47a/ffi-number-ptr-crash
branch
from
da4c06c to
e14f8af
Compare
There was a problem hiding this comment.
LGTM — all prior review concerns have been addressed.
Extended reasoning...
Overview
Single new file: test/regression/issue/29346.test.ts. Adds a Linux-only regression test for FFI pointer marshaling (#29346): compiles an inline C shared library at runtime, loads it via bun:ffi/dlopen, and verifies the Int32-encoded pointer round-trip that previously segfaulted.
Security risks
None. This is a test-only file that compiles and exercises existing bun:ffi functionality. No production code paths are modified.
Level of scrutiny
Low. A self-contained regression test with no impact on shipped code.
Other factors
All issues from my prior reviews have been resolved: correct file placement, Linux guard on compilation, no spurious beforeAll, conditional stderr assertion, trimmed comments, and unused #include <string.h> removed. The remaining open nit (not calling lib.close() after dlopen) has zero practical impact for a single-shot test process and matches the existing convention in adjacent test files. CI failures are pre-existing infrastructure flakes unrelated to this PR (worker_threads core dump, shell leak test).
|
CI status: 60+ pass, 1 fail — pre-existing flake, not fixable from this PR. The only failure is My test ( |
|
Disengaging — the sole CI failure ( |
2 participants
Issue
#29346 reports that passing a JS number (e.g. a handle returned from
ffiRead.ptr()) as a'ptr'FFI argument segfaults on Linux x64 at0xFFFFFFFFFFFFFFFF.Cause
When a handle lands in the first 2 GiB of address space,
JSValue.jsNumber(u64)boxes it as an Int32-encoded JSValue. The pre-fixJSVALUE_TO_PTRinsrc/bun.js/api/FFI.htook the double-encoded path unconditionally for every non-null non-typed-array value, which sign-extended the Int32 bit pattern into0xFFFFFFFFxxxxxxxx— hence the crash.Fix
#25045 (commit
7335cb747b) landed an explicit Int32 branch inJSVALUE_TO_PTRwell before v1.3.12:The reporter's reproduction no longer crashes on current main or on a freshly installed v1.3.12. This PR adds a regression test so the fix can't regress silently.
Test
test/js/bun/ffi/addr32.test.tsgains a new case.addr32.cgains two helper symbols:addr32_out(void **out)— writes a mmap'd low-2-GiB pointer (magic0xDEADBEEF) through an out-param. Same pattern asduckdb_open.addr32_read(void *handle)— reads the magic back, which requires the handle to have round-tripped correctly.The test calls
addr32_outwithptr(buf), reads the handle withffiRead.ptr, asserts viajscDescribethat it's Int32-encoded (so the test exercises the right branch and can't silently pass on the double path), then passes it back intoaddr32_read. Pre-fix that segfaulted; post-fix it reads0xDEADBEEF.