Repository: denoland/deno_core
Pull request #407 (2024-01-03 00:28:27)

 fix(core): clamp V8Slice slice to bounds 
We should always clamp the expected buffer range to that of the underlying storage. 


Patch:
diff --git a/ops/op2/dispatch_fast.rs b/ops/op2/dispatch_fast.rs
index 4e531b7..e497cbc 100644
--- a/ops/op2/dispatch_fast.rs
+++ b/ops/op2/dispatch_fast.rs
@@ -775,7 +775,7 @@ fn map_arg_to_v8_fastcall_type(
     | Arg::SerdeV8(_)
     | Arg::Ref(..) => return Ok(None),
     // We don't support v8 global arguments
-    Arg::V8Global(_) => return Ok(None),
+    Arg::V8Global(_) | Arg::OptionV8Global(_) => return Ok(None),
     // We do support v8 type arguments (including Option<...>)
     Arg::V8Ref(RefType::Ref, _)
     | Arg::V8Local(_)
@@ -853,6 +853,7 @@ fn map_retval_to_v8_fastcall_type(
     | Arg::V8Global(_)
     | Arg::V8Local(_)
     | Arg::OptionV8Local(_)
+    | Arg::OptionV8Global(_)
     | Arg::OptionV8Ref(..) => return Ok(None),
     Arg::Buffer(..) | Arg::OptionBuffer(..) => return Ok(None),
     Arg::External(..) => V8FastCallType::Pointer,
diff --git a/serde_v8/magic/v8slice.rs b/serde_v8/magic/v8slice.rs
index e56b4a6..c2f0be2 100644
--- a/serde_v8/magic/v8slice.rs
+++ b/serde_v8/magic/v8slice.rs
@@ -146,6 +146,12 @@ where
     let Some(ptr) = store.data() else {
       return &[];
     };
+    let clamped_end =
+      std::cmp::min(self.range.end, store.len() / std::mem::size_of::<T>());
+    let clamped_len = clamped_end.saturating_sub(self.range.start);
+    if clamped_len == 0 {
+      return &mut [];
+    }
     let ptr = ptr.cast::<T>().as_ptr();
     // SAFETY: v8::SharedRef<v8::BackingStore> is similar to Arc<[u8]>,
     // it points to a fixed continuous slice of bytes on the heap.
@@ -156,7 +162,7 @@ where
     // do not have overlapping read/write phases.
     unsafe {
       let ptr = ptr.add(self.range.start);
-      std::slice::from_raw_parts(ptr, self.range.len())
+      std::slice::from_raw_parts(ptr, clamped_len)
     }
   }
 
@@ -165,6 +171,12 @@ where
     let Some(ptr) = store.data() else {
       return &mut [];
     };
+    let clamped_end =
+      std::cmp::min(self.range.end, store.len() / std::mem::size_of::<T>());
+    let clamped_len = clamped_end.saturating_sub(self.range.start);
+    if clamped_len == 0 {
+      return &mut [];
+    }
     let ptr = ptr.cast::<T>().as_ptr();
     // SAFETY: v8::SharedRef<v8::BackingStore> is similar to Arc<[u8]>,
     // it points to a fixed continuous slice of bytes on the heap.
@@ -175,16 +187,24 @@ where
     // do not have overlapping read/write phases.
     unsafe {
       let ptr = ptr.add(self.range.start);
-      std::slice::from_raw_parts_mut(ptr, self.range.len())
+      std::slice::from_raw_parts_mut(ptr, clamped_len)
     }
   }
 
+  /// Returns the underlying length of the range of this slice. If the range of this slice would exceed the range
+  /// of the underlying backing store, the range is clamped so that it falls within the underlying backing store's
+  /// valid length.
   pub fn len(&self) -> usize {
-    self.range.len()
+    let store = &self.store;
+    let clamped_end =
+      std::cmp::min(self.range.end, store.len() / std::mem::size_of::<T>());
+    clamped_end.saturating_sub(self.range.start)
   }
 
+  /// Returns whether this slice is empty. See `len` for notes about how the length is treated when the range of this
+  /// slice exceeds that of the underlying backing store.
   pub fn is_empty(&self) -> bool {
-    self.range.is_empty()
+    self.len() == 0
   }
 
   /// Create a [`Vec<T>`] copy of this slice data.


Test patch:
diff --git a/testing/checkin.d.ts b/testing/checkin.d.ts
index 8121c35..56f3e21 100644
--- a/testing/checkin.d.ts
+++ b/testing/checkin.d.ts
@@ -12,6 +12,14 @@ interface PromiseConstructor {
   };
 }
 
