From 55595ca1b74e07eb2365d5aec3861600e2266470 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Mon, 5 Dec 2022 08:10:22 -0800 Subject: [PATCH] fix(ops): disallow auto-borrowing OpState across potential await point (#16952) Fixes https://github.com/denoland/deno/issues/16934 Example compiler error: ``` error: mutable opstate is not supported in async ops --> core/ops_builtin.rs:122:1 | 122 | #[op] | ^^^^^ | = note: this error originates in the attribute macro `op` (in Nightly builds, run with -Z macro-backtrace for more info) ``` --- ext/ffi/lib.rs | 3 +- ext/flash/lib.rs | 6 +- ops/lib.rs | 13 ++- ops/optimizer.rs | 4 +- ops/optimizer_tests/issue16934.expected | 11 +++ ops/optimizer_tests/issue16934.out | 92 ++++++++++++++++++++ ops/optimizer_tests/issue16934.rs | 11 +++ ops/optimizer_tests/issue16934_fast.expected | 1 + ops/optimizer_tests/issue16934_fast.out | 90 +++++++++++++++++++ ops/optimizer_tests/issue16934_fast.rs | 8 ++ 10 files changed, 234 insertions(+), 5 deletions(-) create mode 100644 ops/optimizer_tests/issue16934.expected create mode 100644 ops/optimizer_tests/issue16934.out create mode 100644 ops/optimizer_tests/issue16934.rs create mode 100644 ops/optimizer_tests/issue16934_fast.expected create mode 100644 ops/optimizer_tests/issue16934_fast.out create mode 100644 ops/optimizer_tests/issue16934_fast.rs diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs index 52e101a92e..7e7756c931 100644 --- a/ext/ffi/lib.rs +++ b/ext/ffi/lib.rs @@ -1829,9 +1829,10 @@ impl Future for CallbackInfo { #[op] fn op_ffi_unsafe_callback_ref( - state: &mut deno_core::OpState, + state: Rc>, rid: ResourceId, ) -> Result>, AnyError> { + let state = state.borrow(); let callback_resource = state.resource_table.get::(rid)?; diff --git a/ext/flash/lib.rs b/ext/flash/lib.rs index d08cdbcdc5..04ed54e1ad 100644 --- a/ext/flash/lib.rs +++ b/ext/flash/lib.rs @@ -1290,10 +1290,11 @@ where #[op] fn op_flash_wait_for_listening( - state: &mut OpState, + state: Rc>, server_id: u32, ) -> Result> + 'static, AnyError> { let mut listening_rx = { + let mut state = state.borrow_mut(); let flash_ctx = state.borrow_mut::(); let server_ctx = flash_ctx .servers @@ -1312,10 +1313,11 @@ fn op_flash_wait_for_listening( #[op] fn op_flash_drive_server( - state: &mut OpState, + state: Rc>, server_id: u32, ) -> Result> + 'static, AnyError> { let join_handle = { + let mut state = state.borrow_mut(); let flash_ctx = state.borrow_mut::(); flash_ctx .join_handles diff --git a/ops/lib.rs b/ops/lib.rs index 26fcb2a78b..7fa800b14c 100644 --- a/ops/lib.rs +++ b/ops/lib.rs @@ -193,7 +193,8 @@ fn codegen_v8_async( .inputs .iter() .map_while(|a| { - (if is_v8 { scope_arg(a) } else { None }).or_else(|| opstate_arg(a)) + (if is_v8 { scope_arg(a) } else { None }) + .or_else(|| rc_refcell_opstate_arg(a)) }) .collect::>(); let rust_i0 = special_args.len(); @@ -291,6 +292,16 @@ fn opstate_arg(arg: &FnArg) -> Option { } } +fn rc_refcell_opstate_arg(arg: &FnArg) -> Option { + match arg { + arg if is_rc_refcell_opstate(arg) => Some(quote! { ctx.state.clone(), }), + arg if is_mut_ref_opstate(arg) => Some( + quote! { compile_error!("mutable opstate is not supported in async ops"), }, + ), + _ => None, + } +} + /// Generate the body of a v8 func for a sync op fn codegen_v8_sync( core: &TokenStream2, diff --git a/ops/optimizer.rs b/ops/optimizer.rs index 17435407cd..99de4b4240 100644 --- a/ops/optimizer.rs +++ b/ops/optimizer.rs @@ -650,7 +650,9 @@ impl Optimizer { let segment = single_segment(segments)?; match segment { // Is `T` a OpState? - PathSegment { ident, .. } if ident == "OpState" => { + PathSegment { ident, .. } + if ident == "OpState" && !self.is_async => + { self.has_ref_opstate = true; } // Is `T` a str? diff --git a/ops/optimizer_tests/issue16934.expected b/ops/optimizer_tests/issue16934.expected new file mode 100644 index 0000000000..6b75ff5bf3 --- /dev/null +++ b/ops/optimizer_tests/issue16934.expected @@ -0,0 +1,11 @@ +=== Optimizer Dump === +returns_result: false +has_ref_opstate: false +has_rc_opstate: false +has_fast_callback_option: false +needs_fast_callback_option: false +fast_result: None +fast_parameters: [] +transforms: {} +is_async: false +fast_compatible: false diff --git a/ops/optimizer_tests/issue16934.out b/ops/optimizer_tests/issue16934.out new file mode 100644 index 0000000000..63abd21920 --- /dev/null +++ b/ops/optimizer_tests/issue16934.out @@ -0,0 +1,92 @@ +#[allow(non_camel_case_types)] +///Auto-generated by `deno_ops`, i.e: `#[op]` +/// +///Use `send_stdin::decl()` to get an op-declaration +///you can include in a `deno_core::Extension`. +pub struct send_stdin; +#[doc(hidden)] +impl send_stdin { + pub fn name() -> &'static str { + stringify!(send_stdin) + } + pub fn v8_fn_ptr<'scope>() -> deno_core::v8::FunctionCallback { + use deno_core::v8::MapFnTo; + Self::v8_func.map_fn_to() + } + pub fn decl<'scope>() -> deno_core::OpDecl { + deno_core::OpDecl { + name: Self::name(), + v8_fn_ptr: Self::v8_fn_ptr(), + enabled: true, + fast_fn: None, + is_async: true, + is_unstable: false, + is_v8: false, + argc: 1usize, + } + } + #[inline] + #[allow(clippy::too_many_arguments)] + async fn call(state: &mut OpState, cmd: String) -> Result<(), anyhow::Error> { + let instance = state.borrow::().clone(); + instance.send_command(&cmd, CausedBy::Unknown).await?; + Ok(()) + } + pub fn v8_func<'scope>( + scope: &mut deno_core::v8::HandleScope<'scope>, + args: deno_core::v8::FunctionCallbackArguments, + mut rv: deno_core::v8::ReturnValue, + ) { + use deno_core::futures::FutureExt; + let ctx = unsafe { + &*(deno_core::v8::Local::::cast(args.data()).value() + as *const deno_core::_ops::OpCtx) + }; + let op_id = ctx.id; + let promise_id = args.get(0); + let promise_id = deno_core::v8::Local::< + deno_core::v8::Integer, + >::try_from(promise_id) + .map(|l| l.value() as deno_core::PromiseId) + .map_err(deno_core::anyhow::Error::from); + let promise_id: deno_core::PromiseId = match promise_id { + Ok(promise_id) => promise_id, + Err(err) => { + deno_core::_ops::throw_type_error( + scope, + format!("invalid promise id: {}", err), + ); + return; + } + }; + let arg_0 = match deno_core::v8::Local::< + deno_core::v8::String, + >::try_from(args.get(1usize as i32)) { + Ok(v8_string) => deno_core::serde_v8::to_utf8(v8_string, scope), + Err(_) => { + return deno_core::_ops::throw_type_error( + scope, + format!("Expected string at position {}", 1usize), + ); + } + }; + let get_class = { + let state = ::std::cell::RefCell::borrow(&ctx.state); + state.tracker.track_async(op_id); + state.get_error_class_fn + }; + deno_core::_ops::queue_async_op( + ctx, + scope, + false, + async move { + let result = Self::call( + compile_error!("mutable opstate is not supported in async ops"), + arg_0, + ) + .await; + (promise_id, op_id, deno_core::_ops::to_op_result(get_class, result)) + }, + ); + } +} diff --git a/ops/optimizer_tests/issue16934.rs b/ops/optimizer_tests/issue16934.rs new file mode 100644 index 0000000000..1e77f12812 --- /dev/null +++ b/ops/optimizer_tests/issue16934.rs @@ -0,0 +1,11 @@ +async fn send_stdin( + state: &mut OpState, + cmd: String, +) -> Result<(), anyhow::Error> { + // https://github.com/denoland/deno/issues/16934 + // + // OpState borrowed across await point is not allowed, as it will likely panic at runtime. + let instance = state.borrow::().clone(); + instance.send_command(&cmd, CausedBy::Unknown).await?; + Ok(()) +} diff --git a/ops/optimizer_tests/issue16934_fast.expected b/ops/optimizer_tests/issue16934_fast.expected new file mode 100644 index 0000000000..250ff1022d --- /dev/null +++ b/ops/optimizer_tests/issue16934_fast.expected @@ -0,0 +1 @@ +FastUnsupportedParamType \ No newline at end of file diff --git a/ops/optimizer_tests/issue16934_fast.out b/ops/optimizer_tests/issue16934_fast.out new file mode 100644 index 0000000000..615bc6b3b9 --- /dev/null +++ b/ops/optimizer_tests/issue16934_fast.out @@ -0,0 +1,90 @@ +#[allow(non_camel_case_types)] +///Auto-generated by `deno_ops`, i.e: `#[op]` +/// +///Use `send_stdin::decl()` to get an op-declaration +///you can include in a `deno_core::Extension`. +pub struct send_stdin; +#[doc(hidden)] +impl send_stdin { + pub fn name() -> &'static str { + stringify!(send_stdin) + } + pub fn v8_fn_ptr<'scope>() -> deno_core::v8::FunctionCallback { + use deno_core::v8::MapFnTo; + Self::v8_func.map_fn_to() + } + pub fn decl<'scope>() -> deno_core::OpDecl { + deno_core::OpDecl { + name: Self::name(), + v8_fn_ptr: Self::v8_fn_ptr(), + enabled: true, + fast_fn: None, + is_async: true, + is_unstable: false, + is_v8: false, + argc: 1usize, + } + } + #[inline] + #[allow(clippy::too_many_arguments)] + async fn call(state: &mut OpState, v: i32) -> Result<(), anyhow::Error> { + Ok(()) + } + pub fn v8_func<'scope>( + scope: &mut deno_core::v8::HandleScope<'scope>, + args: deno_core::v8::FunctionCallbackArguments, + mut rv: deno_core::v8::ReturnValue, + ) { + use deno_core::futures::FutureExt; + let ctx = unsafe { + &*(deno_core::v8::Local::::cast(args.data()).value() + as *const deno_core::_ops::OpCtx) + }; + let op_id = ctx.id; + let promise_id = args.get(0); + let promise_id = deno_core::v8::Local::< + deno_core::v8::Integer, + >::try_from(promise_id) + .map(|l| l.value() as deno_core::PromiseId) + .map_err(deno_core::anyhow::Error::from); + let promise_id: deno_core::PromiseId = match promise_id { + Ok(promise_id) => promise_id, + Err(err) => { + deno_core::_ops::throw_type_error( + scope, + format!("invalid promise id: {}", err), + ); + return; + } + }; + let arg_0 = args.get(1usize as i32); + let arg_0 = match deno_core::serde_v8::from_v8(scope, arg_0) { + Ok(v) => v, + Err(err) => { + let msg = format!( + "Error parsing args at position {}: {}", 1usize, + deno_core::anyhow::Error::from(err) + ); + return deno_core::_ops::throw_type_error(scope, msg); + } + }; + let get_class = { + let state = ::std::cell::RefCell::borrow(&ctx.state); + state.tracker.track_async(op_id); + state.get_error_class_fn + }; + deno_core::_ops::queue_async_op( + ctx, + scope, + false, + async move { + let result = Self::call( + compile_error!("mutable opstate is not supported in async ops"), + arg_0, + ) + .await; + (promise_id, op_id, deno_core::_ops::to_op_result(get_class, result)) + }, + ); + } +} diff --git a/ops/optimizer_tests/issue16934_fast.rs b/ops/optimizer_tests/issue16934_fast.rs new file mode 100644 index 0000000000..8d3488e9d0 --- /dev/null +++ b/ops/optimizer_tests/issue16934_fast.rs @@ -0,0 +1,8 @@ +async fn send_stdin(state: &mut OpState, v: i32) -> Result<(), anyhow::Error> { + // @test-attr:fast + // + // https://github.com/denoland/deno/issues/16934 + // + // OpState borrowed across await point is not allowed, as it will likely panic at runtime. + Ok(()) +}