Change plugin interface to prevent segfaults when unloading plugin (d…

…enoland#5210)

Fixes: denoland#3473
Closes: denoland#5193

cli/ops/mod.rs

@@ -19,7 +19,7 @@ pub mod net;
 mod net_unix;
 pub mod os;
 pub mod permissions;
-pub mod plugins;
+pub mod plugin;
 pub mod process;
 pub mod random;
 pub mod repl;

cli/ops/plugin.rs

@@ -0,0 +1,153 @@
+// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
+use crate::fs::resolve_from_cwd;
+use crate::op_error::OpError;
+use crate::ops::dispatch_json::Deserialize;
+use crate::ops::dispatch_json::JsonOp;
+use crate::ops::dispatch_json::Value;
+use crate::ops::json_op;
+use crate::state::State;
+use deno_core::plugin_api;
+use deno_core::CoreIsolate;
+use deno_core::Op;
+use deno_core::OpAsyncFuture;
+use deno_core::OpId;
+use deno_core::ZeroCopyBuf;
+use dlopen::symbor::Library;
+use futures::prelude::*;
+use std::path::Path;
+use std::pin::Pin;
+use std::rc::Rc;
+use std::task::Context;
+use std::task::Poll;
+
+pub fn init(i: &mut CoreIsolate, s: &State) {
+ i.register_op(
+ "op_open_plugin",
+ s.core_op(json_op(s.stateful_op2(op_open_plugin))),
+ );
+}
+
+#[derive(Deserialize)]
+#[serde(rename_all = "camelCase")]
+struct OpenPluginArgs {
+ filename: String,
+}
+
+pub fn op_open_plugin(
+ isolate: &mut CoreIsolate,
+ state: &State,
+ args: Value,
+ _zero_copy: Option<ZeroCopyBuf>,
+) -> Result<JsonOp, OpError> {
+ state.check_unstable("Deno.openPlugin");
+ let args: OpenPluginArgs = serde_json::from_value(args).unwrap();
+ let filename = resolve_from_cwd(Path::new(&args.filename))?;
+
+ state.check_plugin(&filename)?;
+
+ debug!("Loading Plugin: {:#?}", filename);
+ let plugin_lib = Library::open(filename)
+ .map(Rc::new)
+ .map_err(OpError::from)?;
+ let plugin_resource = PluginResource::new(&plugin_lib);
+
+ let mut resource_table = isolate.resource_table.borrow_mut();
+ let rid = resource_table.add("plugin", Box::new(plugin_resource));
+ let plugin_resource = resource_table.get::<PluginResource>(rid).unwrap();
+
+ let deno_plugin_init = *unsafe {
+ plugin_resource
+ .lib
+ .symbol::<plugin_api::InitFn>("deno_plugin_init")
+ }
+ .unwrap();
+ drop(resource_table);
+
+ let mut interface = PluginInterface::new(isolate, &plugin_lib);
+ deno_plugin_init(&mut interface);
+
+ Ok(JsonOp::Sync(json!(rid)))
+}
+
+struct PluginResource {
+ lib: Rc<Library>,
+}
+
+impl PluginResource {
+ fn new(lib: &Rc<Library>) -> Self {
+ Self { lib: lib.clone() }
+ }
+}
+
+struct PluginInterface<'a> {
+ isolate: &'a mut CoreIsolate,
+ plugin_lib: &'a Rc<Library>,
+}
+
+impl<'a> PluginInterface<'a> {
+ fn new(isolate: &'a mut CoreIsolate, plugin_lib: &'a Rc<Library>) -> Self {
+ Self {
+ isolate,
+ plugin_lib,
+ }
+ }
+}
+
+impl<'a> plugin_api::Interface for PluginInterface<'a> {
+ /// Does the same as `core::Isolate::register_op()`, but additionally makes
+ /// the registered op dispatcher, as well as the op futures created by it,
+ /// keep reference to the plugin `Library` object, so that the plugin doesn't
+ /// get unloaded before all its op registrations and the futures created by
+ /// them are dropped.
+ fn register_op(
+ &mut self,
+ name: &str,
+ dispatch_op_fn: plugin_api::DispatchOpFn,
+ ) -> OpId {
+ let plugin_lib = self.plugin_lib.clone();
+ self.isolate.op_registry.register(
+ name,
+ move |isolate, control, zero_copy| {
+ let mut interface = PluginInterface::new(isolate, &plugin_lib);
+ let op = dispatch_op_fn(&mut interface, control, zero_copy);
+ match op {
+ sync_op @ Op::Sync(..) => sync_op,
+ Op::Async(fut) => {
+ Op::Async(PluginOpAsyncFuture::new(&plugin_lib, fut))
+ }
+ Op::AsyncUnref(fut) => {
+ Op::AsyncUnref(PluginOpAsyncFuture::new(&plugin_lib, fut))
+ }
+ }
+ },
+ )
+ }
+}
+
+struct PluginOpAsyncFuture {
+ fut: Option<OpAsyncFuture>,
+ _plugin_lib: Rc<Library>,
+}
+
+impl PluginOpAsyncFuture {
+ fn new(plugin_lib: &Rc<Library>, fut: OpAsyncFuture) -> Pin<Box<Self>> {
+ let wrapped_fut = Self {
+ fut: Some(fut),
+ _plugin_lib: plugin_lib.clone(),
+ };
+ Box::pin(wrapped_fut)
+ }
+}
+
+impl Future for PluginOpAsyncFuture {
+ type Output = <OpAsyncFuture as Future>::Output;
+ fn poll(mut self: Pin<&mut Self>, ctx: &mut Context) -> Poll<Self::Output> {
+ self.fut.as_mut().unwrap().poll_unpin(ctx)
+ }
+}
+
+impl Drop for PluginOpAsyncFuture {
+ fn drop(&mut self) {
+ self.fut.take();
+ }
+}

