[JSC] Read an FFI buffer address after the argument conversions that can run JS

JSTests/stress/ffi-argument-buffer-snapshot.js

@@ -0,0 +1,211 @@
+//@ requireOptions("--useDollarVM=1", "--useConcurrentJIT=0", "--jitPolicyScale=0", "--useExecutableAllocationFuzz=false")
+
+// Marshalling one FFI argument list can run JS in the middle of it: an integer or a float argument
+// coerces through valueOf / Symbol.toPrimitive, and an object passed for "ptr" has its "ptr"
+// property read. That JS can detach, transfer, or resize a buffer another argument is read from,
+// and then the address or the "buffer_length" of that argument describes memory that is gone. So
+// every tier reads the addresses and the lengths after all of that JS has run: the pointer of a
+// detached view is null, and the length behind a shrunk view is the new one. Each case runs cold
+// (the C++ host path) and hot through a monomorphic exact-arity caller (the IC stub, the DFG, the
+// FTL), so no tier may disagree.
+
+if (!$vm.useJIT())
+    quit();
+
+const fixture = name => $vm.ffiFixture(name);
+
+// ffi_ptr_identity returns its first argument, so the call answers with the address the engine
+// read for that argument: null once the view behind it is detached.
+const pointerOfThenInt = $vm.ffiFunction({ args: ["ptr", "i32"], returns: "ptr" }, fixture("ffi_ptr_identity"), "ffi_ptr_identity(ptr,i32)");
+const pointerOfThenPointer = $vm.ffiFunction({ args: ["ptr", "ptr"], returns: "ptr" }, fixture("ffi_ptr_identity"), "ffi_ptr_identity(ptr,ptr)");
+// "cstring" takes a view like "ptr" does, and an untyped "cstring" argument also makes the DFG
+// and the FTL bracket the call with the string arena.
+const pointerOfCStringThenInt = $vm.ffiFunction({ args: ["cstring", "i32"], returns: "ptr" }, fixture("ffi_ptr_identity"), "ffi_ptr_identity(cstring,i32)");
+// The widest signature there is: the view first, the conversion that runs JS last.
+const pointerOfThenManyInts = $vm.ffiFunction({ args: ["ptr", ...Array(31).fill("i32")], returns: "ptr" }, fixture("ffi_ptr_identity"), "ffi_ptr_identity(32 arguments)");
+// ffi_add_u64 adds its two arguments, so one answer carries both marshalled slots.
+const sumOfPointers = $vm.ffiFunction({ args: ["ptr", "ptr"], returns: "u64" }, fixture("ffi_add_u64"), "ffi_add_u64(ptr,ptr)");
+// ffi_view_byte_length returns the length it was given. ffi_view_last_byte reads the last byte of
+// the address and length pair it was given, which reads out of bounds if the two disagree.
+const byteLength = $vm.ffiFunction({ args: ["buffer", "buffer_length", "i32"], returns: "u64" }, fixture("ffi_view_byte_length"), "ffi_view_byte_length");
+const lastByte = $vm.ffiFunction({ args: ["buffer", "buffer_length", "i32"], returns: "i32" }, fixture("ffi_view_last_byte"), "ffi_view_last_byte");
+
+let failures = 0;
+function check(actual, expected, label) {
+    if (Object.is(actual, expected))
+        return;
+    print(`FAIL [${label}]: got ${String(actual)} (${typeof actual}), expected ${String(expected)} (${typeof expected})`);
+    if (++failures > 8)
+        throw new Error("too many failures");
+}
+
+// A NEW caller function per case and per tier that holds its callable as a closure constant and
+// passes exactly as many arguments as the signature takes: the only call site shape the DFG
+// converts to a CallFFI node. `tag` keeps the source text unique, because two `new Function` calls
+// with the same text share one unlinked code block, and then only the first caller gets the
+// conversion.
+function makeCaller(callable, arity, tag) {
+    const argumentList = Array.from({ length: arity }, (_, i) => "args[" + i + "]").join(", ");
+    return new Function("callable", `/* ${tag} */ return function (args) { return callable(${argumentList}); };`)(callable);
+}
+
+// The nested case makes FFI calls outside its hot caller. They go through these two helpers, which
+// are never inlined and are compiled in every tier right here, before any case reads the compile
+// counts. So a CallFFI node that is compiled while a case runs belongs to that case's hot caller.
+function addressOf(view) { return BigInt(pointerOfThenInt(view, 0)); }
+function byteLengthOf(view) { return byteLength(view, view, 0); }
+noInline(addressOf);
+noInline(byteLengthOf);
+{
+    const view = new Uint8Array(64);
+    for (let i = 0; i < 1e3; ++i) {
+        addressOf(view);
+        byteLengthOf(view);
+    }
+}
+
+// Every call below detaches or shrinks the buffer it gets, so each one needs a buffer of its own.
+const cases = [
+    {
+        name: "valueOf transfers the view passed for ptr",
+        callable: pointerOfThenInt,
+        arity: 2,
+        setUp() {
+            const view = new Uint8Array(64);
+            return { args: [view, { valueOf() { view.buffer.transfer(); return 1; } }], expected: null };
+        },
+    },
+    {
+        name: "Symbol.toPrimitive transfers the view passed for ptr",
+        callable: pointerOfThenInt,
+        arity: 2,
+        setUp() {
+            const view = new Uint8Array(64);
+            return { args: [view, { [Symbol.toPrimitive]() { view.buffer.transfer(); return 1; } }], expected: null };
+        },
+    },
+    {
+        name: "a later ptr getter transfers the view passed for ptr",
+        callable: pointerOfThenPointer,
+        arity: 2,
+        setUp() {
+            const view = new Uint8Array(64);
+            return { args: [view, { get ptr() { view.buffer.transfer(); return 1; } }], expected: null };
+        },
+    },
+    {
+        name: "valueOf shrinks the view behind buffer_length",
+        callable: byteLength,
+        arity: 3,
+        setUp() {
+            const buffer = new ArrayBuffer(1024, { maxByteLength: 1024 });
+            const view = new Uint8Array(buffer).fill(7);
+            return { args: [view, view, { valueOf() { buffer.resize(16); return 1; } }], expected: 16n };
+        },
+    },
+    {
+        name: "the address and buffer_length still agree after a shrink",
+        callable: lastByte,
+        arity: 3,
+        setUp() {
+            const buffer = new ArrayBuffer(1024, { maxByteLength: 1024 });
+            const view = new Uint8Array(buffer).fill(7);
+            return { args: [view, view, { valueOf() { buffer.resize(16); return 1; } }], expected: 7 };
+        },
+    },
+    {
+        name: "valueOf transfers the view passed for cstring",
+        callable: pointerOfCStringThenInt,
+        arity: 2,
+        setUp() {
+            const view = new Uint8Array(64);
+            return { args: [view, { valueOf() { view.buffer.transfer(); return 1; } }], expected: null };
+        },
+    },
+    {
+        name: "the last of 32 arguments transfers the view passed for the first",
+        callable: pointerOfThenManyInts,
+        arity: 32,
+        setUp() {
+            const view = new Uint8Array(64);
+            const args = [view];
+            for (let i = 1; i < 31; ++i)
+                args.push(i);
+            args.push({ valueOf() { view.buffer.transfer(); return 1; } });
+            return { args, expected: null };
+        },
+    },
+    {
+        // The getter makes a whole FFI call of its own before the outer call reads its first
+        // argument again. The answer is the live address plus the 1 the getter returned.
+        name: "a nested FFI call inside a ptr getter leaves the outer arguments alone",
+        callable: sumOfPointers,
+        arity: 2,
+        setUp() {
+            const view = new Uint8Array(64);
+            const nested = new Uint8Array(1024);
+            const argument = {
+                get ptr() {
+                    if (byteLengthOf(nested) !== 1024n)
+                        throw new Error("the nested call marshalled the wrong length");
+                    return 1;
+                },
+            };
+            return { args: [view, argument], expected: addressOf(view) + 1n };
+        },
+    },
+    {
+        name: "a live view keeps its length",
+        callable: byteLength,
+        arity: 3,
+        setUp() {
+            const view = new Uint8Array(1024).fill(7);
+            return { args: [view, view, { valueOf() { return 1; } }], expected: 1024n };
+        },
+    },
+];
+
+function runCase(testCase, caller, label) {
+    const { args, expected } = testCase.setUp();
+    check(caller(args), expected, label + ": " + testCase.name);
+}
+
+for (const testCase of cases) {
+    // Cold: this call site has nothing compiled yet, so the call runs the C++ host path (the IC
+    // stub hands every argument that needs real conversion to it).
+    runCase(testCase, makeCaller(testCase.callable, testCase.arity, "cold " + testCase.name), "cold");
+
+    const before = $vm.ffiCompileCounts();
+    const hot = makeCaller(testCase.callable, testCase.arity, "hot " + testCase.name);
+    noInline(hot);
+    for (let i = 0; i < 1e3; ++i)
+        runCase(testCase, hot, "hot");
+    // The loop must have reached the inline conversions the DFG and the FTL emit, not only the
+    // host path: that is where a snapshot an earlier argument took has to be taken again.
+    if (numberOfDFGCompiles(hot) > 0) {
+        const counts = $vm.ffiCompileCounts();
+        if (counts.dfgCallFFI + counts.ftlCallFFI === before.dfgCallFFI + before.ftlCallFFI)
+            throw new Error("the DFG compiled the hot caller but no CallFFI node was compiled: " + JSON.stringify(counts));
+    }
+}
+
+// A getter on a pointer argument must run exactly once: reading an address again after JS has run
+// must not read the property a second time.
+{
+    const view = new Uint8Array(64);
+    let reads = 0;
+    const argument = { get ptr() { ++reads; return 1; } };
+    const hot = makeCaller(pointerOfThenPointer, 2, "ptr getter runs once");
+    noInline(hot);
+    for (let i = 0; i < 1e3; ++i) {
+        reads = 0;
+        hot([view, argument]);
+        if (reads !== 1)
+            throw new Error("the 'ptr' getter ran " + reads + " times, expected once");
+    }
+    // The live view still marshals to a real address.
+    check(hot([view, argument]) === null, false, "a live view has a non-null address");
+}
+
+if (failures)
+    throw new Error(failures + " case(s) failed");

Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -2660,8 +2660,10 @@ ByteCodeParser::CallOptimizationResult ByteCodeParser::handleInlining(
                     m_graph.m_plan.recordedStatuses().addCallLinkStatus(currentNodeOrigin().semantic, CallLinkStatus(callee));
                     auto* frozenFunction = m_graph.freeze(ffiFunction);
                     addToGraph(CheckIsConstant, OpInfo(frozenFunction), Edge(callTargetNode, CellUse));
+                    // The backend marshals the call in this area: one slot per argument, the return
+                    // slot, then the parked argument values (see FFI::argumentValueOffset).
                     m_parameterSlots = std::max(m_parameterSlots, Graph::parameterSlotsForArgCount(
-                        std::max<unsigned>(ffiFunction->signature().slotCount() + 1, argumentCountIncludingThis)));
+                        std::max<unsigned>(ffiFunction->signature().slotCount() + ffiFunction->signature().argumentCount() + 1, argumentCountIncludingThis)));
                     addCall(result, Call, OpInfo(), jsConstant(frozenFunction), argumentCountIncludingThis, registerOffset, prediction);
                     return CallOptimizationResult::Inlined;
                 }

