diff --git a/src/jsc/bindings/ffi.cpp b/src/jsc/bindings/ffi.cpp index 2f4b6f561cee..f62fb0657c19 100644 --- a/src/jsc/bindings/ffi.cpp +++ b/src/jsc/bindings/ffi.cpp @@ -5,13 +5,20 @@ typedef struct FFIFields { uint32_t JSArrayBufferView__offsetOfByteOffset; uint32_t JSArrayBufferView__offsetOfVector; uint32_t JSCell__offsetOfType; + // Register (8-byte slot) indices into a CallFrame, not byte offsets. + uint32_t CallFrame__argumentCountIncludingThisSlot; + uint32_t CallFrame__firstArgumentSlot; } FFIFields; extern "C" FFIFields Bun__FFI__offsets = { 0 }; +static_assert(sizeof(JSC::Register) == sizeof(void*), "the cc() wrapper indexes the CallFrame as an array of pointer-sized slots"); + extern "C" void Bun__FFI__ensureOffsetsAreLoaded() { Bun__FFI__offsets.JSArrayBufferView__offsetOfLength = JSC::JSArrayBufferView::offsetOfLength(); Bun__FFI__offsets.JSArrayBufferView__offsetOfByteOffset = JSC::JSArrayBufferView::offsetOfByteOffset(); Bun__FFI__offsets.JSArrayBufferView__offsetOfVector = JSC::JSArrayBufferView::offsetOfVector(); Bun__FFI__offsets.JSCell__offsetOfType = JSC::JSCell::typeInfoTypeOffset(); + Bun__FFI__offsets.CallFrame__argumentCountIncludingThisSlot = static_cast(JSC::CallFrameSlot::argumentCountIncludingThis); + Bun__FFI__offsets.CallFrame__firstArgumentSlot = static_cast(JSC::CallFrame::argumentOffset(0)); } diff --git a/src/jsc/lib.rs b/src/jsc/lib.rs index 377b657f5ab7..8e0654edbad4 100644 --- a/src/jsc/lib.rs +++ b/src/jsc/lib.rs @@ -76,8 +76,6 @@ pub mod marked_argument_buffer; pub mod regular_expression; #[path = "ScriptExecutionStatus.rs"] pub mod script_execution_status; -#[path = "sizes.rs"] -pub mod sizes; #[path = "SourceProvider.rs"] pub mod source_provider; #[path = "TextCodec.rs"] diff --git a/src/jsc/sizes.rs b/src/jsc/sizes.rs deleted file mode 100644 index d5b83de50d55..000000000000 --- a/src/jsc/sizes.rs +++ /dev/null @@ -1,8 +0,0 @@ -//! This namespace contains JSC C++ type sizes/alignments exported from a code -//! generator. Do not rely on any of these values in new code. If possible, -//! rewrite old ones to use another approach. -//! -//! It is not reliable to interpret C++ classes as raw bytes, since the -//! memory layout is not guaranteed by the compiler. - -pub const BUN_FFI_POINTER_OFFSET_TO_ARGUMENTS_LIST: usize = 6; diff --git a/src/runtime/ffi/FFI.h b/src/runtime/ffi/FFI.h index 6a4f733c3e17..ec15923e426f 100644 --- a/src/runtime/ffi/FFI.h +++ b/src/runtime/ffi/FFI.h @@ -128,13 +128,13 @@ EncodedJSValue ValueTrue = { TagValueTrue }; typedef void* JSContext; -// Bun_FFI_PointerOffsetToArgumentsList is injected into the build -// The value is generated in `make sizegen` -// The value is 6. -// On ARM64_32, the value is something else but it really doesn't matter for our case -// However, I don't want this to subtly break amidst future upgrades to JavaScriptCore +// The Bun_FFI_PointerOffsetTo* slot indices into the JSC::CallFrame are defined from JSC::CallFrameSlot by src/jsc/bindings/ffi.cpp. #define LOAD_ARGUMENTS_FROM_CALL_FRAME \ - int64_t *argsPtr = (int64_t*)((size_t*)callFrame + Bun_FFI_PointerOffsetToArgumentsList) + int64_t *argsPtr = (int64_t*)((size_t*)callFrame + Bun_FFI_PointerOffsetToArgumentsList); \ + int32_t argsCount = ((EncodedJSValue*)((size_t*)callFrame + Bun_FFI_PointerOffsetToArgumentCountIncludingThis))->asBits.payload - 1 + +// JSC::CallFrame::argument(i) as encoded bits: an argument the caller did not pass reads as undefined, not as the stale slot. +#define ARGUMENT(i) ((i) < argsCount ? argsPtr[i] : TagValueUndefined) @@ -208,7 +208,8 @@ static uint64_t JSVALUE_TO_TYPED_ARRAY_LENGTH(EncodedJSValue val) { // This behavior change enables the JIT to handle it better // It also is better readability when console.log(myPtr) static void* JSVALUE_TO_PTR(EncodedJSValue val) { - if (val.asInt64 == TagValueNull) + // undefined (e.g. an argument that was not passed) is NULL, as in the engine's writePointerSlot. + if (val.asInt64 == TagValueNull || val.asInt64 == TagValueUndefined) return 0; if (JSCELL_IS_TYPED_ARRAY(val)) { diff --git a/src/runtime/ffi/abi_type.rs b/src/runtime/ffi/abi_type.rs index 32dc51c65896..a57fd65c0357 100644 --- a/src/runtime/ffi/abi_type.rs +++ b/src/runtime/ffi/abi_type.rs @@ -191,20 +191,6 @@ impl ABIType { }) } - /// Types that we can directly pass through as an `int64_t` - pub(crate) fn needs_a_cast_in_c(self) -> bool { - !matches!( - self, - ABIType::Char - | ABIType::Int8T - | ABIType::Uint8T - | ABIType::Int16T - | ABIType::Uint16T - | ABIType::Int32T - | ABIType::Uint32T - ) - } - pub(crate) fn is_floating_point(self) -> bool { matches!(self, ABIType::Double | ABIType::Float) } diff --git a/src/runtime/ffi/ffi_body.rs b/src/runtime/ffi/ffi_body.rs index 679a87b1e36a..14b5ec6455c7 100644 --- a/src/runtime/ffi/ffi_body.rs +++ b/src/runtime/ffi/ffi_body.rs @@ -72,12 +72,15 @@ fn dangerously_run_without_jit_protections(func: impl FnOnce() -> R) -> R { func() } +/// Mirrors `FFIFields` in `src/jsc/bindings/ffi.cpp`. #[repr(C)] struct Offsets { js_array_buffer_view_offset_of_length: u32, js_array_buffer_view_offset_of_byte_offset: u32, js_array_buffer_view_offset_of_vector: u32, js_cell_offset_of_type: u32, + call_frame_argument_count_including_this_slot: u32, + call_frame_first_argument_slot: u32, } unsafe extern "C" { @@ -2151,41 +2154,16 @@ impl Function { )?; } - if !self.arg_types.is_empty() { + // napi_env comes from `to_c` but still occupies its JS argument position, so `i` is the JS index. + if self.arg_types.iter().any(|arg| *arg != ABIType::NapiEnv) { writer.write_all(b" LOAD_ARGUMENTS_FROM_CALL_FRAME;\n")?; for (i, arg) in self.arg_types.iter().enumerate() { - if *arg == ABIType::NapiEnv { - write!( - writer, - " napi_env arg{} = (napi_env)&Bun__thisFFIModuleNapiEnv;\n argsPtr++;\n", - i - )?; - } else if *arg == ABIType::NapiValue { + if *arg != ABIType::NapiEnv { + // Member store: TinyCC emits a memset call for a brace-initialized local and a memmove call (unresolvable under -nostdlib) for a union copy. writeln!( writer, - " EncodedJSValue arg{} = {{ .asInt64 = *argsPtr++ }};", - i + " EncodedJSValue arg{i};\n arg{i}.asInt64 = ARGUMENT({i});" )?; - } else if arg.needs_a_cast_in_c() { - if i < self.arg_types.len() - 1 { - writeln!( - writer, - " EncodedJSValue arg{} = {{ .asInt64 = *argsPtr++ }};", - i - )?; - } else { - write!( - writer, - " EncodedJSValue arg{};\n arg{}.asInt64 = *argsPtr;\n", - i, i - )?; - } - } else { - if i < self.arg_types.len() - 1 { - writeln!(writer, " int64_t arg{} = *argsPtr++;", i)?; - } else { - writeln!(writer, " int64_t arg{} = *argsPtr;", i)?; - } } } } @@ -2213,11 +2191,7 @@ impl Function { let length_buf = bun_core::fmt::print_int(&mut arg_buf[3..], i); let arg_name = &arg_buf[0..3 + length_buf]; - if arg.needs_a_cast_in_c() { - write!(writer, "{}", arg.to_c(arg_name))?; - } else { - writer.write_all(arg_name)?; - } + write!(writer, "{}", arg.to_c(arg_name))?; } writer.write_all(b");\n")?; @@ -2542,7 +2516,11 @@ impl CompilerRT { state.define_symbols(&[ ( "Bun_FFI_PointerOffsetToArgumentsList", - bun_jsc::sizes::BUN_FFI_POINTER_OFFSET_TO_ARGUMENTS_LIST as i64, + offsets.call_frame_first_argument_slot as i64, + ), + ( + "Bun_FFI_PointerOffsetToArgumentCountIncludingThis", + offsets.call_frame_argument_count_including_this_slot as i64, ), ( "JSArrayBufferView__offsetOfLength", diff --git a/test/js/bun/ffi/cc.test.ts b/test/js/bun/ffi/cc.test.ts index 0726df91e629..1f37bf4ba634 100644 --- a/test/js/bun/ffi/cc.test.ts +++ b/test/js/bun/ffi/cc.test.ts @@ -75,10 +75,13 @@ describe.skipIf(isASAN)("given an add(a, b) function", () => { expect(() => res.symbols.add("1", "2")).toThrow(); }); - // looks like `b` defaults to `0`, is this U.B. or expected? - it.skip("when passed too few arguments, throws an error", () => { + // A missing argument is undefined, which converts to 0, exactly as it does + // for a dlopen() symbol. + it("when passed too few arguments, the missing ones are 0", () => { // @ts-expect-error - expect(() => res.symbols.add(1)).toThrow(); + expect(res.symbols.add(1)).toBe(1); + // @ts-expect-error + expect(res.symbols.add()).toBe(0); }); it("when passed too many arguments, still works", () => { @@ -101,6 +104,134 @@ describe.skipIf(isASAN)("given an add(a, b) function", () => { }); }); // +// The wrapper cc() compiles for a symbol used to load one call-frame slot per +// declared parameter without looking at how many arguments were passed, so a +// call with fewer arguments handed C whatever the caller's frame held past the +// last one: garbage integers, and for pointer parameters a segfault while the +// stale slot was being inspected as a JSValue. A missing argument is undefined, +// which converts the way it does for dlopen() symbols: 0 for integers and +// pointers, NaN for floating point, false for bool, undefined for napi_value. +// Runs in a subprocess because the unfixed wrapper crashes. +describe("calling a symbol with fewer arguments than it declares", () => { + it("converts the missing arguments as undefined instead of reading past the call frame", async () => { + using dir = tempDir("bun-ffi-cc-missing-args", { + // Every function reports what C received as an int, so a bad argument + // decode cannot be masked by the matching return conversion. + "arity.c": /* c */ ` + int digits(int a, int b, int c) { return a * 100 + b * 10 + c; } + int u8_value(unsigned char x) { return x; } + int bool_value(_Bool x) { return x; } + int f64_is_nan(double x) { return x != x; } + int f32_is_nan(float x) { return x != x; } + int ptr_is_null(void* p) { return p == 0; } + int cstring_is_null(const char* s) { return s == 0; } + int function_is_null(void (*f)(void)) { return f == 0; } + /* bit i is set when parameter i arrived as the value undefined converts to */ + int missing_mask(int a, double b, void* c, _Bool d) { + return (a == 0) | ((b != b) << 1) | ((c == 0) << 2) | ((d == 0) << 3); + } + typedef struct bun_test_env* env_t; + typedef struct bun_test_value* value_t; + value_t echo_napi_value(env_t env, value_t value) { return value; } + `, + "fixture.js": /* js */ ` + import { cc, ptr } from "bun:ffi"; + import path from "path"; + + const { symbols } = cc({ + source: path.join(import.meta.dir, "arity.c"), + symbols: { + digits: { args: ["i32", "i32", "i32"], returns: "i32" }, + u8_value: { args: ["u8"], returns: "i32" }, + bool_value: { args: ["bool"], returns: "i32" }, + f64_is_nan: { args: ["f64"], returns: "i32" }, + f32_is_nan: { args: ["f32"], returns: "i32" }, + ptr_is_null: { args: ["ptr"], returns: "i32" }, + cstring_is_null: { args: ["cstring"], returns: "i32" }, + function_is_null: { args: ["function"], returns: "i32" }, + missing_mask: { args: ["i32", "f64", "ptr", "bool"], returns: "i32" }, + echo_napi_value: { args: ["napi_env", "napi_value"], returns: "napi_value" }, + }, + }); + + const bytes = new Uint8Array(8); + + // Repeated calls from one call site walk through the JIT tiers, which + // is where the stale slot past the arguments tends to hold a pointer. + let nullPointers = 0; + for (let i = 0; i < 500; i++) nullPointers += symbols.ptr_is_null(); + + const results = { + digits: { + none: symbols.digits(), + one: symbols.digits(1), + two: symbols.digits(1, 2), + all: symbols.digits(1, 2, 3), + extra: symbols.digits(1, 2, 3, 4), + }, + // 200.5 is a double-encoded JSValue; integer parameters used to receive the raw slot bits. + u8: { missing: symbols.u8_value(), passed: symbols.u8_value(200), double_encoded: symbols.u8_value(200.5) }, + bool: { missing: symbols.bool_value(), passed: symbols.bool_value(true) }, + f64_is_nan: { missing: symbols.f64_is_nan(), passed: symbols.f64_is_nan(1.5) }, + f32_is_nan: { missing: symbols.f32_is_nan(), passed: symbols.f32_is_nan(1.5) }, + ptr_is_null: { + missing: symbols.ptr_is_null(), + missing_500_times: nullPointers, + undefined: symbols.ptr_is_null(undefined), + null: symbols.ptr_is_null(null), + typed_array: symbols.ptr_is_null(bytes), + address: symbols.ptr_is_null(ptr(bytes)), + }, + cstring_is_null: { missing: symbols.cstring_is_null(), undefined: symbols.cstring_is_null(undefined) }, + // The engine's FFI throws a TypeError for an undefined callback; the cc() wrapper + // has no throwing conversion yet (#37989 adds it), so C receives NULL until then. + function_is_null: { missing: symbols.function_is_null(), undefined: symbols.function_is_null(undefined) }, + missing_mask: { + none: symbols.missing_mask(), + first_only: symbols.missing_mask(5), + all: symbols.missing_mask(5, 2.5, bytes, true), + }, + // napi_env takes up position 0 of the JS argument list but is filled in by the wrapper. + echo_napi_value: { + none: typeof symbols.echo_napi_value(), + placeholder_only: typeof symbols.echo_napi_value(null), + passed: symbols.echo_napi_value(undefined, 42), + }, + }; + 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]); + + // stderr is included in the received object so failures show it, but is not + // asserted empty: debug builds emit benign startup warnings. + const results = stdout.startsWith("{") ? JSON.parse(stdout) : stdout; + expect({ results, stderr, exitCode }).toMatchObject({ + results: { + digits: { none: 0, one: 100, two: 120, all: 123, extra: 123 }, + u8: { missing: 0, passed: 200, double_encoded: 200 }, + bool: { missing: 0, passed: 1 }, + f64_is_nan: { missing: 1, passed: 0 }, + f32_is_nan: { missing: 1, passed: 0 }, + ptr_is_null: { missing: 1, missing_500_times: 500, undefined: 1, null: 1, typed_array: 0, address: 0 }, + cstring_is_null: { missing: 1, undefined: 1 }, + function_is_null: { missing: 1, undefined: 1 }, + missing_mask: { none: 0b1111, first_only: 0b1110, all: 0 }, + echo_napi_value: { none: "undefined", placeholder_only: "undefined", passed: 42 }, + }, + exitCode: 0, + }); + }); +}); + describe("given a source file with syntax errors", () => { const source = /* c */ ` int add(int a, int b) { diff --git a/test/js/bun/ffi/ffi.test.fixture.receiver.c b/test/js/bun/ffi/ffi.test.fixture.receiver.c index 03c89180e442..f33c302bd4d0 100644 --- a/test/js/bun/ffi/ffi.test.fixture.receiver.c +++ b/test/js/bun/ffi/ffi.test.fixture.receiver.c @@ -130,13 +130,13 @@ EncodedJSValue ValueTrue = { TagValueTrue }; typedef void* JSContext; -// Bun_FFI_PointerOffsetToArgumentsList is injected into the build -// The value is generated in `make sizegen` -// The value is 6. -// On ARM64_32, the value is something else but it really doesn't matter for our case -// However, I don't want this to subtly break amidst future upgrades to JavaScriptCore +// The Bun_FFI_PointerOffsetTo* slot indices into the JSC::CallFrame are defined from JSC::CallFrameSlot by src/jsc/bindings/ffi.cpp. #define LOAD_ARGUMENTS_FROM_CALL_FRAME \ - int64_t *argsPtr = (int64_t*)((size_t*)callFrame + Bun_FFI_PointerOffsetToArgumentsList) + int64_t *argsPtr = (int64_t*)((size_t*)callFrame + Bun_FFI_PointerOffsetToArgumentsList); \ + int32_t argsCount = ((EncodedJSValue*)((size_t*)callFrame + Bun_FFI_PointerOffsetToArgumentCountIncludingThis))->asBits.payload - 1 + +// JSC::CallFrame::argument(i) as encoded bits: an argument the caller did not pass reads as undefined, not as the stale slot. +#define ARGUMENT(i) ((i) < argsCount ? argsPtr[i] : TagValueUndefined) @@ -210,7 +210,8 @@ static uint64_t JSVALUE_TO_TYPED_ARRAY_LENGTH(EncodedJSValue val) { // This behavior change enables the JIT to handle it better // It also is better readability when console.log(myPtr) static void* JSVALUE_TO_PTR(EncodedJSValue val) { - if (val.asInt64 == TagValueNull) + // undefined (e.g. an argument that was not passed) is NULL, as in the engine's writePointerSlot. + if (val.asInt64 == TagValueNull || val.asInt64 == TagValueUndefined) return 0; if (JSCELL_IS_TYPED_ARRAY(val)) { @@ -375,7 +376,7 @@ float not_a_callback(float arg0); ZIG_REPR_TYPE JSFunctionCall(void* JS_GLOBAL_OBJECT, void* callFrame) { LOAD_ARGUMENTS_FROM_CALL_FRAME; EncodedJSValue arg0; - arg0.asInt64 = *argsPtr; + arg0.asInt64 = ARGUMENT(0); float return_value = not_a_callback( JSVALUE_TO_FLOAT(arg0)); return FLOAT_TO_JSVALUE(return_value).asZigRepr;