cli/web_worker.rs

@@ -136,7 +136,7 @@ impl WebWorker {
  ops::runtime_compiler::init(isolate, &state);
  ops::fs::init(isolate, &state);
  ops::fs_events::init(isolate, &state);
- ops::plugins::init(isolate, &state);
+ ops::plugin::init(isolate, &state);
  ops::net::init(isolate, &state);
  ops::tls::init(isolate, &state);
  ops::os::init(isolate, &state);

cli/worker.rs

@@ -243,7 +243,7 @@ impl MainWorker {
  ops::fs::init(isolate, &state);
  ops::fs_events::init(isolate, &state);
  ops::io::init(isolate, &state);
- ops::plugins::init(isolate, &state);
+ ops::plugin::init(isolate, &state);
  ops::net::init(isolate, &state);
  ops::tls::init(isolate, &state);
  ops::os::init(isolate, &state);

core/lib.rs

@@ -17,6 +17,7 @@ mod js_errors;
 mod module_specifier;
 mod modules;
 mod ops;
+pub mod plugin_api;
 mod resources;
 mod shared_queue;
 

core/plugin_api.rs

@@ -0,0 +1,23 @@
+// Copyright 2018-2020 the Deno authors. All rights reserved. MIT license.
+
+// This file defines the public interface for dynamically loaded plugins.
+
+// The plugin needs to do all interaction with the CLI crate through trait
+// objects and function pointers. This ensures that no concrete internal methods
+// (such as register_op and the closures created by it) can end up in the plugin
+// shared library itself, which would cause segfaults when the plugin is
+// unloaded and all functions in the plugin library are unmapped from memory.
+
+pub use crate::Buf;
+pub use crate::Op;
+pub use crate::OpId;
+pub use crate::ZeroCopyBuf;
+
+pub type InitFn = fn(&mut dyn Interface);
+
+pub type DispatchOpFn =
+ fn(&mut dyn Interface, &[u8], Option<ZeroCopyBuf>) -> Op;
+
+pub trait Interface {
+ fn register_op(&mut self, name: &str, dispatcher: DispatchOpFn) -> OpId;
+}

test_plugin/Cargo.toml

@@ -11,4 +11,3 @@ crate-type = ["cdylib"]
 [dependencies]
 futures = "0.3.4"
 deno_core = { path = "../core" }
-deno = { path = "../cli" }

test_plugin/src/lib.rs

@@ -1,20 +1,17 @@
-extern crate deno_core;
-extern crate futures;
-
-use deno_core::Buf;
-use deno_core::CoreIsolate;
-use deno_core::Op;
-use deno_core::ZeroCopyBuf;
+use deno_core::plugin_api::Buf;
+use deno_core::plugin_api::Interface;
+use deno_core::plugin_api::Op;
+use deno_core::plugin_api::ZeroCopyBuf;
 use futures::future::FutureExt;
 
 #[no_mangle]
-pub fn deno_plugin_init(isolate: &mut CoreIsolate) {
- isolate.register_op("testSync", op_test_sync);
- isolate.register_op("testAsync", op_test_async);
+pub fn deno_plugin_init(interface: &mut dyn Interface) {
+ interface.register_op("testSync", op_test_sync);
+ interface.register_op("testAsync", op_test_async);
 }
 
-pub fn op_test_sync(
- _isolate: &mut CoreIsolate,
+fn op_test_sync(
+ _interface: &mut dyn Interface,
  data: &[u8],
  zero_copy: Option<ZeroCopyBuf>,
 ) -> Op {
@@ -31,8 +28,8 @@ pub fn op_test_sync(
  Op::Sync(result_box)
 }
 
-pub fn op_test_async(
- _isolate: &mut CoreIsolate,
+fn op_test_async(
+ _interface: &mut dyn Interface,
  data: &[u8],
  zero_copy: Option<ZeroCopyBuf>,
 ) -> Op {

test_plugin/tests/integration_tests.rs

@@ -1,13 +1,26 @@
 // To run this test manually:
 // cd test_plugin
-// ../target/debug/deno --allow-plugin tests/test.js debug
+// ../target/debug/deno run --unstable --allow-plugin tests/test.js debug
 
-// TODO(ry) Re-enable this test on windows. It is flaky for an unknown reason.
-#![cfg(not(windows))]
-
-use deno::test_util::*;
+use std::path::PathBuf;
 use std::process::Command;
 
+fn target_dir() -> PathBuf {
+ let current_exe = std::env::current_exe().unwrap();
+ let target_dir = current_exe.parent().unwrap().parent().unwrap();
+ println!("target_dir {}", target_dir.display());
+ target_dir.into()
+}
+
+fn deno_exe_path() -> PathBuf {
+ // Something like /Users/rld/src/deno/target/debug/deps/deno
+ let mut p = target_dir().join("deno");
+ if cfg!(windows) {
+ p.set_extension("exe");
+ }
+ p
+}
+
 fn deno_cmd() -> Command {
  assert!(deno_exe_path().exists());
  Command::new(deno_exe_path())