Skip to content

bun:ffi: make lib.symbols.<fn>.ptr a numeric native address - #34008

Closed
robobun wants to merge 2 commits into
mainfrom
farm/bb627218/ffi-symbol-ptr-numeric
Closed

bun:ffi: make lib.symbols.<fn>.ptr a numeric native address#34008
robobun wants to merge 2 commits into
mainfrom
farm/bb627218/ffi-symbol-ptr-numeric

Conversation

@robobun

@robobun robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Reproduction

import { CFunction, JSCallback, linkSymbols } from "bun:ffi";
const cb = new JSCallback((a, b) => a + b, { args: ["int", "int"], returns: "int" });
const lib = linkSymbols({ add2: { ptr: cb.ptr, args: ["int", "int"], returns: "int" } });
console.log(lib.symbols.add2.ptr);
// 2.8613662853287e-311   (a denormal double, not an address)
new CFunction({ ptr: lib.symbols.add2.ptr, args: ["int", "int"], returns: "int" });
// TypeError: Symbol "CFunction0" is missing a "ptr" field

Same with dlopen: lib.symbols.fn.ptr is always a denormal in the e-310 / e-311 range, while every other bun:ffi pointer producer (ptr(), JSCallback.ptr, read.ptr) returns a plain integer address. The documented symbols.fn.ptr -> CFunction / linkSymbols round-trip can never have worked on this representation: the denormal truncates to 0 in the consumer's as_ptr_address() and the constructor throws.

Cause

Bun__CreateFFIFunctionValue in src/jsc/bindings/JSFFIFunction.cpp stored JSC::jsNumber(std::bit_cast<double>(functionPointer)): the pointer's raw bits reinterpreted as IEEE-754 bits, which for any userspace address is a denormal. It was also the wrong pointer: functionPointer is the TinyCC-compiled trampoline with JSC calling convention ((JSGlobalObject*, CallFrame*) -> EncodedJSValue), not something a CFunction wrapper could ever call with the declared native signature.

Fix

