Skip to content
Open
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
14 changes: 8 additions & 6 deletions src/jsc/bindings/JSFFIFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "root.h"
#include "JSFFIFunction.h"

#include <JavaScriptCore/FFIConversions.h>
#include <JavaScriptCore/JSCJSValueInlines.h>
#include <JavaScriptCore/VM.h>
#include "ZigGlobalObject.h"
Expand Down Expand Up @@ -73,14 +74,15 @@ extern "C" void Bun__FFIFunction_setDataPtr(JSC::EncodedJSValue jsValue, void* p
extern "C" JSC::EncodedJSValue Bun__CreateFFIFunctionValue(Zig::GlobalObject* globalObject, const ZigString* symbolName, unsigned argCount, Zig::FFIFunction functionPointer, bool addPtrField, void* symbolFromDynamicLibrary)
{
if (addPtrField) {
auto* function = Zig::JSFFIFunction::createForFFI(globalObject->vm(), globalObject, argCount, symbolName != nullptr ? Zig::toStringCopy(*symbolName) : String(), reinterpret_cast<Bun::CFFIFunction>(functionPointer));
auto& vm = JSC::getVM(globalObject);
// We should only expose the "ptr" field when it's a JSCallback for bun:ffi.
// Not for internal usages of this function type.
// We should also consider a separate JSFunction type for our usage to not have this branch in the first place...
function->putDirect(vm, JSC::Identifier::fromString(vm, String("ptr"_s)), JSC::jsNumber(std::bit_cast<double>(functionPointer)), JSC::PropertyAttribute::ReadOnly | 0);
auto scope = DECLARE_THROW_SCOPE(vm);
auto* function = Zig::JSFFIFunction::createForFFI(vm, globalObject, argCount, symbolName != nullptr ? Zig::toStringCopy(*symbolName) : String(), reinterpret_cast<Bun::CFFIFunction>(functionPointer));
// `functionPointer` is the TinyCC-compiled JSC-ABI wrapper, so `.ptr` must be the native function it calls.
JSC::JSValue ptr = JSC::FFI::pointerToJSValue(globalObject, reinterpret_cast<uint64_t>(symbolFromDynamicLibrary));
RETURN_IF_EXCEPTION(scope, {});
function->putDirect(vm, JSC::Identifier::fromString(vm, "ptr"_s), ptr, JSC::PropertyAttribute::ReadOnly | 0);
function->symbolFromDynamicLibrary = symbolFromDynamicLibrary;
return JSC::JSValue::encode(function);
RELEASE_AND_RETURN(scope, JSC::JSValue::encode(function));
}

return Bun__CreateFFIFunctionWithDataValue(globalObject, symbolName, argCount, functionPointer, nullptr);
Expand Down
39 changes: 22 additions & 17 deletions src/runtime/ffi/ffi_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ unsafe extern "C" {
arg_count: u32,
function_pointer: *const c_void,
add_ptr_property: bool,
input_function_ptr: *mut c_void,
symbol_from_dynamic_library: *mut c_void,
) -> JSValue;

fn Bun__CreateJSCFFIFunction(
Expand Down Expand Up @@ -164,28 +164,33 @@ mod exposed_to_ffi {
}
}

/// `host_fn::NewRuntimeFunction` thin wrapper. See host_fn.rs:310.
/// `cc()`'s variant of `host_fn::new_runtime_function`: with `add_ptr_property`
/// the C++ side encodes `symbol_from_dynamic_library` as the symbol's `.ptr`,
/// which allocates (and can throw) for addresses above 2^53.
Comment on lines +167 to +169

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.

If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code

#[track_caller]
#[inline]
fn new_runtime_function(
global: &JSGlobalObject,
symbol_name: &ZigString,
arg_count: u32,
function_pointer: *const c_void,
add_ptr_property: bool,
input_function_ptr: Option<*mut c_void>,
) -> JSValue {
// SAFETY: thin FFI wrapper; `global` is a live opaque JSC handle,
// `function_pointer` is a JIT'd entry point owned by the caller.
unsafe {
Bun__CreateFFIFunctionValue(
global,
symbol_name,
arg_count,
function_pointer,
add_ptr_property,
input_function_ptr.unwrap_or(core::ptr::null_mut()),
)
}
symbol_from_dynamic_library: Option<*mut c_void>,
) -> JsResult<JSValue> {
jsc::call_zero_is_throw(global, || {
// SAFETY: thin FFI wrapper; `global` is a live opaque JSC handle,
// `function_pointer` is a JIT'd entry point owned by the caller.
unsafe {
Bun__CreateFFIFunctionValue(
global,
symbol_name,
arg_count,
function_pointer,
add_ptr_property,
symbol_from_dynamic_library.unwrap_or(core::ptr::null_mut()),
)
}
})
}

/// `jsc::codegen::JSFFI::symbols_value_set_cached` thin wrapper.
Expand Down Expand Up @@ -1249,7 +1254,7 @@ impl FFI {
compiled.ptr.cast_const(),
true,
function.symbol_from_dynamic_library,
);
)?;
// `cb` is rooted by the `symbolsValue` cached own-property set below.
obj.put(global_this, str.slice(), cb);
}
Expand Down
70 changes: 70 additions & 0 deletions test/js/bun/ffi/cc.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1189,3 +1189,73 @@ describe.skipIf(isASAN)("compiler runtime header directory under BUN_TMPDIR", ()
expect(exitCode).toBe(0);
});
});