Source/JavaScriptCore/ffi/FFICallHost.cpp

@@ -59,9 +59,28 @@ static ALWAYS_INLINE EncodedJSValue ffiCall(JSGlobalObject* globalObject, CallFr
     ASSERT(argumentCount <= Signature::maxArguments);
     uint64_t slots[Signature::maxArguments + 1];
 
+    // Two passes over the arguments. A conversion can run JS: an integer or a float argument
+    // coerces through valueOf or Symbol.toPrimitive, and an object passed for a pointer has its
+    // 'ptr' property read. That JS can detach, transfer, or resize a buffer, so every address and
+    // byte length read off a live buffer is taken in the second pass, after all of it has run.
+    // Without that, well-behaved C gets a pointer and a length for memory that is gone.
+    static_assert(Signature::maxArguments <= 32);
+    uint32_t snapshots = 0;
     for (unsigned i = 0; i < argumentCount; ++i) {
         Type type = signature.argumentType(i);
-        writeSlotFromJSValue(globalObject, context, type, callFrame->argument(i), slots[i], &context.stringArena());
+        JSValue argument = callFrame->argument(i);
+        if (slotIsBufferSnapshot(type, argument)) {
+            snapshots |= 1u << i;
+            continue;
+        }
+        writeSlotFromJSValue(globalObject, context, type, argument, slots[i], &context.stringArena());
+        RETURN_IF_EXCEPTION(scope, { });
+    }
+    for (unsigned i = 0; snapshots; ++i) {
+        if (!(snapshots & (1u << i)))
+            continue;
+        snapshots &= ~(1u << i);
+        writeSlotFromJSValue(globalObject, context, signature.argumentType(i), callFrame->argument(i), slots[i], &context.stringArena());
         RETURN_IF_EXCEPTION(scope, { });
     }
     slots[argumentCount] = 0;

Source/JavaScriptCore/ffi/FFIConversions.cpp

@@ -33,6 +33,7 @@
 #include "Error.h"
 #include "ExceptionHelpers.h"
 #include "FFIContext.h"
+#include "FFISignature.h"
 #include "FFIType.h"
 #include "FrameTracers.h"
 #include "JSArrayBuffer.h"
@@ -431,6 +432,25 @@ bool writeSlotFromJSValue(JSGlobalObject* globalObject, FFIContext& context, Typ
     return false;
 }
 
+bool slotIsBufferSnapshot(Type type, JSValue value)
+{
+    switch (type) {
+    case Type::Pointer:
+    case Type::CString:
+    case Type::Function:
+    case Type::Buffer:
+    case Type::BufferLength:
+        break;
+    default:
+        return false;
+    }
+
+    if (!value.isCell())
+        return false;
+    JSCell* cell = value.asCell();
+    return dynamicDowncast<JSArrayBufferView>(cell) || dynamicDowncast<JSArrayBuffer>(cell);
+}
+
 JSValue jsValueFromSlot(JSGlobalObject* globalObject, FFIContext&, Type type, uint64_t slot)
 {
     switch (type) {
@@ -525,6 +545,39 @@ JSC_DEFINE_JIT_OPERATION(operationFFIWriteSlot, void, (JSGlobalObject* globalObj
     OPERATION_RETURN(scope);
 }
 
+JSC_DEFINE_JIT_OPERATION(operationFFIWriteSlotAndRetakeSnapshots, void, (JSGlobalObject* globalObject, FFI::Signature* signature, uint32_t index, EncodedJSValue value, uint64_t* slot))
+{
+    VM& vm = globalObject->vm();
+    CallFrame* callFrame = DECLARE_CALL_FRAME(vm);
+    JITOperationPrologueCallFrameTracer tracer(vm, callFrame);
+    auto scope = DECLARE_THROW_SCOPE(vm);
+
+    ASSERT(signature);
+    ASSERT(index < signature->argumentCount());
+    FFI::FFIContext& context = globalObject->ffiContext();
+    uint64_t* slots = slot - index;
+    const EncodedJSValue* parkedValues = reinterpret_cast<const EncodedJSValue*>(slots + signature->slotCount());
+
+    FFI::writeSlotFromJSValue(globalObject, context, signature->argumentType(index), JSValue::decode(value), *slot, &context.arena());
+    OPERATION_RETURN_IF_EXCEPTION(scope);
+
+    // This conversion can have run JS: an integer or a float argument coerces through valueOf or
+    // Symbol.toPrimitive, and an object passed for a pointer has its 'ptr' property read. That JS
+    // can detach, transfer, or resize a buffer an earlier argument took its address or its byte
+    // length from. So take those snapshots again. Only the parked arguments that are still a
+    // snapshot are read again: a 'ptr' property getter must not run a second time.
+    for (unsigned i = 0; i < index; ++i) {
+        FFI::Type type = signature->argumentType(i);
+        JSValue parked = JSValue::decode(parkedValues[i]);
+        if (!FFI::slotIsBufferSnapshot(type, parked))
+            continue;
+        FFI::writeSlotFromJSValue(globalObject, context, type, parked, slots[i], &context.arena());
+        OPERATION_RETURN_IF_EXCEPTION(scope);
+    }
+
+    OPERATION_RETURN(scope);
+}
+
 JSC_DEFINE_JIT_OPERATION(operationFFIArenaEnter, void, (JSGlobalObject* globalObject))
 {
     VM& vm = globalObject->vm();

Source/JavaScriptCore/ffi/FFIConversions.h

@@ -42,8 +42,17 @@ class JSGlobalObject;
 
 namespace FFI {
 
+class Signature;
+
 JS_EXPORT_PRIVATE bool writeSlotFromJSValue(JSGlobalObject*, FFIContext&, Type, JSValue, uint64_t& slotOut, StringArena* arena);
 
+// True when writeSlotFromJSValue fills this slot off the live bytes of a TypedArray, a DataView,
+// or an ArrayBuffer: the address of those bytes for a pointer, how many of them there are for
+// 'buffer_length'. Such a write is a snapshot, and JS that runs after it can detach, transfer, or
+// resize that buffer. Then the address points at freed memory and the length describes bytes that
+// are gone. So a snapshot is always taken after the conversions that can run JS.
+JS_EXPORT_PRIVATE bool slotIsBufferSnapshot(Type, JSValue);
+
 JS_EXPORT_PRIVATE JSValue jsValueFromSlot(JSGlobalObject*, FFIContext&, Type, uint64_t slot);
 
 JS_EXPORT_PRIVATE JSValue pointerToJSValue(JSGlobalObject*, uint64_t address);
@@ -57,6 +66,11 @@ JS_EXPORT_PRIVATE int64_t doubleToInt64(double);
 
 JSC_DECLARE_JIT_OPERATION(operationFFIBoxSlot, EncodedJSValue, (JSGlobalObject*, uint32_t typeTag, uint64_t slot, int32_t exitArena));
 JSC_DECLARE_JIT_OPERATION(operationFFIWriteSlot, void, (JSGlobalObject*, FFI::FFIContext*, uint32_t typeTag, EncodedJSValue value, uint64_t* slot));
+// Writes argument `index`'s slot, whose address is `slot`, and then takes the buffer snapshot of
+// every earlier argument again. Use it instead of operationFFIWriteSlot when this conversion can
+// run JS and an earlier argument already took one. The earlier arguments come from the parked
+// JSValues above the slot buffer (see FFI::argumentValueOffset).
+JSC_DECLARE_JIT_OPERATION(operationFFIWriteSlotAndRetakeSnapshots, void, (JSGlobalObject*, FFI::Signature*, uint32_t index, EncodedJSValue value, uint64_t* slot));
 JSC_DECLARE_JIT_OPERATION(operationFFIArenaEnter, void, (JSGlobalObject*));
 JSC_DECLARE_JIT_OPERATION(operationFFIArenaExit, void, (JSGlobalObject*));