chore(ext/ffi): migrate from op -> op2 for ffi (#20509)

Migrate to op2. Making a few decisions to get this across the line:

- Empty slices, no matter where the come from, are null pointers. The v8
bugs (https://bugs.chromium.org/p/v8/issues/detail?id=13489) and
(https://bugs.chromium.org/p/v8/issues/detail?id=13488) make passing
around zero-length slice pointers too dangerous as they might be
uninitialized or null data.
- Offsets and lengths are `#[number] isize` and `#[number] usize`
respectively -- 53 bits should be enough for anyone
- Pointers are bigints. This is a u64 in the fastcall world, and can
accept Integer/Int32/Number/BigInt v8 types in the slow world.

ext/ffi/00_ffi.js

@@ -5,6 +5,7 @@ const ops = core.ops;
 const primordials = globalThis.__bootstrap.primordials;
 const {
   ArrayBufferIsView,
+  ArrayBufferPrototype,
   ArrayBufferPrototypeGetByteLength,
   ArrayPrototypeMap,
   ArrayPrototypeJoin,
@@ -221,7 +222,24 @@ class UnsafePointer {
     if (ObjectPrototypeIsPrototypeOf(UnsafeCallbackPrototype, value)) {
       return value.pointer;
     }
-    const pointer = ops.op_ffi_ptr_of(value);
+    let pointer;
+    if (ArrayBufferIsView(value)) {
+      if (value.length === 0) {
+        pointer = ops.op_ffi_ptr_of_exact(value);
+      } else {
+        pointer = ops.op_ffi_ptr_of(value);
+      }
+    } else if (ObjectPrototypeIsPrototypeOf(ArrayBufferPrototype, value)) {
+      if (value.length === 0) {
+        pointer = ops.op_ffi_ptr_of_exact(new Uint8Array(value));
+      } else {
+        pointer = ops.op_ffi_ptr_of(new Uint8Array(value));
+      }
+    } else {
+      throw new TypeError(
+        "Expected ArrayBuffer, ArrayBufferView or UnsafeCallbackPrototype",
+      );
+    }
     if (pointer) {
       POINTER_TO_BUFFER_WEAK_MAP.set(pointer, value);
     }

ext/ffi/lib.rs

@@ -74,6 +74,7 @@ deno_core::extension!(deno_ffi,
     op_ffi_ptr_create<P>,
     op_ffi_ptr_equals<P>,
     op_ffi_ptr_of<P>,
+    op_ffi_ptr_of_exact<P>,
     op_ffi_ptr_offset<P>,
     op_ffi_ptr_value<P>,
     op_ffi_get_buf<P>,

ext/ffi/repr.rs

@@ -5,8 +5,7 @@ use crate::FfiPermissions;
 use deno_core::error::range_error;
 use deno_core::error::type_error;
 use deno_core::error::AnyError;
-use deno_core::op;
+use deno_core::op2;
-use deno_core::serde_v8;
 use deno_core::v8;
 use deno_core::OpState;
 use std::ffi::c_char;
@@ -14,10 +13,10 @@ use std::ffi::c_void;
 use std::ffi::CStr;
 use std::ptr;
 
-#[op(fast)]
+#[op2(fast)]
-fn op_ffi_ptr_create<FP>(
+pub fn op_ffi_ptr_create<FP>(
   state: &mut OpState,
-  ptr_number: usize,
+  #[bigint] ptr_number: usize,
 ) -> Result<*mut c_void, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -29,7 +28,7 @@ where
   Ok(ptr_number as *mut c_void)
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_ptr_equals<FP>(
   state: &mut OpState,
   a: *const c_void,
@@ -45,10 +44,10 @@ where
   Ok(a == b)
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_ptr_of<FP>(
   state: &mut OpState,
-  buf: *const u8,
+  #[buffer] buf: *const u8,
 ) -> Result<*mut c_void, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -60,11 +59,32 @@ where
   Ok(buf as *mut c_void)
 }
 
-#[op(fast)]
+#[op2(fast)]
-fn op_ffi_ptr_offset<FP>(
+pub fn op_ffi_ptr_of_exact<FP>(
+  state: &mut OpState,
+  buf: v8::Local<v8::ArrayBufferView>,
+) -> Result<*mut c_void, AnyError>
+where
+  FP: FfiPermissions + 'static,
+{
+  check_unstable(state, "Deno.UnsafePointer#of");
+  let permissions = state.borrow_mut::<FP>();
+  permissions.check_partial(None)?;
+
+  let Some(buf) = buf.get_backing_store() else {
+    return Ok(0 as _);
+  };
+  let Some(buf) = buf.data() else {
+    return Ok(0 as _);
+  };
+  Ok(buf.as_ptr() as _)
+}
+
+#[op2(fast)]
+pub fn op_ffi_ptr_offset<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<*mut c_void, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -77,8 +97,11 @@ where
     return Err(type_error("Invalid pointer to offset, pointer is null"));
   }
 
-  // SAFETY: Pointer and offset are user provided.
+  // TODO(mmastrac): Create a RawPointer that can safely do pointer math.
-  Ok(unsafe { ptr.offset(offset) })
+
+  // SAFETY: Using `ptr.offset` is *actually unsafe* and has generated UB, but our FFI code relies on this working so we're going to
+  // try and ask the compiler to be less undefined here by using `ptr.wrapping_offset`.
+  Ok(ptr.wrapping_offset(offset))
 }
 
 unsafe extern "C" fn noop_deleter_callback(
@@ -88,11 +111,11 @@ unsafe extern "C" fn noop_deleter_callback(
 ) {
 }
 
-#[op(fast)]
+#[op2(fast)]
-fn op_ffi_ptr_value<FP>(
+pub fn op_ffi_ptr_value<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  out: &mut [u32],
+  #[buffer] out: &mut [u32],
 ) -> Result<(), AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -115,14 +138,14 @@ where
   Ok(())
 }
 
-#[op(v8)]
+#[op2]
 pub fn op_ffi_get_buf<FP, 'scope>(
   scope: &mut v8::HandleScope<'scope>,
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
-  len: usize,
+  #[number] len: usize,
-) -> Result<serde_v8::Value<'scope>, AnyError>
+) -> Result<v8::Local<'scope, v8::ArrayBuffer>, AnyError>
 where
   FP: FfiPermissions + 'static,
 {
@@ -145,18 +168,17 @@ where
     )
   }
   .make_shared();
-  let array_buffer: v8::Local<v8::Value> =
+  let array_buffer = v8::ArrayBuffer::with_backing_store(scope, &backing_store);
-    v8::ArrayBuffer::with_backing_store(scope, &backing_store).into();
+  Ok(array_buffer)
-  Ok(array_buffer.into())
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_buf_copy_into<FP>(
   state: &mut OpState,
   src: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
-  dst: &mut [u8],
+  #[buffer] dst: &mut [u8],
-  len: usize,
+  #[number] len: usize,
 ) -> Result<(), AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -184,13 +206,13 @@ where
   }
 }
 
-#[op(v8)]
+#[op2]
 pub fn op_ffi_cstr_read<FP, 'scope>(
   scope: &mut v8::HandleScope<'scope>,
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
-) -> Result<serde_v8::Value<'scope>, AnyError>
+) -> Result<v8::Local<'scope, v8::String>, AnyError>
 where
   FP: FfiPermissions + 'static,
 {
@@ -206,20 +228,18 @@ where
   let cstr =
   // SAFETY: Pointer and offset are user provided.
     unsafe { CStr::from_ptr(ptr.offset(offset) as *const c_char) }.to_bytes();
-  let value: v8::Local<v8::Value> =
+  let value = v8::String::new_from_utf8(scope, cstr, v8::NewStringType::Normal)
-    v8::String::new_from_utf8(scope, cstr, v8::NewStringType::Normal)
     .ok_or_else(|| {
       type_error("Invalid CString pointer, string exceeds max length")
-      })?
+    })?;
-      .into();
+  Ok(value)
-  Ok(value.into())
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_bool<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<bool, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -237,11 +257,11 @@ where
   Ok(unsafe { ptr::read_unaligned::<bool>(ptr.offset(offset) as *const bool) })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_u8<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<u32, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -261,11 +281,11 @@ where
   })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_i8<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<i32, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -285,11 +305,11 @@ where
   })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_u16<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<u32, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -309,11 +329,11 @@ where
   })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_i16<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<i32, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -333,11 +353,11 @@ where
   })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_u32<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<u32, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -355,11 +375,11 @@ where
   Ok(unsafe { ptr::read_unaligned::<u32>(ptr.offset(offset) as *const u32) })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_i32<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<i32, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -377,12 +397,12 @@ where
   Ok(unsafe { ptr::read_unaligned::<i32>(ptr.offset(offset) as *const i32) })
 }
 