describe("symbols[name].ptr", () => {
// `.ptr` used to be the bits of the TinyCC wrapper's address reinterpreted as
// a double (a denormal like 6e-310), so CFunction/linkSymbols rejected it and
// a C function receiving it as a function pointer called NULL. The fixture
// runs in a child process because that NULL call takes the process down.
it("is the address of the compiled C function and is accepted wherever bun:ffi takes a pointer", async () => {
using dir = tempDir("bun-ffi-cc-symbol-ptr", {
"symbols.c": /* c */ `
int forty_two(void) { return 42; }
int add(int a, int b) { return a + b; }
const char* greeting(void) { return "hi"; }
typedef int (*binop)(int, int);
int apply(binop f, int a, int b) { return f(a, b); }
`,
"fixture.js": /* js */ `
import { cc, CFunction, linkSymbols } from "bun:ffi";
import path from "path";

const { symbols } = cc({
source: path.join(import.meta.dir, "symbols.c"),
symbols: {
forty_two: { args: [], returns: "i32" },
add: { args: ["i32", "i32"], returns: "i32" },
// returns: "cstring" symbols are wrapped in JS; the wrapper copies .ptr
greeting: { args: [], returns: "cstring" },
apply: { args: ["function", "i32", "i32"], returns: "i32" },
},
});

const shape = value => [typeof value, Number.isInteger(value) && value > 0];
const results = {
forty_two_ptr: shape(symbols.forty_two.ptr),
add_ptr: shape(symbols.add.ptr),
greeting_ptr: shape(symbols.greeting.ptr),
distinct_addresses: new Set([symbols.forty_two.ptr, symbols.add.ptr, symbols.greeting.ptr]).size,
cfunction: new CFunction({ ptr: symbols.forty_two.ptr, args: [], returns: "i32" })(),
cfunction_cstring: new CFunction({ ptr: symbols.greeting.ptr, args: [], returns: "cstring" })(),
link_symbols: linkSymbols({ add: { ptr: symbols.add.ptr, args: ["i32", "i32"], returns: "i32" } }).symbols.add(40, 2),
function_argument: symbols.apply(symbols.add.ptr, 20, 22),
};
console.log(JSON.stringify(results));
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "fixture.js"],
env: bunEnv,
cwd: String(dir),
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

const results = stdout.startsWith("{") ? JSON.parse(stdout) : stdout;
expect({ results, stderr, exitCode }).toMatchObject({
results: {
forty_two_ptr: ["number", true],
add_ptr: ["number", true],
greeting_ptr: ["number", true],
distinct_addresses: 3,
cfunction: 42,
cfunction_cstring: "hi",
link_symbols: 42,
function_argument: 42,
},
exitCode: 0,
});
});
});