+interface ArrayBuffer {
+  transfer(size: number);
+}
+
+interface SharedArrayBuffer {
+  transfer(size: number);
+}
+
 declare namespace Deno {
   export function refTimer(id);
   export function unrefTimer(id);
diff --git a/testing/checkin/runner/mod.rs b/testing/checkin/runner/mod.rs
index 7f9c5a4..0fb83c8 100644
--- a/testing/checkin/runner/mod.rs
+++ b/testing/checkin/runner/mod.rs
@@ -14,9 +14,12 @@ use testing::Output;
 use tokio::fs::File;
 use tokio::io::AsyncReadExt;
 
+use crate::checkin::runner::testing::TestData;
+
 use self::testing::TestFunctions;
 
 mod ops;
+mod ops_buffer;
 mod testing;
 mod ts_module_loader;
 
@@ -26,6 +29,8 @@ deno_core::extension!(
     ops::op_log_debug,
     ops::op_log_info,
     ops::op_test_register,
+    ops_buffer::op_v8slice_store,
+    ops_buffer::op_v8slice_clone,
   ],
   esm_entry_point = "ext:checkin_runtime/__init.js",
   esm = [
@@ -37,6 +42,7 @@ deno_core::extension!(
   ],
   state = |state| {
     state.put(TestFunctions::default());
+    state.put(TestData::default());
     state.put(Output::default());
   }
 );
@@ -145,6 +151,12 @@ async fn run_unit_test_task(
     runtime
       .with_event_loop_promise(call, PollEventLoopOptions::default())
       .await?;
+
+    // Clear any remaining test data so we have a fresh state
+    let state = runtime.op_state();
+    let mut state = state.borrow_mut();
+    let data = state.borrow_mut::<TestData>();
+    data.data.clear();
   }
 
   Ok(())
diff --git a/testing/checkin/runner/ops_buffer.rs b/testing/checkin/runner/ops_buffer.rs
new file mode 100644
index 0000000..b70fe85
--- /dev/null
+++ b/testing/checkin/runner/ops_buffer.rs
@@ -0,0 +1,23 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+use deno_core::op2;
+use deno_core::JsBuffer;
+
+use super::testing::TestData;
+
+#[op2(fast)]
+pub fn op_v8slice_store(
+  #[state] test_data: &mut TestData,
+  #[string] name: String,
+  #[buffer] data: JsBuffer,
+) {
+  test_data.insert(name, data);
+}
+
+#[op2]
+#[buffer]
+pub fn op_v8slice_clone(
+  #[state] test_data: &mut TestData,
+  #[string] name: String,
+) -> Vec<u8> {
+  test_data.get::<JsBuffer>(name).to_vec()
+}
diff --git a/testing/checkin/runner/testing.rs b/testing/checkin/runner/testing.rs
index a9870c5..f3d6956 100644
--- a/testing/checkin/runner/testing.rs
+++ b/testing/checkin/runner/testing.rs
@@ -1,4 +1,8 @@
 // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+use std::any::Any;
+use std::any::TypeId;
+use std::collections::HashMap;
+
 use deno_core::v8;
 
 #[derive(Default)]
@@ -10,3 +14,23 @@ pub struct Output {
 pub struct TestFunctions {
   pub functions: Vec<(String, v8::Global<v8::Function>)>,
 }
+
+#[derive(Default)]
+pub struct TestData {
+  pub data: HashMap<(String, TypeId), Box<dyn Any>>,
+}
+
+impl TestData {
+  pub fn insert<T: 'static + Any>(&mut self, name: String, data: T) {
+    self.data.insert((name, TypeId::of::<T>()), Box::new(data));
+  }
+
+  pub fn get<T: 'static + Any>(&self, name: String) -> &T {
+    self
+      .data
+      .get(&(name, TypeId::of::<T>()))
+      .unwrap()
+      .downcast_ref()
+      .unwrap()
+  }
+}
diff --git a/testing/lib.rs b/testing/lib.rs
index c15b3f0..e472b65 100644
--- a/testing/lib.rs
+++ b/testing/lib.rs
@@ -35,6 +35,7 @@ macro_rules! integration_test {
 unit_test!(
   encode_decode_test,
   microtask_test,
+  ops_buffer_test,
   serialize_deserialize_test,
   tc39_test,
   timer_test,
diff --git a/testing/unit/ops_buffer_test.ts b/testing/unit/ops_buffer_test.ts
new file mode 100644
index 0000000..0555b1a
--- /dev/null
+++ b/testing/unit/ops_buffer_test.ts
@@ -0,0 +1,25 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+import { assertArrayEquals, test } from "checkin:testing";
+const {
+  op_v8slice_store,
+  op_v8slice_clone,
+} = Deno.core.ensureFastOps();
+
+// Cloning a buffer should result in the same buffer being returned
+test(function testBufferStore() {
+  const data = new Uint8Array(1024 * 1024);
+  op_v8slice_store("buffer", data);
+  const output = op_v8slice_clone("buffer");
+  assertArrayEquals(output, new Uint8Array(1024 * 1024));
+});
+
+// Ensure that the returned buffer size is correct when a buffer is resized
+// externally via `ArrayBuffer.transfer`.
+test(function testBufferTransfer() {
+  const data = new Uint8Array(1024 * 1024);
+  const buffer = data.buffer;
+  op_v8slice_store("buffer", data);
+  buffer.transfer(100);
+  const output = op_v8slice_clone("buffer");
+  assertArrayEquals(output, new Uint8Array(100));
+});