Abandon contract after running command in zone (#3761)

Author: citrus-it

illumos-utils/src/lib.rs

@@ -90,8 +90,33 @@ mod inner {
         }))
     }
 
-    // Helper function for starting the process and checking the
+    // Helper functions for starting the process and checking the
     // exit code result.
+
+    pub fn spawn(
+        command: &mut std::process::Command,
+    ) -> Result<std::process::Child, ExecutionError> {
+        command.spawn().map_err(|err| ExecutionError::ExecutionStart {
+            command: to_string(command),
+            err,
+        })
+    }
+
+    pub fn run_child(
+        command: &mut std::process::Command,
+        child: std::process::Child,
+    ) -> Result<std::process::Output, ExecutionError> {
+        let output = child.wait_with_output().map_err(|err| {
+            ExecutionError::ExecutionStart { command: to_string(command), err }
+        })?;
+
+        if !output.status.success() {
+            return Err(output_to_exec_error(command, &output));
+        }
+
+        Ok(output)
+    }
+
     pub fn execute(
         command: &mut std::process::Command,
     ) -> Result<std::process::Output, ExecutionError> {

illumos-utils/src/running_zone.rs

@@ -12,6 +12,8 @@ use crate::svc::wait_for_service;
 use crate::zone::{AddressRequest, IPADM, ZONE_PREFIX};
 use camino::{Utf8Path, Utf8PathBuf};
 use ipnetwork::IpNetwork;
+#[cfg(target_os = "illumos")]
+use libc::pid_t;
 use omicron_common::backoff;
 use slog::error;
 use slog::info;
@@ -156,10 +158,15 @@ pub enum GetZoneError {
 // inside a non-global zone.
 #[cfg(target_os = "illumos")]
 mod zenter {
+    use libc::ctid_t;
+    use libc::pid_t;
     use libc::zoneid_t;
     use std::ffi::c_int;
     use std::ffi::c_uint;
-    use std::ffi::CStr;
+    use std::ffi::{CStr, CString};
+    use std::fs::File;
+    use std::os::unix::prelude::FileExt;
+    use std::process;
 
     #[link(name = "contract")]
     extern "C" {
@@ -169,13 +176,68 @@ mod zenter {
         fn ct_pr_tmpl_set_param(fd: c_int, params: c_uint) -> c_int;
         fn ct_tmpl_activate(fd: c_int) -> c_int;
         fn ct_tmpl_clear(fd: c_int) -> c_int;
+        fn ct_ctl_abandon(fd: c_int) -> c_int;
     }
 
     #[link(name = "c")]
     extern "C" {
         pub fn zone_enter(zid: zoneid_t) -> c_int;
     }
 
+    #[derive(thiserror::Error, Debug)]
+    pub enum AbandonContractError {
+        #[error("Error opening file {file}: {error}")]
+        Open { file: String, error: std::io::Error },
+
+        #[error("Error abandoning contract {ctid}: {error}")]
+        Abandon { ctid: ctid_t, error: std::io::Error },
+
+        #[error("Error closing file {file}: {error}")]
+        Close { file: String, error: std::io::Error },
+    }
+
+    pub fn get_contract(pid: pid_t) -> std::io::Result<ctid_t> {
+        // The offset of "id_t pr_contract" in struct psinfo which is an
+        // interface documented in proc(5).
+        const CONTRACT_OFFSET: u64 = 280;
+
+        let path = format!("/proc/{}/psinfo", pid);
+
+        let file = File::open(path)?;
+        let mut buffer = [0; 4];
+        file.read_exact_at(&mut buffer, CONTRACT_OFFSET)?;
+        Ok(ctid_t::from_ne_bytes(buffer))
+    }
+
+    pub fn abandon_contract(ctid: ctid_t) -> Result<(), AbandonContractError> {
+        let path = format!("/proc/{}/contracts/{}/ctl", process::id(), ctid);
+
+        let cpath = CString::new(path.clone()).unwrap();
+        let fd = unsafe { libc::open(cpath.as_ptr(), libc::O_WRONLY) };
+        if fd < 0 {
+            return Err(AbandonContractError::Open {
+                file: path,
+                error: std::io::Error::last_os_error(),
+            });
+        }
+        let ret = unsafe { ct_ctl_abandon(fd) };
+        if ret != 0 {
+            unsafe { libc::close(fd) };
+            return Err(AbandonContractError::Abandon {
+                ctid,
+                error: std::io::Error::from_raw_os_error(ret),
+            });
+        }
+        if unsafe { libc::close(fd) } != 0 {
+            return Err(AbandonContractError::Close {
+                file: path,
+                error: std::io::Error::last_os_error(),
+            });
+        }
+
+        Ok(())
+    }
+
     // A Rust wrapper around the process contract template.
     #[derive(Debug)]
     pub struct Template {
@@ -277,11 +339,11 @@ impl RunningZone {
     // another, if the task is swapped out at an await point. That would leave
     // the first thread's template in a modified state.
     //
-    // If we do need to make this method asynchronous, we will need to change the
-    // internals to avoid changing the thread's contract. One possible approach
-    // here would be to use `libscf` directly, rather than `exec`-ing `svccfg`
-    // directly in a forked child. That would obviate the need to work on the
-    // contract at all.
+    // If we do need to make this method asynchronous, we will need to change
+    // the internals to avoid changing the thread's contract. One possible
+    // approach here would be to use `libscf` directly, rather than `exec`-ing
+    // `svccfg` directly in a forked child. That would obviate the need to work
+    // on the contract at all.
     #[cfg(target_os = "illumos")]
     pub fn run_cmd<I, S>(&self, args: I) -> Result<String, RunCommandError>
     where
@@ -319,13 +381,46 @@ impl RunningZone {
         }
         let command = command.args(args);
 
-        // Capture the result, and be sure to clear the template for this
-        // process itself before returning.
-        let res = crate::execute(command).map_err(|err| RunCommandError {
+        let child = crate::spawn(command).map_err(|err| RunCommandError {
             zone: self.name().to_string(),
             err,
+        })?;
+
+        // Record the process contract now in use by the child; the contract
+        // just created from the template that we applied to this thread
+        // moments ago. We need to abandon it once the child is finished
+        // executing.
+        // unwrap() safety - child.id() returns u32 but pid_t is i32.
+        // PID_MAX is 999999 so this will not overflow.
+        let child_pid: pid_t = child.id().try_into().unwrap();
+        let contract = zenter::get_contract(child_pid);
+
+        // Capture the result, and be sure to clear the template for this
+        // process itself before returning.
+        let res = crate::run_child(command, child).map_err(|err| {
+            RunCommandError { zone: self.name().to_string(), err }
         });
         template.clear();
+
+        // Now abandon the contract that was used for the child.
+        match contract {
+            Err(e) => error!(
+                self.inner.log,
+                "Could not retrieve contract for pid {}: {}", child_pid, e
+            ),
+            Ok(ctid) => {
+                if let Err(e) = zenter::abandon_contract(ctid) {
+                    error!(
+                        self.inner.log,
+                        "Failed to abandon contract {} for pid {}: {}",
+                        ctid,
+                        child_pid,
+                        e
+                    );
+                }
+            }
+        }
+
         res.map(|output| String::from_utf8_lossy(&output.stdout).to_string())
     }
 
@@ -342,7 +437,11 @@ impl RunningZone {
         // that's actually run is irrelevant.
         let mut command = std::process::Command::new("echo");
         let command = command.args(args);
-        crate::execute(command)
+        let child = crate::spawn(command).map_err(|err| RunCommandError {
+            zone: self.name().to_string(),
+            err,
+        })?;
+        crate::run_child(command, child)
             .map_err(|err| RunCommandError {
                 zone: self.name().to_string(),
                 err,

sled-agent/src/services.rs

@@ -3176,8 +3176,19 @@ mod test {
         wait_ctx.expect().return_once(|_, _| Ok(()));
 
         // Import the manifest, enable the service
-        let execute_ctx = illumos_utils::execute_context();
-        execute_ctx.expect().times(..).returning(|_| {
+        let spawn_ctx = illumos_utils::spawn_context();
+        spawn_ctx.expect().times(..).returning(|_| {
+            std::process::Command::new("/bin/false").spawn().map_err(|err| {
+                illumos_utils::ExecutionError::ExecutionStart {
+                    command: "mock".to_string(),
+                    err,
+                }
+            })
+        });
+        let run_child_ctx = illumos_utils::run_child_context();
+        run_child_ctx.expect().times(..).returning(|_, mut child| {
+            let _ = child.kill();
+
             Ok(std::process::Output {
                 status: std::process::ExitStatus::from_raw(0),
                 stdout: vec![],
@@ -3192,7 +3203,8 @@ mod test {
             Box::new(id_ctx),
             Box::new(ensure_address_ctx),
             Box::new(wait_ctx),
-            Box::new(execute_ctx),
+            Box::new(spawn_ctx),
+            Box::new(run_child_ctx),
         ]
     }