Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions src/bun.js/api/ffi.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1222,10 +1222,9 @@

if (function.symbol_from_dynamic_library == null) {
const ret = global.toInvalidArguments("Symbol \"{s}\" is missing a \"ptr\" field. When using linkSymbols() or CFunction(), you must provide a \"ptr\" field with the memory address of the native function.", .{bun.asByteSlice(function_name)});
for (symbols.values()) |*value| {
allocator.free(@constCast(bun.asByteSlice(value.base_name.?)));
value.arg_types.clearAndFree(allocator);
value.deinit(global);
}

Check notice on line 1227 in src/bun.js/api/ffi.zig

View check run for this annotation

Claude / Claude Code Review

dlopen error paths still have the leak pattern this PR fixes for linkSymbols

Pre-existing, but worth noting since the PR description cites `dlopen`'s cleanup as the model: `FFI.open()` still has the same leak class in two error paths. The `dylib.lookup() orelse` branch (~ffi.zig:1110-1118) uses the manual `free(base_name); arg_types.clearAndFree()` pattern instead of `value.deinit(global)`, leaking the `TCC.State` of any previously-compiled symbol; and the `.failed` branch (~ffi.zig:1138-1146) registers the `deinit` loop with `defer` but then calls `symbols.clearAndFree(
Comment on lines 1225 to 1227

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing, but worth noting since the PR description cites dlopen's cleanup as the model: FFI.open() still has the same leak class in two error paths. The dylib.lookup() orelse branch (~ffi.zig:1110-1118) uses the manual free(base_name); arg_types.clearAndFree() pattern instead of value.deinit(global), leaking the TCC.State of any previously-compiled symbol; and the .failed branch (~ffi.zig:1138-1146) registers the deinit loop with defer but then calls symbols.clearAndFree() inline, so the deferred loop runs over an already-empty map and frees nothing. Both are one-line fixes identical in spirit to the linkSymbols changes here and would be a natural addition to this PR.

Extended reasoning...

What the bug is

This PR fixes the error-cleanup paths in FFI.linkSymbols() by replacing the manual free(base_name); arg_types.clearAndFree() pattern with value.deinit(global), and the PR description explicitly cites "the equivalent cleanup in dlopen" as the model. However, FFI.open() (the dlopen implementation) still contains two error paths with the same leak class that this PR fixes:

  1. The dylib.lookup() orelse branch (around src/bun.js/api/ffi.zig:1110-1118) still uses the manual-free pattern:

    const resolved_symbol = dylib.lookup(*anyopaque, function_name) orelse {
        const ret = global.toInvalidArguments("Symbol \"{s}\" not found in \"{s}\"", ...);
        for (symbols.values()) |*value| {
            bun.default_allocator.free(@constCast(bun.asByteSlice(value.base_name.?)));
            value.arg_types.clearAndFree(bun.default_allocator);
        }
        symbols.clearAndFree(bun.default_allocator);
        dylib.close();
        return ret;
    };

    This frees base_name and arg_types but never calls Function.deinit(), so it never frees state (the TCC.State) or step.failed.msg.

  2. The .failed branch (around src/bun.js/api/ffi.zig:1138-1146) does call deinit, but via defer:

    .failed => |err| {
        defer for (symbols.values()) |*other_function| {
            other_function.deinit(global);
        };
        const res = ZigString.init(err.msg).toErrorInstance(global);
        symbols.clearAndFree(bun.default_allocator);
        dylib.close();
        return res;
    },

    In Zig, the defer body runs at scope exit — after symbols.clearAndFree() has already executed inline. By the time the deferred loop runs, symbols.values() returns an empty slice, so the loop iterates zero times and nothing is freed.

Code path that triggers it

The open() loop processes each symbol completely (lookup → compile() → switch on step) before moving to the next iteration. Function.compile() allocates a TCC.State and stores it in function.state on success.

Step-by-step proof for case (1)dlopen(lib, { good: {...}, missing: {...} }) where good exists in the library and missing does not:

  1. Iteration 1: dylib.lookup("good") succeeds → good.compile() runs → allocates a TCC.State into good.stategood.step = .compiled.
  2. Iteration 2: dylib.lookup("missing") returns null → enters the orelse branch.
  3. The cleanup loop runs over both symbols and frees only base_name + arg_types for each.
  4. good.state (a live TCC.State) is never freed → leaked.

Step-by-step proof for case (2)dlopen(lib, { good: {...}, bad: {...} }) where both resolve in the library but TinyCC compilation of bad fails (e.g. handleTCCError sets bad.step = .failed):

  1. Iteration 1: good is looked up and compiled → good.state holds a TCC.State.
  2. Iteration 2: bad is looked up, compile() runs, TinyCC reports an error → bad.step = .failed with an allocated err.msg.
  3. The .failed arm registers defer for (symbols.values()) |*f| f.deinit(global);.
  4. res is built from err.msg.
  5. symbols.clearAndFree() runs inline — the map's backing arrays are freed and values() is now empty.
  6. return res; triggers the defer. symbols.values() returns a zero-length slice, so the deinit loop body never executes.
  7. Net result: good.base_name, good.arg_types, good.state (TCC.State), bad.base_name, bad.arg_types, and bad.step.failed.msg all leak.

Why existing code doesn't prevent it

For (1), the adjacent compile() catch, .failed, and .pending branches in open() all already call value.deinit(global) — the dylib.lookup() orelse branch is simply the one that was missed when the others were converted. For (2), the code appears correct at a glance because it does call deinit on every value, but Zig's defer semantics mean the loop's iteration source (symbols.values()) is evaluated at scope-exit time, after clearAndFree() has emptied it. This is exactly the ordering bug this PR avoids in the linkSymbols .failed branch by running the deinit loop inline (after building res, before clearAndFree).

Impact

Each leaked TCC.State is a full TinyCC compiler state (relocated code, symbol tables, etc.) — typically tens of KB or more per symbol. In long-running processes that probe for symbols or repeatedly attempt dlopen against libraries with partial symbol coverage, this accumulates. The .failed path additionally leaks every base_name, every arg_types buffer, and the duplicated TinyCC error message string. These are not crashes or correctness bugs in user-visible behavior — they are pure memory leaks on the error path — but they are the same class of bug this PR is explicitly fixing in linkSymbols.

How to fix

Both are one-line changes mirroring this PR's linkSymbols edits:

  • dylib.lookup() orelse branch: replace the loop body with value.deinit(global); (identical to the change at lines 1225-1227 of this PR).
  • .failed branch: drop the defer and run the deinit loop inline after building res but before symbols.clearAndFree():
    .failed => |err| {
        const res = ZigString.init(err.msg).toErrorInstance(global);
        for (symbols.values()) |*other_function| {
            other_function.deinit(global);
        }
        symbols.clearAndFree(bun.default_allocator);
        dylib.close();
        return res;
    },
    This is exactly the structure this PR uses for the linkSymbols .failed branch at lines 1244-1252.

These are pre-existing — the PR doesn't touch FFI.open() — but since the PR description holds dlopen up as the reference implementation, it's worth flagging that the reference has the same bug.

symbols.clearAndFree(allocator);
return ret;
}
Expand All @@ -1243,20 +1242,18 @@
};
switch (function.step) {
.failed => |err| {
// Build the error before deinit: `err.msg` is owned by `function.step`
// and `function` is one of `symbols.values()`.
const res = ZigString.init(err.msg).toErrorInstance(global);
for (symbols.values()) |*value| {
allocator.free(@constCast(bun.asByteSlice(value.base_name.?)));
value.arg_types.clearAndFree(allocator);
value.deinit(global);
}

Check notice on line 1250 in src/bun.js/api/ffi.zig

View check run for this annotation

Claude / Claude Code Review

err.msg UAF: toErrorInstance does not copy, .message dangles after deinit

Pre-existing, but worth noting since this PR's new comment implies the ordering makes it safe: `ZigString.init(err.msg).toErrorInstance()` does **not** copy the bytes — `Zig::getErrorInstance` uses `toString()` (unlike its siblings `getTypeErrorInstance`/`getSyntaxErrorInstance`/`getRangeErrorInstance` which use `toStringCopy()`), so for an untagged Latin1 `ZigString` it hits `WTF::StringImpl::createWithoutCopying` and the returned Error's `.message` directly references `err.msg`'s heap buffer.
Comment on lines +1245 to 1250

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing, but worth noting since this PR's new comment implies the ordering makes it safe: ZigString.init(err.msg).toErrorInstance() does not copy the bytes — Zig::getErrorInstance uses toString() (unlike its siblings getTypeErrorInstance/getSyntaxErrorInstance/getRangeErrorInstance which use toStringCopy()), so for an untagged Latin1 ZigString it hits WTF::StringImpl::createWithoutCopying and the returned Error's .message directly references err.msg's heap buffer. After the value.deinit(global) loop frees step.failed.msg, reading e.message from JS is a use-after-free. The same pattern exists in dlopen's .failed branch and FFI.callback's .failed branch; the simplest local fix is ZigString.fromUTF8(err.msg).toErrorInstance(global) (or change getErrorInstance in helpers.h to use toStringCopy).

Extended reasoning...

What the bug is

Building the JS Error before the deinit loop is necessary but not sufficient, because ZigString.init(...).toErrorInstance() does not copy the message bytes for plain Latin1 strings. The returned Error object's .message property is a JSString whose StringImpl points directly into err.msg's heap allocation. The subsequent value.deinit(global) loop frees that allocation (via Function.deinitval.allocator.free(val.step.failed.msg) when .allocated == true), so any later read of e.message from JavaScript dereferences freed memory.

Code path

  1. err.msg is heap-allocated. TinyCC reports the compilation error through handleTCCError (ffi.zig:1531), which sets this.step = .{ .failed = .{ .msg = this.allocator.dupe(u8, msg), .allocated = true } }.

  2. ZigString.init(err.msg) produces an untagged pointer. ZigString.init (ZigString.zig:481-483) just stores .{ ._unsafe_ptr_do_not_use = slice.ptr, .len = slice.len } — no UTF-8 tag, no UTF-16 tag, no external/global tag.

  3. toErrorInstanceZig::getErrorInstancetoString() takes the no-copy path. In src/bun.js/bindings/helpers.h, getErrorInstance (line ~383) calls toString(*str). For an untagged, non-UTF8, non-external Latin1 ZigString, toString() falls through to its final branch (helpers.h:115-118):

    return WTF::String(WTF::StringImpl::createWithoutCopying({ untag(str.ptr), str.len }));

    Note the contrast: getTypeErrorInstance, getSyntaxErrorInstance, and getRangeErrorInstance (helpers.h:395-413) all call toStringCopy(*str). getErrorInstance is the lone outlier that uses non-copying toString().

  4. JSC::createError stores the same StringImpl. createError does putDirect(vm.propertyNames->message, jsString(vm, message)); jsString just refs the existing StringImpl for length > 1, no byte copy.

  5. The deinit loop frees the buffer. Back in linkSymbols, the loop reaches the failing function and Function.deinit executes if (val.step == .failed and val.step.failed.allocated) val.allocator.free(val.step.failed.msg). The JSString backing the returned Error's .message now points into a freed mimalloc allocation.

  6. JS reads dangling memory. When user code does String(e.message), console.log(e), or the error propagates to top-level, the freed buffer is dereferenced.

Why ordering alone doesn't help

The PR's comment ("Build the error before deinit: err.msg is owned by function.step") correctly addresses construction-time safety — you can't pass a freed pointer to ZigString.init. But it does not address post-return lifetime: the constructed JSString still aliases the buffer with no ownership, so freeing after construction is just as bad as freeing before. The comment encodes a stronger invariant than actually holds.

Step-by-step proof

Take the PR's own new test case:

linkSymbols({ "not a valid C identifier!": { ptr: cb.ptr, args: [], returns: "void" } });
  • TinyCC fails to compile the generated C (the symbol name is emitted verbatim), and handleTCCError heap-allocates the diagnostic into step.failed.msg with .allocated = true.
  • linkSymbols enters the .failed branch, builds res = ZigString.init(err.msg).toErrorInstance(global)res.message's StringImpl now points at the duped diagnostic buffer.
  • The deinit loop frees that buffer.
  • The test only checks threw = true and never touches e.message, so ASAN doesn't flag it. Change the catch to catch (e) { threw = true; String(e.message); } and run under ASAN to observe the heap-use-after-free.

Status and scope

This is pre-existing: the old code did exactly the same thing (const res = ZigString.init(err.msg).toErrorInstance(global); function.deinit(global);). The PR neither introduces nor worsens it. The identical pattern also exists in:

  • dlopen's .failed branch (const res = ZigString.init(err.msg).toErrorInstance(global) followed by defer for ... other_function.deinit(global))
  • FFI.callback's .failed branch (const message = ZigString.init(err.msg).toErrorInstance(globalThis); func.deinit(globalThis);)
  • Bun__FFI__cc's .failed branch

Flagging it here because the PR rewrites these exact lines for memory-safety reasons and the new comment reads as though the lifetime issue is handled.

Fix

Either of:

  • Call site: const res = ZigString.fromUTF8(err.msg).toErrorInstance(global);fromUTF8 sets the UTF-8 tag, so toString() takes the fromUTF8ReplacingInvalidSequences path which allocates a copy.
  • Root cause: change getErrorInstance in src/bun.js/bindings/helpers.h to use toStringCopy(*str) like getTypeErrorInstance/getSyntaxErrorInstance/getRangeErrorInstance already do. This fixes all four call sites at once.


const res = ZigString.init(err.msg).toErrorInstance(global);
function.deinit(global);
symbols.clearAndFree(allocator);
return res;
},
.pending => {
for (symbols.values()) |*value| {
allocator.free(@constCast(bun.asByteSlice(value.base_name.?)));
value.arg_types.clearAndFree(allocator);
value.deinit(global);
}
symbols.clearAndFree(allocator);
return ZigString.static("Failed to compile (nothing happend!)").toErrorInstance(global);
Expand Down
40 changes: 39 additions & 1 deletion test/js/bun/ffi/ffi-error-messages.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { dlopen, linkSymbols } from "bun:ffi";
import { describe, expect, test } from "bun:test";
import { isArm64, isMusl, isWindows } from "harness";
import { bunEnv, bunExe, isArm64, isMusl, isWindows } from "harness";

// TinyCC (and all of bun:ffi) is disabled on Windows ARM64
const isFFIUnavailable = isWindows && isArm64;
Expand Down Expand Up @@ -86,4 +86,42 @@ describe.skipIf(isFFIUnavailable)("FFI error messages", () => {
});
}).toThrow('you must provide a "ptr" field with the memory address of the native function.');
});

// The symbol name is embedded verbatim into the C source handed to TinyCC. An invalid
// C identifier makes compilation fail and takes the `.failed` cleanup path in
// `linkSymbols`. That path used to free `base_name` for every symbol and then call
// `function.deinit()` on the failing one, freeing its `base_name` a second time.
// Run in a subprocess so the heap-corruption abort (debug/ASAN builds) is observable
// as a non-zero exit instead of tearing down the test runner.
test("linkSymbols cleans up without double-free when TinyCC compilation fails", async () => {
const src = /* js */ `
const { linkSymbols, JSCallback } = require("bun:ffi");
const cb = new JSCallback(() => {}, { returns: "void", args: [] });
let threw = false;
try {
linkSymbols({
"not a valid C identifier!": {
ptr: cb.ptr,
args: [],
returns: "void",
},
});
} catch (e) {
threw = true;
}
cb.close();
if (!threw) throw new Error("expected linkSymbols to throw");
console.log("ok");
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", src],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout).toBe("ok\n");
expect(exitCode).toBe(0);
});
});
Loading