-#[op]
+#[op2(fast)]
 pub fn op_ffi_read_u64<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
-  out: &mut [u32],
+  #[buffer] out: &mut [u32],
 ) -> Result<(), AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -412,12 +432,12 @@ where
   Ok(())
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_i64<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
-  out: &mut [u32],
+  #[buffer] out: &mut [u32],
 ) -> Result<(), AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -446,11 +466,11 @@ where
   Ok(())
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_f32<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<f32, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -468,11 +488,11 @@ where
   Ok(unsafe { ptr::read_unaligned::<f32>(ptr.offset(offset) as *const f32) })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_f64<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<f64, AnyError>
 where
   FP: FfiPermissions + 'static,
@@ -490,11 +510,11 @@ where
   Ok(unsafe { ptr::read_unaligned::<f64>(ptr.offset(offset) as *const f64) })
 }
 
-#[op(fast)]
+#[op2(fast)]
 pub fn op_ffi_read_ptr<FP>(
   state: &mut OpState,
   ptr: *mut c_void,
-  offset: isize,
+  #[number] offset: isize,
 ) -> Result<*mut c_void, AnyError>
 where
   FP: FfiPermissions + 'static,

test_ffi/tests/integration_tests.rs

@@ -1,5 +1,6 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
 
+use pretty_assertions::assert_eq;
 use std::process::Command;
 use test_util::deno_cmd;
 
