fix(core): Drop fast function info after isolate (#837)

Fixes crashes caused by #832
by making it the responsibility of `InnerIsolateState` to free the
cfunctioninfo/ctypeinfo _after_ dropping the isolate, instead of the
`OpCtx` freeing those on drop.

The cause of the crash is that the `OpCtx` gets dropped when the realm
is destroyed (and we free the fast api info), but the isolate still
lives because we don't destroy the isolate until [after destroying the
realm](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsrealm.rs#L183-L184).
In that time before we destroy the isolate, a V8 worker thread still has
a reference to the dropped CFunctionInfo, and boom UAF.
Here's a proper stacktrace for the check failure
```
#
# Fatal error in , line 0
# Check failed: c_argument_count >= kReceiver.
#
#
#
#FailureMessage Object: 0x17031cf28
==== C stack trace ===============================

    0   url_ops-d558488f29975416            0x0000000100a1aff4 v8::base::debug::StackTrace::StackTrace() + 24
    1   url_ops-d558488f29975416            0x0000000100a1f854 v8::platform::(anonymous namespace)::PrintStackTrace() + 24
    2   url_ops-d558488f29975416            0x0000000100a17cf8 V8_Fatal(char const*, ...) + 356
    3   url_ops-d558488f29975416            0x00000001018854c4 v8::internal::compiler::FastApiCallReducerAssembler::ReduceFastApiCall() + 1608
    4   url_ops-d558488f29975416            0x00000001018841a0 v8::internal::compiler::JSCallReducer::ReduceCallApiFunction(v8::internal::compiler::Node*, v8::internal::compiler::SharedFunctionInfoRef) + 856
    5   url_ops-d558488f29975416            0x0000000101886bac v8::internal::compiler::JSCallReducer::ReduceJSCall(v8::internal::compiler::Node*, v8::internal::compiler::SharedFunctionInfoRef) + 356
    6   url_ops-d558488f29975416            0x000000010187d564 v8::internal::compiler::JSCallReducer::ReduceJSCall(v8::internal::compiler::Node*) + 1120
    7   url_ops-d558488f29975416            0x000000010187d720 v8::internal::compiler::JSCallReducer::ReduceJSCall(v8::internal::compiler::Node*) + 1564
    8   url_ops-d558488f29975416            0x0000000101796c8c v8::internal::compiler::GraphReducer::Reduce(v8::internal::compiler::Node*) + 180
    9   url_ops-d558488f29975416            0x00000001017968e4 v8::internal::compiler::GraphReducer::ReduceTop() + 584
    10  url_ops-d558488f29975416            0x00000001017964d4 v8::internal::compiler::GraphReducer::ReduceNode(v8::internal::compiler::Node*) + 172
    11  url_ops-d558488f29975416            0x00000001018d96a8 v8::internal::compiler::InliningPhase::Run(v8::internal::compiler::TFPipelineData*, v8::internal::Zone*) + 704
    12  url_ops-d558488f29975416            0x00000001018cd664 auto v8::internal::compiler::PipelineImpl::Run<v8::internal::compiler::InliningPhase>() + 132
    13  url_ops-d558488f29975416            0x00000001018ca290 v8::internal::compiler::PipelineImpl::CreateGraph() + 232
    14  url_ops-d558488f29975416            0x00000001018c9edc v8::internal::compiler::PipelineCompilationJob::ExecuteJobImpl(v8::internal::RuntimeCallStats*, v8::internal::LocalIsolate*) + 248
    15  url_ops-d558488f29975416            0x0000000100aea274 v8::internal::OptimizedCompilationJob::ExecuteJob(v8::internal::RuntimeCallStats*, v8::internal::LocalIsolate*) + 80
    16  url_ops-d558488f29975416            0x0000000100b1d9e8 v8::internal::OptimizingCompileDispatcher::CompileNext(v8::internal::TurbofanCompilationJob*, v8::internal::LocalIsolate*) + 44
    17  url_ops-d558488f29975416            0x0000000100b1f298 v8::internal::OptimizingCompileDispatcher::CompileTask::Run(v8::JobDelegate*) + 396
    18  url_ops-d558488f29975416            0x0000000100a1c75c v8::platform::DefaultJobWorker::Run() + 216
    19  url_ops-d558488f29975416            0x0000000100a20d3c v8::platform::DefaultWorkerThreadsTaskRunner::WorkerThread::Run() + 160
    20  url_ops-d558488f29975416            0x0000000100a1a968 v8::base::ThreadEntry(void*) + 160
    21  libsystem_pthread.dylib             0x00000001810d6f94 _pthread_start + 136
```
The opctx gets dropped during
[JsRuntime::cleanup](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsruntime.rs#L175-L176),
called in
[InnerIsolateState::drop](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsruntime.rs#L207),
then we [drop the
isolate](https://github.com/denoland/deno_core/blob/19d2d603e506b4d6b6aa1f88c092dbd59f4000b6/core/runtime/jsruntime.rs#L218)
to destroy it

core/ops.rs

@@ -74,6 +74,12 @@ impl OpMetadata {
   }
 }
 
+#[derive(Clone, Copy)]
+pub(crate) struct FastFunctionInfo {
+  pub(crate) fn_info: NonNull<CFunctionInfo>,
+  pub(crate) fn_sig: (NonNull<CTypeInfo>, NonNull<CTypeInfo>),
+}
+
 /// Per-op context.
 ///
 // Note: We don't worry too much about the size of this struct because it's allocated once per realm, and is
@@ -92,8 +98,7 @@ pub struct OpCtx {
   pub get_error_class_fn: GetErrorClassFn,
 
   pub(crate) decl: OpDecl,
-  pub(crate) fast_fn_c_info: Option<NonNull<CFunctionInfo>>,
-  fast_fn_signature: Option<(NonNull<CTypeInfo>, NonNull<CTypeInfo>)>,
+  pub(crate) fast_fn_info: Option<FastFunctionInfo>,
   pub(crate) metrics_fn: Option<OpMetricsFn>,
   /// If the last fast op failed, stores the error to be picked up by the slow op.
   pub(crate) last_fast_error: UnsafeCell<Option<AnyError>>,
@@ -102,21 +107,6 @@ pub struct OpCtx {
   runtime_state: *const JsRuntimeState,
 }
 
-impl Drop for OpCtx {
-  fn drop(&mut self) {
-    if let Some(ptr) = self.fast_fn_c_info {
-      // SAFETY: Call drop manually because `fast_fn_c_info` is a `NonNull` and doesn't implement `Drop`.
-      unsafe {
-        std::ptr::drop_in_place(ptr.as_ptr());
-        if let Some((args, ret)) = self.fast_fn_signature {
-          std::ptr::drop_in_place(args.as_ptr());
-          std::ptr::drop_in_place(ret.as_ptr());
-        }
-      }
-    }
-  }
-}
-
 impl OpCtx {
   #[allow(clippy::too_many_arguments)]
   pub(crate) fn new(
@@ -129,9 +119,6 @@ impl OpCtx {
     get_error_class_fn: GetErrorClassFn,
     metrics_fn: Option<OpMetricsFn>,
   ) -> Self {
-    let mut fast_fn_c_info = None;
-    let mut fast_fn_signature = None;
-
     // If we want metrics for this function, create the fastcall `CFunctionInfo` from the metrics
     // `FastFunction`. For some extremely fast ops, the parameter list may change for the metrics
     // version and require a slightly different set of arguments (for example, it may need the fastcall
@@ -142,7 +129,7 @@ impl OpCtx {
       &decl.fast_fn
     };
 
-    if let Some(fast_fn) = fast_fn {
+    let fast_fn_info = fast_fn.map(|fast_fn| {
       let args = CTypeInfo::new_from_slice(fast_fn.args);
       let ret = CTypeInfo::new(fast_fn.return_type);
 
@@ -156,9 +143,11 @@ impl OpCtx {
           fast_fn.repr,
         )
       };
-      fast_fn_c_info = Some(c_fn);
-      fast_fn_signature = Some((args, ret));
-    }
+      FastFunctionInfo {
+        fn_info: c_fn,
+        fn_sig: (args, ret),
+      }
+    });
 
     Self {
       id,
@@ -167,8 +156,7 @@ impl OpCtx {
       runtime_state,
       decl,
       op_driver,
-      fast_fn_c_info,
-      fast_fn_signature,
+      fast_fn_info,
       last_fast_error: UnsafeCell::new(None),
       isolate,
       metrics_fn,
@@ -201,14 +189,14 @@ impl OpCtx {
       let slow_fn = v8::ExternalReference {
         function: self.decl.slow_fn_with_metrics,
       };
-      if let (Some(fast_fn), Some(fast_fn_c_info)) =
-        (&self.decl.fast_fn_with_metrics, &self.fast_fn_c_info)
+      if let (Some(fast_fn), Some(fast_fn_info)) =
+        (&self.decl.fast_fn_with_metrics, &self.fast_fn_info)
       {
         let fast_fn = v8::ExternalReference {
           pointer: fast_fn.function as _,
         };
         let fast_info = v8::ExternalReference {
-          pointer: fast_fn_c_info.as_ptr() as _,
+          pointer: fast_fn_info.fn_info.as_ptr() as _,
         };
         [ctx_ptr, slow_fn, fast_fn, fast_info]
       } else {
@@ -218,14 +206,14 @@ impl OpCtx {
       let slow_fn = v8::ExternalReference {
         function: self.decl.slow_fn,
       };
-      if let (Some(fast_fn), Some(fast_fn_c_info)) =
-        (&self.decl.fast_fn, &self.fast_fn_c_info)
+      if let (Some(fast_fn), Some(fast_fn_info)) =
+        (&self.decl.fast_fn, &self.fast_fn_info)
       {
         let fast_fn = v8::ExternalReference {
           pointer: fast_fn.function as _,
         };
         let fast_info = v8::ExternalReference {
-          pointer: fast_fn_c_info.as_ptr() as _,
+          pointer: fast_fn_info.fn_info.as_ptr() as _,
         };
         [ctx_ptr, slow_fn, fast_fn, fast_info]
       } else {

core/runtime/bindings.rs

@@ -354,7 +354,7 @@ fn op_ctx_function<'s>(
     builder.build_fast(
       scope,
       fast_function,
-      Some(op_ctx.fast_fn_c_info.unwrap().as_ptr()),
+      Some(op_ctx.fast_fn_info.unwrap().fn_info.as_ptr()),
       None,
       None,
     )

core/runtime/jsruntime.rs

@@ -38,6 +38,7 @@ use crate::modules::ModuleMap;
 use crate::modules::ModuleName;
 use crate::modules::RequestedModuleType;
 use crate::modules::ValidateImportAttributesCb;
+use crate::ops::FastFunctionInfo;
 use crate::ops_metrics::dispatch_metrics_async;
 use crate::ops_metrics::OpMetricsFactoryFn;
 use crate::runtime::ContextState;
@@ -151,6 +152,7 @@ pub(crate) struct InnerIsolateState {
   main_realm: ManuallyDrop<JsRealm>,
   pub(crate) state: ManuallyDropRc<JsRuntimeState>,
   v8_isolate: ManuallyDrop<v8::OwnedIsolate>,
+  fast_fn_infos: Vec<FastFunctionInfo>,
 }
 
 impl InnerIsolateState {
@@ -218,6 +220,20 @@ impl Drop for InnerIsolateState {
         ManuallyDrop::drop(&mut self.v8_isolate);
       }
     }
+    // Free the fast function infos manually.
+    for FastFunctionInfo {
+      fn_info,
+      fn_sig: (args, ret),
+    } in std::mem::take(&mut self.fast_fn_infos)
+    {
+      // SAFETY: We logically own these, and there are no remaining references because we just destroyed the
+      // realm and isolate above.
+      unsafe {
+        std::ptr::drop_in_place(fn_info.as_ptr());
+        std::ptr::drop_in_place(args.as_ptr());
+        std::ptr::drop_in_place(ret.as_ptr());
+      }
+    }
   }
 }
 
@@ -824,6 +840,13 @@ impl JsRuntime {
     };
     op_state.borrow_mut().put(isolate_ptr);
 
+    let mut fast_fn_infos = Vec::with_capacity(op_ctxs.len());
+    for op_ctx in &*op_ctxs {
+      if let Some(fast_fn_info) = op_ctx.fast_fn_info {
+        fast_fn_infos.push(fast_fn_info);
+      }
+    }
+
     // ...once ops and isolate are set up, we can create a `ContextState`...
     let context_state = Rc::new(ContextState::new(
       op_driver.clone(),
@@ -976,6 +999,7 @@ impl JsRuntime {
         main_realm: ManuallyDrop::new(main_realm),
         state: ManuallyDropRc(ManuallyDrop::new(state_rc)),
         v8_isolate: ManuallyDrop::new(isolate),
+        fast_fn_infos,
       },
       allocations: isolate_allocations,
       files_loaded_from_fs_during_snapshot: vec![],