From 1b610bfdad1d83846c62ef797c1e1ab1f7af44ba Mon Sep 17 00:00:00 2001 From: Aapo Alasuutari Date: Sun, 7 May 2023 13:31:01 +0300 Subject: [PATCH] fix(ext/ffi): UnsafeCallback can hang with 'deno test' (#19018) --- ext/ffi/00_ffi.js | 2 +- ext/ffi/callback.rs | 82 +++++++++++++++++++++++++++++---------------- ext/ffi/lib.rs | 8 ++--- 3 files changed, 57 insertions(+), 35 deletions(-) diff --git a/ext/ffi/00_ffi.js b/ext/ffi/00_ffi.js index 2091a55b38..67cb13ab6d 100644 --- a/ext/ffi/00_ffi.js +++ b/ext/ffi/00_ffi.js @@ -426,7 +426,7 @@ class UnsafeCallback { close() { this.#refcount = 0; - core.close(this.#rid); + ops.op_ffi_unsafe_callback_close(this.#rid); } } diff --git a/ext/ffi/callback.rs b/ext/ffi/callback.rs index bd4d6a5454..ef613b3ede 100644 --- a/ext/ffi/callback.rs +++ b/ext/ffi/callback.rs @@ -6,7 +6,6 @@ use crate::FfiPermissions; use crate::FfiState; use crate::ForeignFunction; use crate::PendingFfiAsyncWork; -use crate::LOCAL_ISOLATE_POINTER; use crate::MAX_SAFE_INTEGER; use crate::MIN_SAFE_INTEGER; use deno_core::error::AnyError; @@ -30,9 +29,18 @@ use std::pin::Pin; use std::ptr; use std::ptr::NonNull; use std::rc::Rc; +use std::sync::atomic; +use std::sync::atomic::AtomicU32; use std::sync::mpsc::sync_channel; use std::task::Poll; use std::task::Waker; + +static THREAD_ID_COUNTER: AtomicU32 = AtomicU32::new(1); + +thread_local! { + static LOCAL_THREAD_ID: RefCell = RefCell::new(0); +} + #[derive(Clone)] pub struct PtrSymbol { pub cif: libffi::middle::Cif, @@ -81,26 +89,16 @@ impl Resource for UnsafeCallbackResource { fn close(self: Rc) { self.cancel.cancel(); - // SAFETY: This drops the closure and the callback info associated with it. - // Any retained function pointers to the closure become dangling pointers. - // It is up to the user to know that it is safe to call the `close()` on the - // UnsafeCallback instance. - unsafe { - let info = Box::from_raw(self.info); - let isolate = info.isolate.as_mut().unwrap(); - let _ = v8::Global::from_raw(isolate, info.callback); - let _ = v8::Global::from_raw(isolate, info.context); - } } } struct CallbackInfo { - pub parameters: Vec, - pub result: NativeType, pub async_work_sender: mpsc::UnboundedSender, pub callback: NonNull, pub context: NonNull, - pub isolate: *mut v8::Isolate, + pub parameters: Vec, + pub result: NativeType, + pub thread_id: u32, pub waker: Option, } @@ -122,8 +120,8 @@ unsafe extern "C" fn deno_ffi_callback( args: *const *const c_void, info: &CallbackInfo, ) { - LOCAL_ISOLATE_POINTER.with(|s| { - if ptr::eq(*s.borrow(), info.isolate) { + LOCAL_THREAD_ID.with(|s| { + if *s.borrow() == info.thread_id { // Own isolate thread, okay to call directly do_ffi_callback(cif, info, result, args); } else { @@ -155,9 +153,6 @@ unsafe fn do_ffi_callback( ) { let callback: NonNull = info.callback; let context: NonNull = info.context; - let isolate: *mut v8::Isolate = info.isolate; - let isolate = &mut *isolate; - let callback = v8::Global::from_raw(isolate, callback); let context = std::mem::transmute::< NonNull, v8::Local, @@ -174,7 +169,10 @@ unsafe fn do_ffi_callback( // refer the same `let bool_value`. let mut cb_scope = v8::CallbackScope::new(context); let scope = &mut v8::HandleScope::new(&mut cb_scope); - let func = callback.open(scope); + let func = std::mem::transmute::< + NonNull, + v8::Local, + >(callback); let result = result as *mut c_void; let vals: &[*const c_void] = std::slice::from_raw_parts(args, info.parameters.len()); @@ -267,7 +265,6 @@ unsafe fn do_ffi_callback( let recv = v8::undefined(scope); let call_result = func.call(scope, recv.into(), ¶ms); - std::mem::forget(callback); if call_result.is_none() { // JS function threw an exception. Set the return value to zero and return. @@ -555,13 +552,21 @@ where let v8_value = cb.v8_value; let cb = v8::Local::::try_from(v8_value)?; - let isolate: *mut v8::Isolate = &mut *scope as &mut v8::Isolate; - LOCAL_ISOLATE_POINTER.with(|s| { - if s.borrow().is_null() { - s.replace(isolate); + let thread_id: u32 = LOCAL_THREAD_ID.with(|s| { + let value = *s.borrow(); + if value == 0 { + let res = THREAD_ID_COUNTER.fetch_add(1, atomic::Ordering::SeqCst); + s.replace(res); + res + } else { + value } }); + if thread_id == 0 { + panic!("Isolate ID counter overflowed u32"); + } + let async_work_sender = state.borrow_mut::().async_work_sender.clone(); let callback = v8::Global::new(scope, cb).into_raw(); @@ -569,12 +574,12 @@ where let context = v8::Global::new(scope, current_context).into_raw(); let info: *mut CallbackInfo = Box::leak(Box::new(CallbackInfo { - parameters: args.parameters.clone(), - result: args.result.clone(), async_work_sender, callback, context, - isolate, + parameters: args.parameters.clone(), + result: args.result.clone(), + thread_id, waker: None, })); let cif = Cif::new( @@ -607,3 +612,24 @@ where Ok(array_value.into()) } + +#[op(v8)] +pub fn op_ffi_unsafe_callback_close( + state: &mut OpState, + scope: &mut v8::HandleScope, + rid: ResourceId, +) -> Result<(), AnyError> { + // SAFETY: This drops the closure and the callback info associated with it. + // Any retained function pointers to the closure become dangling pointers. + // It is up to the user to know that it is safe to call the `close()` on the + // UnsafeCallback instance. + unsafe { + let callback_resource = + state.resource_table.take::(rid)?; + let info = Box::from_raw(callback_resource.info); + let _ = v8::Global::from_raw(scope, info.callback); + let _ = v8::Global::from_raw(scope, info.context); + callback_resource.close(); + } + Ok(()) +} diff --git a/ext/ffi/lib.rs b/ext/ffi/lib.rs index c11f08dd8e..ccad69d738 100644 --- a/ext/ffi/lib.rs +++ b/ext/ffi/lib.rs @@ -2,7 +2,6 @@ use deno_core::error::AnyError; use deno_core::futures::channel::mpsc; -use deno_core::v8; use deno_core::OpState; use std::cell::RefCell; @@ -10,7 +9,6 @@ use std::mem::size_of; use std::os::raw::c_char; use std::os::raw::c_short; use std::path::Path; -use std::ptr; use std::rc::Rc; mod call; @@ -25,6 +23,7 @@ mod turbocall; use call::op_ffi_call_nonblocking; use call::op_ffi_call_ptr; use call::op_ffi_call_ptr_nonblocking; +use callback::op_ffi_unsafe_callback_close; use callback::op_ffi_unsafe_callback_create; use callback::op_ffi_unsafe_callback_ref; use dlfcn::op_ffi_load; @@ -43,10 +42,6 @@ const _: () = { assert!(size_of::<*const ()>() == 8); }; -thread_local! { - static LOCAL_ISOLATE_POINTER: RefCell<*const v8::Isolate> = RefCell::new(ptr::null()); -} - pub(crate) const MAX_SAFE_INTEGER: isize = 9007199254740991; pub(crate) const MIN_SAFE_INTEGER: isize = -9007199254740991; @@ -109,6 +104,7 @@ deno_core::extension!(deno_ffi, op_ffi_read_f64

, op_ffi_read_ptr

, op_ffi_unsafe_callback_create

, + op_ffi_unsafe_callback_close, op_ffi_unsafe_callback_ref, ], esm = [ "00_ffi.js" ],