ffi: don't free caller-owned memory in toBuffer without a finalizer - #36521
Merged
Jarred-Sumner merged 5 commits intoJul 31, 2026
Merged
Claude / Claude Code Review
completed
Jul 31, 2026 in 16m 17s
Code review found 2 potential issues
Found 4 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/bun/ffi/ffi.test.js:1458-1459 |
cc()-based finalizer test not gated on isASAN |
| 🟡 Nit | test/js/bun/ffi/ffi.test.js:1411-1412 |
Subprocess tests drop stderr and use toContain instead of combined-object assertion |
Annotations
Check warning on line 1459 in test/js/bun/ffi/ffi.test.js
claude / Claude Code Review
cc()-based finalizer test not gated on isASAN
This test invokes `cc()` (TinyCC) in-process without an `isASAN` guard. Every other in-process `cc()` invocation in the repo — including the trivial `add(a,b)` case and the "GC liveness of compiled symbols" case, which is the closest analogue to this test — is wrapped in `skipIf(isASAN)` with the comment "TinyCC's setjmp/longjmp error handling conflicts with ASan" (see `test/js/bun/ffi/cc.test.ts:22,231,390,459,572,621`). Consider adding `it.skipIf(isASAN)(…)` here, or reusing the compiled fixtu
Check warning on line 1412 in test/js/bun/ffi/ffi.test.js
claude / Claude Code Review
Subprocess tests drop stderr and use toContain instead of combined-object assertion
The three `runsClean` subprocess tests destructure only `{ stdout, exitCode }` (dropping the `stderr` that `runsClean` reads and returns) and assert with `toContain("survived-gc")`. REVIEW.md's subprocess-test rule and every sibling subprocess test in this file use the combined-object form — `expect({ stdout, stderr, exitCode }).toEqual({ stdout: "survived-gc\n", stderr: "", exitCode: 0 })` — which is available here since `bunEnv` silences debug logs, and gives a much better failure message. App
Loading