Expose symbolFromDynamicLibrary (the dlsym'd / user-provided / cc-compiled native function, which every addPtrField caller already passes), encoded as jsNumber((double)(uintptr_t)addr), matching PTR_TO_JSVALUE in FFI.h and JSValue::from_ptr_address everywhere else.

Verification

New test in test/js/bun/ffi/ffi.test.js builds a linkSymbols wrapper around a JSCallback, asserts symbols.add2.ptr === cb.ptr (integer), then feeds that .ptr back through CFunction and calls it. On main the test fails with the denormal value and the "missing a ptr field" throw; with the fix it passes, and dlopen("libc.so.6").symbols.abs.ptr round-trips through CFunction to abs(-7) == 7. The rest of test/js/bun/ffi/ passes unchanged.


no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/ffi/ffi.test.js

The .ptr field on dlopen/linkSymbols/cc symbol functions was produced via
std::bit_cast<double>(functionPointer), which reinterprets the pointer bits
as IEEE-754 bits and yields a denormal (~6.8e-310) instead of an integer
address. Passing that to CFunction/linkSymbols truncates to 0 and throws
'missing a "ptr" field'.

It was also the wrong pointer: functionPointer is the TCC-compiled
JSC-calling-convention trampoline, not the native symbol. Expose
symbolFromDynamicLibrary (the dlsym'd / user-provided / cc-compiled native
function) encoded the same way as every other bun:ffi pointer
(PTR_TO_JSVALUE / JSValue::from_ptr_address), so .ptr -> CFunction
round-trips.
@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Changes

FFI pointer round-trip

Layer / File(s) Summary
Symbol pointer encoding and validation
src/jsc/bindings/JSFFIFunction.cpp, test/js/bun/ffi/ffi.test.js
ptr now encodes the dynamic library symbol address, and a subprocess FFI test verifies pointer sanity, CFunction rewrapping, and the "call 5" result.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: returning a numeric native address for lib.symbols..ptr.
Description check ✅ Passed The description covers the change and verification, though it uses different headings than the repository template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 1:04 AM PT - Jul 12th, 2026

@autofix-ci[bot], your commit 741876f has 2 failures in Build #72077 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 34008

That installs a local version of the PR into your bun-34008 executable, so you can run:

bun-34008 --bun

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/js/bun/ffi/ffi.test.js`:
- Around line 730-744: Make the child-process FFI setup and invocation
exception-safe by wrapping the code using linkSymbols, CFunction, and the
callback in try/finally blocks. Ensure rewrapped, lib, and cb are each closed
when successfully created, including when setup or invocation throws, while
preserving the existing logging and call behavior.
🪄 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: 455704f1-808c-4990-85e3-e493abcfeabd

📥 Commits

Reviewing files that changed from the base of the PR and between 8624c2b and 741876f.

📒 Files selected for processing (2)
  • src/jsc/bindings/JSFFIFunction.cpp
  • test/js/bun/ffi/ffi.test.js

Comment thread test/js/bun/ffi/ffi.test.js

@claude claude Bot left a comment

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.

LGTM — one-line encoding fix that makes .ptr a usable numeric address instead of a denormal double.

What was reviewed:

  • Verified the new encoding (jsNumber((double)(uintptr_t)ptr)) matches PTR_TO_JSVALUE in FFI.h and JSValue::from_ptr_address in JSValue.rs.
  • Confirmed all three add_ptr_property=true callers in ffi_body.rs already pass symbol_from_dynamic_library, so switching from the JSC trampoline to the native symbol is safe and requires no signature changes.
  • Checked src/js/bun/ffi.tswrap.ptr = functionToCall.ptr and .native = functionToCall both flow from this field, consistent with the test's symPtr === nativePtr === cbPtr assertion.
Extended reasoning...

Overview

Single functional line changed in src/jsc/bindings/JSFFIFunction.cpp: the .ptr property on bun:ffi symbol functions was previously jsNumber(std::bit_cast<double>(functionPointer)), which reinterprets the pointer's raw bits as an IEEE-754 double — for any userspace address this yields a denormal in the e-310/e-311 range that truncates to 0 in as_ptr_address(). It also exposed the wrong pointer (the JSC-calling-convention trampoline, not the native symbol). The fix exposes symbolFromDynamicLibrary encoded as jsNumber((double)(uintptr_t)addr), matching the encoding used everywhere else in bun:ffi (PTR_TO_JSVALUE, from_ptr_address, JSCallback.ptr, read.ptr). A new subprocess test in ffi.test.js verifies the integer encoding, equality with the input JSCallback.ptr, and a functional round-trip through CFunction.

Security risks

None. This exposes a native address the caller already has (they passed it via linkSymbols({ptr}) or it's the dlsym result of a library they explicitly loaded). No new capability is granted — the same address was already reachable via JSCallback.ptr and via calling the wrapped function. The previous bit_cast value leaked the trampoline address anyway, just in an unusable encoding.

Level of scrutiny

Low-to-medium. bun:ffi is native/unsafe territory, but this is a pure value-encoding fix on a read-only property, not a change to any call/marshalling path. The old value was provably unusable (denormal → 0 → "missing ptr field" throw), so nothing could have depended on it. I traced all call sites: the three FFI-path callers in ffi_body.rs:1223/1585/1681 all pass add_ptr_property=true with a populated symbol_from_dynamic_library; the non-FFI callers (hw_exports.rs, node_os.rs, generated bindings) pass add_ptr_property=false and don't hit this branch. The JS wrapper in src/js/bun/ffi.ts:410 copies this field onto the outer wrapper unchanged.

Other factors

The test follows the file's established subprocess pattern (matching the neighboring JSCallback exceptions propagate test), drains both pipes concurrently, and asserts a combined {parsed, call, stderr, exitCode} object per repo convention. CodeRabbit's try/finally suggestion was withdrawn after the author explained the subprocess boundary makes it unnecessary. The jsNumber(double) vs jsDoubleNumber distinction (int32 fast path) doesn't matter here since as_ptr_address() = as_number() as usize handles both encodings, and 64-bit userspace addresses exceed int32 range anyway.

@robobun

robobun commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator Author

CI status: the only hard failure in #72077 is test/js/node/test/parallel/test-worker-message-port-transfer-terminate.js (SIGABRT in JSC::JSObject::getOwnPropertyDescriptor on the x64-asan lane), which is unrelated to this one-line FFI change and is also red on unrelated PR builds #72074 and #72068. The other annotations (napi finalizer leak, napi_wrap lifetime, spawn timeout) are flaky-tagged and passed on retry. test/js/bun/ffi/ is green on every lane.

Ready for review.

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

Closing this since #35246 (bun:ffi: use the engine-native FFI when available) merged and covers the same ground. Thank you @robobun for the PR — if there's a piece of this that #35246 didn't pick up, please say so and we'll take another look.

(This comment was written by Claude, on behalf of the Bun team.)

@robobun

robobun commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

Confirmed #35246 fixes .ptr for dlopen and linkSymbols (now via the engine-native JSC::JSFFIFunction path), so the repro in this PR's description is fixed on main.

One piece it didn't pick up: cc() still goes through the old Bun__CreateFFIFunctionValuestd::bit_cast<double>(functionPointer) path (JSFFIFunction.cpp:81, called from ffi_body.rs:1247), so cc().symbols.fn.ptr is still a denormal and the round-trip still throws. On canary 65c47c806:

import { cc, CFunction } from "bun:ffi";
// /tmp/a.c: int add2(int a, int b){ return a + b; }
const lib = cc({ source: "/tmp/a.c", symbols: { add2: { args: ["int","int"], returns: "int" } } });
console.log(lib.symbols.add2.ptr);
// 1.5046817921187e-311
new CFunction({ ptr: lib.symbols.add2.ptr, args: ["int","int"], returns: "int" });
// TypeError: Symbol "CFunction" is missing a "ptr" field

The one-line fix in this PR still applies there (it's the only addPtrField=true caller left). Happy to rebase this onto main scoped to the cc() case, or leave it if cc() is headed for the engine-native path anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants