fix(ops): disallow memory slices as inputs to async ops (denoland#16738)

In Rust, it is UB if a slice is mutated while borrowed except through the slice itself, and it is also UB if a mutable slice is read while borrowed. The op macro allows borrowing an `ArrayBuffer{,View}` as a memory slice for the duration of an op, but this is not sound for async ops, since the `ArrayBuffer` could be accessed from JS during the await points. This PR therefore disallows such automatic borrowing only for async ops. Co-authored-by: Divy Srivastava <[email protected]>
DavidDeSimone · Jan 15, 2023 · 90c0381 · 90c0381
1 parent 44d9acc
commit 90c0381
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 79 deletions.
diff --git a/ops/lib.rs b/ops/lib.rs
@@ -209,7 +209,7 @@ fn codegen_v8_async(
   let rust_i0 = special_args.len();
   let args_head = special_args.into_iter().collect::<TokenStream2>();
 
-  let (arg_decls, args_tail, argc) = codegen_args(core, f, rust_i0, 1);
+  let (arg_decls, args_tail, argc) = codegen_args(core, f, rust_i0, 1, true);
   let type_params = exclude_lifetime_params(&f.sig.generics.params);
 
   let (pre_result, mut result_fut) = match asyncness {
@@ -330,7 +330,7 @@ fn codegen_v8_sync(
     .collect::<Vec<_>>();
   let rust_i0 = special_args.len();
   let args_head = special_args.into_iter().collect::<TokenStream2>();
-  let (arg_decls, args_tail, argc) = codegen_args(core, f, rust_i0, 0);
+  let (arg_decls, args_tail, argc) = codegen_args(core, f, rust_i0, 0, false);
   let ret = codegen_sync_ret(core, &f.sig.output);
   let type_params = exclude_lifetime_params(&f.sig.generics.params);
 
@@ -380,6 +380,7 @@ fn codegen_args(
   f: &syn::ItemFn,
   rust_i0: usize, // Index of first generic arg in rust
   v8_i0: usize,   // Index of first generic arg in v8/js
+  asyncness: bool,
 ) -> ArgumentDecl {
   let inputs = &f.sig.inputs.iter().skip(rust_i0).enumerate();
   let ident_seq: TokenStream2 = inputs
@@ -392,7 +393,7 @@ fn codegen_args(
   let decls: TokenStream2 = inputs
     .clone()
     .map(|(i, arg)| {
-      codegen_arg(core, arg, format!("arg_{i}").as_ref(), v8_i0 + i)
+      codegen_arg(core, arg, format!("arg_{i}").as_ref(), v8_i0 + i, asyncness)
     })
     .collect();
   (decls, ident_seq, inputs.len())
@@ -403,6 +404,7 @@ fn codegen_arg(
   arg: &syn::FnArg,
   name: &str,
   idx: usize,
+  asyncness: bool,
 ) -> TokenStream2 {
   let ident = quote::format_ident!("{name}");
   let (pat, ty) = match arg {
@@ -444,12 +446,14 @@ fn codegen_arg(
   match is_ref_slice(&**ty) {
     None => {}
     Some(SliceType::U32Mut) => {
+      assert!(!asyncness, "Memory slices are not allowed in async ops");
       let blck = codegen_u32_mut_slice(core, idx);
       return quote! {
         let #ident = #blck;
       };
     }
     Some(_) => {
+      assert!(!asyncness, "Memory slices are not allowed in async ops");
       let blck = codegen_u8_slice(core, idx);
       return quote! {
         let #ident = #blck;

diff --git a/ops/optimizer_tests/async_result.expected b/ops/optimizer_tests/async_result.expected
@@ -3,9 +3,9 @@ returns_result: true
 has_ref_opstate: false
 has_rc_opstate: true
 has_fast_callback_option: false
-needs_fast_callback_option: true
+needs_fast_callback_option: false
 fast_result: None
-fast_parameters: [V8Value, I32, U32, Uint8Array]
-transforms: {2: Transform { kind: SliceU8(true), index: 2 }}
+fast_parameters: [V8Value, I32, U32]
+transforms: {}
 is_async: true
 fast_compatible: true
diff --git a/ops/optimizer_tests/async_result.out b/ops/optimizer_tests/async_result.out
@@ -1,13 +1,13 @@
 #[allow(non_camel_case_types)]
 ///Auto-generated by `deno_ops`, i.e: `#[op]`
 ///
-///Use `op_read::decl()` to get an op-declaration
+///Use `op_async_result::decl()` to get an op-declaration
 ///you can include in a `deno_core::Extension`.
-pub struct op_read;
+pub struct op_async_result;
 #[doc(hidden)]
-impl op_read {
+impl op_async_result {
     pub fn name() -> &'static str {
-        stringify!(op_read)
+        stringify!(op_async_result)
     }
     pub fn v8_fn_ptr<'scope>() -> deno_core::v8::FunctionCallback {
         use deno_core::v8::MapFnTo;
@@ -19,23 +19,19 @@ impl op_read {
             v8_fn_ptr: Self::v8_fn_ptr(),
             enabled: true,
             fast_fn: Some(
-                Box::new(op_read_fast {
+                Box::new(op_async_result_fast {
                     _phantom: ::std::marker::PhantomData,
                 }),
             ),
             is_async: true,
             is_unstable: false,
             is_v8: false,
-            argc: 2usize,
+            argc: 1usize,
         }
     }
     #[inline]
     #[allow(clippy::too_many_arguments)]
-    async fn call(
-        state: Rc<RefCell<OpState>>,
-        rid: ResourceId,
-        buf: &mut [u8],
-    ) -> Result<u32, Error> {}
+    async fn call(state: Rc<RefCell<OpState>>, rid: ResourceId) -> Result<u32, Error> {}
     pub fn v8_func<'scope>(
         scope: &mut deno_core::v8::HandleScope<'scope>,
         args: deno_core::v8::FunctionCallbackArguments,
@@ -75,51 +71,6 @@ impl op_read {
                 return deno_core::_ops::throw_type_error(scope, msg);
             }
         };
-        let arg_1 = {
-            let value = args.get(2usize as i32);
-            match deno_core::v8::Local::<deno_core::v8::ArrayBuffer>::try_from(value) {
-                Ok(b) => {
-                    let byte_length = b.byte_length();
-                    if let Some(data) = b.data() {
-                        let store = data.cast::<u8>().as_ptr();
-                        unsafe { ::std::slice::from_raw_parts_mut(store, byte_length) }
-                    } else {
-                        &mut []
-                    }
-                }
-                Err(_) => {
-                    if let Ok(view)
-                        = deno_core::v8::Local::<
-                            deno_core::v8::ArrayBufferView,
-                        >::try_from(value) {
-                        let len = view.byte_length();
-                        let offset = view.byte_offset();
-                        let buffer = match view.buffer(scope) {
-                            Some(v) => v,
-                            None => {
-                                return deno_core::_ops::throw_type_error(
-                                    scope,
-                                    format!("Expected ArrayBufferView at position {}", 2usize),
-                                );
-                            }
-                        };
-                        if let Some(data) = buffer.data() {
-                            let store = data.cast::<u8>().as_ptr();
-                            unsafe {
-                                ::std::slice::from_raw_parts_mut(store.add(offset), len)
-                            }
-                        } else {
-                            &mut []
-                        }
-                    } else {
-                        return deno_core::_ops::throw_type_error(
-                            scope,
-                            format!("Expected ArrayBufferView at position {}", 2usize),
-                        );
-                    }
-                }
-            }
-        };
         let get_class = {
             let state = ::std::cell::RefCell::borrow(&ctx.state);
             state.tracker.track_async(op_id);
@@ -130,7 +81,7 @@ impl op_read {
             scope,
             false,
             async move {
-                let result = Self::call(ctx.state.clone(), arg_0, arg_1).await;
+                let result = Self::call(ctx.state.clone(), arg_0).await;
                 (
                     realm_idx,
                     promise_id,
@@ -141,27 +92,26 @@ impl op_read {
         );
     }
 }
-struct op_read_fast {
+struct op_async_result_fast {
     _phantom: ::std::marker::PhantomData<()>,
 }
-impl<'scope> deno_core::v8::fast_api::FastFunction for op_read_fast {
+impl<'scope> deno_core::v8::fast_api::FastFunction for op_async_result_fast {
     fn function(&self) -> *const ::std::ffi::c_void {
-        op_read_fast_fn as *const ::std::ffi::c_void
+        op_async_result_fast_fn as *const ::std::ffi::c_void
     }
     fn args(&self) -> &'static [deno_core::v8::fast_api::Type] {
         use deno_core::v8::fast_api::Type::*;
         use deno_core::v8::fast_api::CType;
-        &[V8Value, Int32, Uint32, TypedArray(CType::Uint8), CallbackOptions]
+        &[V8Value, Int32, Uint32, CallbackOptions]
     }
     fn return_type(&self) -> deno_core::v8::fast_api::CType {
         deno_core::v8::fast_api::CType::Void
     }
 }
-fn op_read_fast_fn<'scope>(
+fn op_async_result_fast_fn<'scope>(
     _: deno_core::v8::Local<deno_core::v8::Object>,
     __promise_id: i32,
     rid: ResourceId,
-    buf: *const deno_core::v8::fast_api::FastApiTypedArray<u8>,
     fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions,
 ) -> () {
     use deno_core::v8;
@@ -174,14 +124,7 @@ fn op_read_fast_fn<'scope>(
             as *const _ops::OpCtx)
     };
     let state = __ctx.state.clone();
-    let buf = match unsafe { &*buf }.get_storage_if_aligned() {
-        Some(v) => v,
-        None => {
-            unsafe { &mut *fast_api_callback_options }.fallback = true;
-            return Default::default();
-        }
-    };
-    let result = op_read::call(state, rid, buf);
+    let result = op_async_result::call(state, rid);
     let __op_id = __ctx.id;
     let __state = ::std::cell::RefCell::borrow(&__ctx.state);
     __state.tracker.track_async(__op_id);

diff --git a/ops/optimizer_tests/async_result.rs b/ops/optimizer_tests/async_result.rs
@@ -1,7 +1,6 @@
-async fn op_read(
+async fn op_async_result(
   state: Rc<RefCell<OpState>>,
   rid: ResourceId,
-  buf: &mut [u8],
 ) -> Result<u32, Error> {
   // @test-attr:fast
 }
diff --git a/ops/tests/compile_fail/mem_slices.rs b/ops/tests/compile_fail/mem_slices.rs
@@ -0,0 +1,24 @@
+// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
+
+use deno_ops::op;
+
+#[op]
+fn sync_test(slice: &mut [u32]) {
+  //
+}
+
+#[op]
+async fn async_test(slice: &[u8]) {
+  // Memory slices are not allowed in async ops.
+}
+
+#[op]
+fn async_test2(slice: &mut [u8]) -> impl Future<Output = ()> {
+  // Memory slices are not allowed in async ops, even when not implemented as an
+  // async function.
+  async {}
+}
+
+fn main() {
+  // pass
+}
diff --git a/ops/tests/compile_fail/mem_slices.stderr b/ops/tests/compile_fail/mem_slices.stderr
@@ -0,0 +1,15 @@
+error: custom attribute panicked
+  --> tests/compile_fail/mem_slices.rs:10:1
+   |
+10 | #[op]
+   | ^^^^^
+   |
+   = help: message: Memory slices are not allowed in async ops
+
+error: custom attribute panicked
+  --> tests/compile_fail/mem_slices.rs:15:1
+   |
+15 | #[op]
+   | ^^^^^
+   |
+   = help: message: Memory slices are not allowed in async ops