Merge #56

56: Simplify sandbox r=MikailBag a=MikailBag



Co-authored-by: Mikail Bagishov <bagishov.mikail@yandex.ru>

Author: bors[bot]

minion-ffi/src/lib.rs

@@ -5,6 +5,7 @@ use std::{
     ffi::{CStr, OsStr, OsString},
     mem::{self},
     os::raw::c_char,
+    sync::Arc,
 };
 
 #[repr(i32)]
@@ -106,7 +107,7 @@ pub struct SandboxOptions {
 }
 
 #[derive(Clone)]
-pub struct Sandbox(Box<dyn minion::erased::Sandbox>);
+pub struct Sandbox(Arc<dyn minion::erased::Sandbox>);
 
 /// # Safety
 /// `out` must be valid

src/command.rs

@@ -2,12 +2,13 @@ use crate::{erased, InputSpecification, OutputSpecification, StdioSpecification}
 use std::{
     ffi::{OsStr, OsString},
     path::{Path, PathBuf},
+    sync::Arc,
 };
 
 /// Child process builder
 #[derive(Default, Debug)]
 pub struct Command {
-    sandbox: Option<Box<dyn erased::Sandbox>>,
+    sandbox: Option<Arc<dyn erased::Sandbox>>,
     exe: Option<PathBuf>,
     argv: Vec<OsString>,
     env: Vec<OsString>,
@@ -56,7 +57,7 @@ impl Command {
         backend.spawn(options)
     }
 
-    pub fn sandbox(&mut self, sandbox: Box<dyn erased::Sandbox>) -> &mut Self {
+    pub fn sandbox(&mut self, sandbox: Arc<dyn erased::Sandbox>) -> &mut Self {
         self.sandbox.replace(sandbox);
         self
     }

src/erased.rs

@@ -3,6 +3,7 @@
 //!
 //! Please note that this API is not type-safe. For example, if you pass
 //! `Sandbox` instance to another backend, it will panic.
+use std::{any::Any, sync::Arc};
 
 use futures_util::{FutureExt, TryFutureExt};
 
@@ -13,16 +14,7 @@ pub trait Sandbox: std::fmt::Debug + Send + Sync + 'static {
     fn check_real_tle(&self) -> anyhow::Result<bool>;
     fn kill(&self) -> anyhow::Result<()>;
     fn resource_usage(&self) -> anyhow::Result<crate::ResourceUsageData>;
-    #[doc(hidden)]
-    fn clone_to_box(&self) -> Box<dyn Sandbox>;
-    #[doc(hidden)]
-    fn clone_into_box_any(&self) -> Box<dyn std::any::Any>;
-}
-
-impl Clone for Box<dyn Sandbox> {
-    fn clone(&self) -> Self {
-        self.clone_to_box()
-    }
+    fn into_arc_any(self: Arc<Self>) -> Arc<dyn Any + Send + Sync + 'static>;
 }
 
 impl<S: crate::Sandbox> Sandbox for S {
@@ -41,12 +33,8 @@ impl<S: crate::Sandbox> Sandbox for S {
     fn resource_usage(&self) -> anyhow::Result<crate::ResourceUsageData> {
         self.resource_usage().map_err(Into::into)
     }
-    fn clone_to_box(&self) -> Box<dyn Sandbox> {
-        Box::new(self.clone())
-    }
-
-    fn clone_into_box_any(&self) -> Box<dyn std::any::Any> {
-        Box::new(self.clone())
+    fn into_arc_any(self: Arc<Self>) -> Arc<dyn Any + Send + Sync + 'static> {
+        self
     }
 }
 
@@ -89,36 +77,33 @@ impl<C: crate::ChildProcess> ChildProcess for C {
 
 /// Type-erased `Backend`
 pub trait Backend: Send + Sync + 'static {
-    fn new_sandbox(&self, options: crate::SandboxOptions) -> anyhow::Result<Box<dyn Sandbox>>;
+    fn new_sandbox(&self, options: crate::SandboxOptions) -> anyhow::Result<Arc<dyn Sandbox>>;
     fn spawn(&self, options: ChildProcessOptions) -> anyhow::Result<Box<dyn ChildProcess>>;
 }
 
 impl<B: crate::Backend> Backend for B {
-    fn new_sandbox(&self, options: crate::SandboxOptions) -> anyhow::Result<Box<dyn Sandbox>> {
+    fn new_sandbox(&self, options: crate::SandboxOptions) -> anyhow::Result<Arc<dyn Sandbox>> {
         let sb = <Self as crate::Backend>::new_sandbox(&self, options)?;
-        Ok(Box::new(sb))
+        Ok(Arc::new(sb))
     }
 
     fn spawn(&self, options: ChildProcessOptions) -> anyhow::Result<Box<dyn ChildProcess>> {
-        let down_sandbox = options
-            .sandbox
-            .clone_into_box_any()
-            .downcast()
-            .expect("sandbox type mismatch");
+        let any_sandbox = options.sandbox.clone().into_arc_any();
+        let down_sandbox = any_sandbox.downcast().expect("sandbox type mismatch");
         let down_options = crate::ChildProcessOptions {
             arguments: options.arguments,
             environment: options.environment,
             path: options.path,
             pwd: options.pwd,
             stdio: options.stdio,
-            sandbox: *down_sandbox,
+            sandbox: down_sandbox,
         };
         let cp = <Self as crate::Backend>::spawn(&self, down_options)?;
         Ok(Box::new(cp))
     }
 }
 
-pub type ChildProcessOptions = crate::ChildProcessOptions<Box<dyn Sandbox>>;
+pub type ChildProcessOptions = crate::ChildProcessOptions<dyn Sandbox>;
 
 /// Returns backend instance
 pub fn setup() -> anyhow::Result<Box<dyn Backend>> {

src/lib.rs

@@ -26,6 +26,7 @@ use std::{
     error::Error as StdError,
     fmt::Debug,
     io::{Read, Write},
+    sync::Arc,
     time::Duration,
 };
 
@@ -111,7 +112,7 @@ impl SandboxOptions {
 }
 
 /// Represents highly-isolated sandbox
-pub trait Sandbox: Clone + Debug + Send + Sync + 'static {
+pub trait Sandbox: Debug + Send + Sync + 'static {
     type Error: StdError + Send + Sync + 'static;
     fn id(&self) -> String;
 
@@ -227,11 +228,11 @@ pub struct StdioSpecification {
 /// This type should only be used by Backend implementations
 /// Use `Command` instead
 #[derive(Debug, Clone)]
-pub struct ChildProcessOptions<Sandbox> {
+pub struct ChildProcessOptions<Sandbox: ?Sized> {
     pub path: PathBuf,
     pub arguments: Vec<OsString>,
     pub environment: Vec<OsString>,
-    pub sandbox: Sandbox,
+    pub sandbox: Arc<Sandbox>,
     pub stdio: StdioSpecification,
     /// Child's working dir. Relative to `sandbox` isolation_root
     pub pwd: PathBuf,

src/linux.rs

@@ -25,8 +25,10 @@ use std::{
     fs,
     os::unix::io::{IntoRawFd, RawFd},
     path::PathBuf,
-    sync::atomic::{AtomicI64, Ordering},
-    sync::Arc,
+    sync::{
+        atomic::{AtomicI64, Ordering},
+        Arc,
+    },
 };
 
 pub type LinuxHandle = libc::c_int;
@@ -36,8 +38,8 @@ pub struct LinuxChildProcess {
     stdin: Option<LinuxWritePipe>,
     stdout: Option<LinuxReadPipe>,
     stderr: Option<LinuxReadPipe>,
-    sandbox_ref: LinuxSandbox,
-    /// Child pid, relative to sandbox namespace.
+    sandbox_ref: Arc<LinuxSandbox>,
+
     pid: Pid,
     /// FD of object which will be readable when child finishes.
     /// Wrapped in Option to catch user errors.

src/linux/sandbox.rs

@@ -49,11 +49,8 @@ impl SandboxState {
     }
 }
 
-#[derive(Clone)]
-pub struct LinuxSandbox(Arc<LinuxSandboxInner>);
-
 #[repr(C)]
-struct LinuxSandboxInner {
+pub struct LinuxSandbox {
     id: String,
     options: SandboxOptions,
     zygote_sock: Mutex<Socket>,
@@ -76,12 +73,12 @@ struct LinuxSandboxDebugHelper<'a> {
 impl Debug for LinuxSandbox {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         let h = LinuxSandboxDebugHelper {
-            id: &self.0.id,
-            options: &self.0.options,
-            zygote_sock: self.0.zygote_sock.lock().unwrap().as_raw_fd(),
-            zygote_pid: self.0.zygote_pid,
-            watchdog_chan: self.0.watchdog_chan,
-            state: self.0.state.snapshot(),
+            id: &self.id,
+            options: &self.options,
+            zygote_sock: self.zygote_sock.lock().unwrap().as_raw_fd(),
+            zygote_pid: self.zygote_pid,
+            watchdog_chan: self.watchdog_chan,
+            state: self.state.snapshot(),
         };
 
         h.fmt(f)
@@ -92,28 +89,28 @@ impl Sandbox for LinuxSandbox {
     type Error = Error;
 
     fn id(&self) -> String {
-        self.0.id.clone()
+        self.id.clone()
     }
 
     fn check_cpu_tle(&self) -> Result<bool, Error> {
         self.poll_state()?;
-        Ok(self.0.state.was_cpu_tle.load(SeqCst))
+        Ok(self.state.was_cpu_tle.load(SeqCst))
     }
 
     fn check_real_tle(&self) -> Result<bool, Error> {
         self.poll_state()?;
-        Ok(self.0.state.was_wall_tle.load(SeqCst))
+        Ok(self.state.was_wall_tle.load(SeqCst))
     }
 
     fn kill(&self) -> Result<(), Error> {
-        jail_common::kill_sandbox(self.0.zygote_pid, &self.0.id, &self.0.cgroup_driver)
+        jail_common::kill_sandbox(self.zygote_pid, &self.id, &self.cgroup_driver)
             .map_err(|err| Error::Io { cause: err })?;
         Ok(())
     }
 
     fn resource_usage(&self) -> Result<crate::ResourceUsageData, Error> {
-        let cpu_usage = self.0.cgroup_driver.get_cpu_usage(&self.0.id)?;
-        let memory_usage = self.0.cgroup_driver.get_memory_usage(&self.0.id)?;
+        let cpu_usage = self.cgroup_driver.get_cpu_usage(&self.id)?;
+        let memory_usage = self.cgroup_driver.get_memory_usage(&self.id)?;
         Ok(crate::ResourceUsageData {
             memory: memory_usage,
             time: Some(cpu_usage),
@@ -132,7 +129,7 @@ impl LinuxSandbox {
     fn poll_state(&self) -> Result<(), Error> {
         for _ in 0..5 {
             let mut buf = [0; 4];
-            let num_read = nix::unistd::read(self.0.watchdog_chan, &mut buf).or_else(|err| {
+            let num_read = nix::unistd::read(self.watchdog_chan, &mut buf).or_else(|err| {
                 if let Some(errno) = err.as_errno() {
                     if errno as i32 == libc::EAGAIN {
                         return Ok(0);
@@ -144,7 +141,7 @@ impl LinuxSandbox {
                 break;
             }
             for ch in &buf[..num_read] {
-                self.0.state.process_flag(*ch)?;
+                self.state.process_flag(*ch)?;
             }
         }
 
@@ -178,7 +175,7 @@ impl LinuxSandbox {
         };
         let startup_info = zygote::start_zygote(jail_options, &cgroup_driver)?;
 
-        let inner = LinuxSandboxInner {
+        let sandbox = LinuxSandbox {
             id: jail_id,
             options,
             zygote_sock: Mutex::new(startup_info.socket),
@@ -191,7 +188,7 @@ impl LinuxSandbox {
             cgroup_driver,
         };
 
-        Ok(LinuxSandbox(Arc::new(inner)))
+        Ok(sandbox)
     }
 
     pub(crate) unsafe fn spawn_job(
@@ -200,7 +197,7 @@ impl LinuxSandbox {
     ) -> Result<(jail_common::JobStartupInfo, RawFd), Error> {
         let q = jail_common::Query::Spawn(query.job_query.clone());
 
-        let mut sock = self.0.zygote_sock.lock().unwrap();
+        let mut sock = self.zygote_sock.lock().unwrap();
 
         // note that we ignore errors, because zygote can be already killed for some reason
         sock.send(&q).ok();
@@ -219,7 +216,7 @@ impl LinuxSandbox {
 
     pub(crate) fn get_exit_code(&self, pid: Pid) -> ExitCode {
         let q = jail_common::Query::GetExitCode(jail_common::GetExitCodeQuery { pid });
-        let mut sock = self.0.zygote_sock.lock().unwrap();
+        let mut sock = self.zygote_sock.lock().unwrap();
         sock.send(&q).ok();
         match sock.recv::<i32>() {
             Ok(ec) => ExitCode(ec.into()),
@@ -230,24 +227,17 @@ impl LinuxSandbox {
 
 impl Drop for LinuxSandbox {
     fn drop(&mut self) {
-        match Arc::get_mut(&mut self.0) {
-            // we are last Sandbox handle, so we can drop it
-            Some(_) => (),
-            // there are other handles, so we must not do anyhing
-            None => return,
-        };
         // Kill all processes.
         if let Err(err) = self.kill() {
             panic!("unable to kill sandbox: {}", err);
         }
         // Remove cgroups.
         if std::env::var("MINION_DEBUG_KEEP_CGROUPS").is_err() {
-            self.0
-                .cgroup_driver
-                .drop_cgroup(&self.0.id, &["pids", "memory", "cpuacct"]);
+            self.cgroup_driver
+                .drop_cgroup(&self.id, &["pids", "memory", "cpuacct"]);
         }
 
         // Close handles
-        nix::unistd::close(self.0.watchdog_chan).ok();
+        nix::unistd::close(self.watchdog_chan).ok();
     }
 }

src/linux/wait.rs

@@ -1,8 +1,12 @@
 //! Implements wait future
-use crate::{linux::util::Pid, linux::LinuxSandbox, ExitCode};
+use crate::{
+    linux::{util::Pid, LinuxSandbox},
+    ExitCode,
+};
 use std::{
     os::unix::io::RawFd,
     pin::Pin,
+    sync::Arc,
     task::{Context, Poll},
 };
 use tokio::io::unix::AsyncFd;
@@ -36,15 +40,15 @@ pub struct WaitFuture {
     /// FD of underlying event source (either pidfd or unix socket)
     // TODO: use pipe instead of socket
     inner: AsyncFd<OwnedFd>,
-    sandbox: LinuxSandbox,
+    sandbox: Arc<LinuxSandbox>,
     pid: Pid,
 }
 
 impl WaitFuture {
     pub(crate) fn new(
         fd: RawFd,
         pid: Pid,
-        sandbox: LinuxSandbox,
+        sandbox: Arc<LinuxSandbox>,
     ) -> Result<Self, crate::linux::Error> {
         let inner = AsyncFd::new(OwnedFd(fd))?;
         Ok(WaitFuture {