diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs index 65d64af337..52e101a92e 100644 --- a/ext/ffi/lib.rs +++ b/ext/ffi/lib.rs @@ -303,7 +303,7 @@ union NativeValue { isize_value: isize, f32_value: f32, f64_value: f64, - pointer: *const u8, + pointer: *mut c_void, } impl NativeValue { @@ -972,11 +972,11 @@ fn ffi_parse_pointer_arg( // 2. Number: Common and supported by Fast API. // 3. Null: Very uncommon / can be represented by a 0. let pointer = if let Ok(value) = v8::Local::::try_from(arg) { - value.u64_value().0 as usize as *const u8 + value.u64_value().0 as usize as *mut c_void } else if let Ok(value) = v8::Local::::try_from(arg) { - value.integer_value(scope).unwrap() as usize as *const u8 + value.integer_value(scope).unwrap() as usize as *mut c_void } else if arg.is_null() { - ptr::null() + ptr::null_mut() } else { return Err(type_error( "Invalid FFI pointer type, expected null, integer or BigInt", @@ -996,19 +996,28 @@ fn ffi_parse_buffer_arg( // 5. Null: Very uncommon / can be represented by a 0. let pointer = if let Ok(value) = v8::Local::::try_from(arg) { - let backing_store = value.get_backing_store(); - &backing_store[..] as *const _ as *const u8 + if let Some(non_null) = value.data() { + non_null.as_ptr() + } else { + ptr::null_mut() + } } else if let Ok(value) = v8::Local::::try_from(arg) { let byte_offset = value.byte_offset(); - let backing_store = value + let pointer = value .buffer(scope) .ok_or_else(|| { type_error("Invalid FFI ArrayBufferView, expected data in the buffer") })? - .get_backing_store(); - &backing_store[byte_offset..] as *const _ as *const u8 + .data(); + if let Some(non_null) = pointer { + // SAFETY: Pointer is non-null, and V8 guarantees that the byte_offset + // is within the buffer backing store. + unsafe { non_null.as_ptr().add(byte_offset) } + } else { + ptr::null_mut() + } } else if arg.is_null() { - ptr::null() + ptr::null_mut() } else { return Err(type_error( "Invalid FFI buffer type, expected null, ArrayBuffer, or ArrayBufferView", @@ -1027,11 +1036,11 @@ fn ffi_parse_function_arg( // 2. Number: Common and supported by Fast API, optimise this case as second. // 3. Null: Very uncommon / can be represented by a 0. let pointer = if let Ok(value) = v8::Local::::try_from(arg) { - value.u64_value().0 as usize as *const u8 + value.u64_value().0 as usize as *mut c_void } else if let Ok(value) = v8::Local::::try_from(arg) { - value.integer_value(scope).unwrap() as usize as *const u8 + value.integer_value(scope).unwrap() as usize as *mut c_void } else if arg.is_null() { - ptr::null() + ptr::null_mut() } else { return Err(type_error( "Invalid FFI function type, expected null, integer, or BigInt", @@ -1241,7 +1250,7 @@ where }, NativeType::Pointer | NativeType::Function | NativeType::Buffer => { NativeValue { - pointer: cif.call::<*const u8>(*fun_ptr, &call_args), + pointer: cif.call::<*mut c_void>(*fun_ptr, &call_args), } } }) @@ -1312,7 +1321,7 @@ fn ffi_call( }, NativeType::Pointer | NativeType::Function | NativeType::Buffer => { NativeValue { - pointer: cif.call::<*const u8>(fun_ptr, &call_args), + pointer: cif.call::<*mut c_void>(fun_ptr, &call_args), } } }) diff --git a/test_ffi/src/lib.rs b/test_ffi/src/lib.rs index be0d2e42f8..1c1770bf02 100644 --- a/test_ffi/src/lib.rs +++ b/test_ffi/src/lib.rs @@ -45,8 +45,8 @@ pub extern "C" fn return_buffer() -> *const u8 { } #[no_mangle] -pub extern "C" fn is_null_ptr(ptr: *const u8) -> u8 { - ptr.is_null() as u8 +pub extern "C" fn is_null_ptr(ptr: *const u8) -> bool { + ptr.is_null() } #[no_mangle] diff --git a/test_ffi/tests/test.js b/test_ffi/tests/test.js index 0af94b9065..65cb974f91 100644 --- a/test_ffi/tests/test.js +++ b/test_ffi/tests/test.js @@ -3,7 +3,7 @@ // Run using cargo test or `--v8-options=--allow-natives-syntax` -import { assertEquals } from "https://deno.land/std@0.149.0/testing/asserts.ts"; +import { assertEquals, assertNotEquals } from "https://deno.land/std@0.149.0/testing/asserts.ts"; import { assertThrows, assert, @@ -50,7 +50,8 @@ const dylib = Deno.dlopen(libPath, { result: "void", }, "return_buffer": { parameters: [], result: "buffer" }, - "is_null_ptr": { parameters: ["pointer"], result: "u8" }, + "is_null_ptr": { parameters: ["pointer"], result: "bool" }, + "is_null_buf": { name: "is_null_ptr", parameters: ["buffer"], result: "bool" }, "add_u32": { parameters: ["u32", "u32"], result: "u32" }, "add_i32": { parameters: ["i32", "i32"], result: "i32" }, "add_u64": { parameters: ["u64", "u64"], result: "u64" }, @@ -251,13 +252,56 @@ const stringPtr = Deno.UnsafePointer.of(string); const stringPtrview = new Deno.UnsafePointerView(stringPtr); console.log(stringPtrview.getCString()); console.log(stringPtrview.getCString(11)); -console.log(Boolean(dylib.symbols.is_null_ptr(ptr0))); -console.log(Boolean(dylib.symbols.is_null_ptr(null))); -console.log(Boolean(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(into)))); -const emptyBuffer = new BigUint64Array(0); -console.log(Boolean(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptyBuffer)))); +console.log(dylib.symbols.is_null_ptr(ptr0)); +console.log(dylib.symbols.is_null_ptr(null)); +console.log(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(into))); +const emptyBuffer = new Uint8Array(0); +console.log(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptyBuffer))); const emptySlice = into.subarray(6); -console.log(Boolean(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptySlice)))); +console.log(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptySlice))); + +const { is_null_buf } = symbols; +function isNullBuffer(buffer) { return is_null_buf(buffer); }; +function isNullBufferDeopt(buffer) { return is_null_buf(buffer); }; +%PrepareFunctionForOptimization(isNullBuffer); +isNullBuffer(emptyBuffer); +%NeverOptimizeFunction(isNullBufferDeopt); +%OptimizeFunctionOnNextCall(isNullBuffer); +isNullBuffer(emptyBuffer); +assertIsOptimized(isNullBuffer); + +// ==== ZERO LENGTH BUFFER TESTS ==== +assertEquals(isNullBuffer(emptyBuffer), true, "isNullBuffer(emptyBuffer) !== true"); +assertEquals(isNullBufferDeopt(emptyBuffer), true, "isNullBufferDeopt(emptyBuffer) !== true"); +assertEquals(isNullBuffer(emptySlice), false, "isNullBuffer(emptySlice) !== false"); +assertEquals(isNullBufferDeopt(emptySlice), false, "isNullBufferDeopt(emptySlice) !== false"); +assertEquals(isNullBufferDeopt(new Uint8Array()), true, "isNullBufferDeopt(new Uint8Array()) !== true"); + +// ==== V8 ZERO LENGTH BUFFER ANOMALIES ==== + +// V8 bug: inline Uint8Array creation to fast call sees non-null pointer +// https://bugs.chromium.org/p/v8/issues/detail?id=13489 +assertEquals(isNullBuffer(new Uint8Array()), false, "isNullBuffer(new Uint8Array()) !== false"); + +// Externally backed ArrayBuffer has a non-null data pointer, even though its length is zero. +const externalZeroBuffer = new Uint8Array(Deno.UnsafePointerView.getArrayBuffer(ptr0, 0)); +// However: V8 Fast calls get null pointers for zero-sized buffers. +assertEquals(isNullBuffer(externalZeroBuffer), true, "isNullBuffer(externalZeroBuffer) !== true"); +// Also: V8's `Local->Data()` method returns null pointers for zero-sized buffers. +// Using `Local->GetBackingStore()->Data()` would give the original pointer. +assertEquals(isNullBufferDeopt(externalZeroBuffer), true, "isNullBufferDeopt(externalZeroBuffer) !== true"); + +// The same pointer with a non-zero byte length for the buffer will return non-null pointers in +// both Fast call and V8 API calls. +const externalOneBuffer = new Uint8Array(Deno.UnsafePointerView.getArrayBuffer(ptr0, 1)); +assertEquals(isNullBuffer(externalOneBuffer), false, "isNullBuffer(externalOneBuffer) !== false"); +assertEquals(isNullBufferDeopt(externalOneBuffer), false, "isNullBufferDeopt(externalOneBuffer) !== false"); + +// Due to ops macro using `Local->Data()` to get the pointer for the slice that is then used to get +// the pointer of an ArrayBuffer / TypedArray, the same effect can be seen where a zero byte length buffer returns +// a null pointer as its pointer value. +assertEquals(Deno.UnsafePointer.of(externalZeroBuffer), 0, "Deno.UnsafePointer.of(externalZeroBuffer) !== 0"); +assertNotEquals(Deno.UnsafePointer.of(externalOneBuffer), 0, "Deno.UnsafePointer.of(externalOneBuffer) !== 0"); const addU32Ptr = dylib.symbols.get_add_u32_ptr(); const addU32 = new Deno.UnsafeFnPointer(addU32Ptr, {