@@ -54,11 +55,11 @@ fn basic() {
     [ 4, 5, 6 ]\n\
     Hello from pointer!\n\
     pointer!\n\
-    false\n\
+    false false\n\
-    true\n\
+    true true\n\
-    false\n\
+    false false\n\
-    true\n\
+    true true\n\
-    false\n\
+    false false\n\
     579\n\
     true\n\
     579\n\

test_ffi/tests/test.js

@@ -341,13 +341,13 @@ const stringPtr = Deno.UnsafePointer.of(string);
 const stringPtrview = new Deno.UnsafePointerView(stringPtr);
 console.log(stringPtrview.getCString());
 console.log(stringPtrview.getCString(11));
-console.log(dylib.symbols.is_null_ptr(ptr0));
+console.log("false", dylib.symbols.is_null_ptr(ptr0));
-console.log(dylib.symbols.is_null_ptr(null));
+console.log("true", dylib.symbols.is_null_ptr(null));
-console.log(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(into)));
+console.log("false", 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)));
+console.log("true", dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptyBuffer)));
 const emptySlice = into.subarray(6);
-console.log(dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptySlice)));
+console.log("false", dylib.symbols.is_null_ptr(Deno.UnsafePointer.of(emptySlice)));
 
 const { is_null_buf } = symbols;
 function isNullBuffer(buffer) { return is_null_buf(buffer); };
@@ -386,10 +386,9 @@ const externalOneBuffer = new Uint8Array(Deno.UnsafePointerView.getArrayBuffer(p
 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
+// UnsafePointer.of uses an exact-pointer fallback for zero-length buffers and slices to ensure that it always gets
-// the pointer of an ArrayBuffer / TypedArray, the same effect can be seen where a zero byte length buffer returns
+// the underlying pointer right.
-// a null pointer as its pointer value.
+assertNotEquals(Deno.UnsafePointer.of(externalZeroBuffer), null, "Deno.UnsafePointer.of(externalZeroBuffer) === null");
-assertEquals(Deno.UnsafePointer.of(externalZeroBuffer), null, "Deno.UnsafePointer.of(externalZeroBuffer) !== null");
 assertNotEquals(Deno.UnsafePointer.of(externalOneBuffer), null, "Deno.UnsafePointer.of(externalOneBuffer) === null");
 
 const addU32Ptr = dylib.symbols.get_add_u32_ptr();
@@ -726,7 +725,9 @@ assertEquals(view.getUint32(), 55);
   assertThrows(() => Deno.UnsafePointer.offset(null, 5));
   const offsetPointer = Deno.UnsafePointer.offset(createdPointer, 5);
   assertEquals(Deno.UnsafePointer.value(offsetPointer), 6);
-  assertEquals(Deno.UnsafePointer.offset(offsetPointer, -6), null);
+  const zeroPointer = Deno.UnsafePointer.offset(offsetPointer, -6);
+  assertEquals(Deno.UnsafePointer.value(zeroPointer), 0);
+  assertEquals(zeroPointer, null);
 }
 
 // Test non-UTF-8 characters