fix(ext/ffi): Null buffer pointer value is inconsistent (#16625)

Currently, slow call path will always create a dangling pointer to
replace a null pointer when called with eg. a `new Uint8Array()`
parameter, which V8 initialises as a null pointer backed buffer.

However, the fast call path will never change the pointer value and will
thus expose a null pointer. Thus, it's possible that the pointer value
that a native call sees coming from Deno changes between two sequential
invocations of the same function with the exact same parameters.

Since null pointers can be quite important, and `Uint8Array` is the
chosen fast path for Deno FFI `"buffer"` parameters, I think it is
fairly important that the null pointer be properly exposed to the native
code. Thus this PR.

### `*mut c_void`
While here, I also changed the type of our pointer values to `*mut
c_void`. This is mainly due to JS buffers always being `*mut`, and
because we offer a way to turn a pointer into a JS `ArrayBuffer`
(`op_ffi_get_buf`) which is read-write. I'm not exactly sure which way
we should really go here, we have pointers that are definitely mut but
we also cannot assume all of our pointers are. So, do we go with the
maxima or the minima?

### `optimisedCall(new Uint8Array())`
V8 seems to have a bug where calling an optimised function with a newly
created empty `Uint8Array` (no argument or 0) will not see the data
pointer being null but instead it's some stable pointer, perhaps
pointing to some internal null-backing-store. The pointer value is also
an odd (not even) number, so it might specifically be a tagged pointer.

This will probably be an issue for some users, if they try to use eg.
`method(cstr("something"), new Uint8Array())` as a way to do a fast call
to `method` with a null pointer as the second parameter.

If instead of a `new Uint8Array()` the user instead uses some `const
NULL = new Uint8Array()` where the `NULL` buffer has been passed to a
slow call previously, then the fast call will properly see a null
pointer.

I'll take this up with some V8 engineers to see if this couldn't be
fixed.

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::<v8::BigInt>::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::<v8::Number>::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::<v8::ArrayBuffer>::try_from(arg) {
-    let backing_store = value.get_backing_store();
+    if let Some(non_null) = value.data() {
-    &backing_store[..] as *const _ as *const u8
+      non_null.as_ptr()
+    } else {
+      ptr::null_mut()
+    }
   } else if let Ok(value) = v8::Local::<v8::ArrayBufferView>::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();
+      .data();
-    &backing_store[byte_offset..] as *const _ as *const u8
+    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::<v8::BigInt>::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::<v8::Number>::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),
         }
       }
     })

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 {
+pub extern "C" fn is_null_ptr(ptr: *const u8) -> bool {
-  ptr.is_null() as u8
+  ptr.is_null()
 }
 
 #[no_mangle]

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(dylib.symbols.is_null_ptr(ptr0));
-console.log(Boolean(dylib.symbols.is_null_ptr(null)));
+console.log(dylib.symbols.is_null_ptr(null));
-console.log(Boolean(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(into))));
+console.log(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(into)));
-const emptyBuffer = new BigUint64Array(0);
+const emptyBuffer = new Uint8Array(0);
-console.log(Boolean(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptyBuffer))));
+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<ArrayBuffer>->Data()` method returns null pointers for zero-sized buffers.
+// Using `Local<ArrayBuffer>->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<ArrayBuffer>->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, {