feature: improve error messages

Cargo.toml

@@ -23,7 +23,7 @@ tokio = { version = "1.52.3", optional = true }
 tempfile = "3"
 
 [features]
-async = ["dep:tokio"]
+async = ["dep:tokio", "tokio/process"]
 
 [lib]
 name = "styrolite"

bin/styrojail.rs

@@ -1,6 +1,6 @@
 use std::{env, path::PathBuf};
 
-use anyhow::{Result, anyhow};
+use anyhow::{Result, anyhow, bail};
 use clap::Parser;
 use styrolite::config::{IdMapping, MountSpec as StyroMountSpec};
 use styrolite::namespace::Namespace;
@@ -207,5 +207,10 @@ fn main() -> Result<()> {
     let runner = Runner::new(&cli.styrolite_bin);
     runner.exec(req)?;
 
-    Err(anyhow!("styrolite exec failed"))
+    // exec() replaces this process image on success and returns Err on
+    // failure (propagated above), so reaching here is never expected.
+    bail!(
+        "styrolite runner '{}' returned without exec'ing the workload",
+        cli.styrolite_bin
+    )
 }

bin/styrolite.rs

@@ -1,22 +1,27 @@
 use std::{env, fs, path::PathBuf};
 
-use anyhow::Result;
+use anyhow::{Context, Result, bail};
 use env_logger::{Env, fmt::TimestampPrecision};
 use styrolite::config::{Config, Wrappable};
 
 fn main() -> Result<()> {
-    let config_path = env::args().nth(1).expect("config file path missing");
+    let Some(config_path) = env::args().nth(1) else {
+        bail!("missing config file path (usage: styrolite <config.json>)");
+    };
     let config_path = PathBuf::from(config_path);
 
     if !config_path.exists() {
-        panic!("config file did not exist");
+        bail!("config file '{}' does not exist", config_path.display());
     }
 
     env_logger::Builder::from_env(Env::default().default_filter_or("info"))
         .format_timestamp(Some(TimestampPrecision::Micros))
         .init();
 
-    let config: Config = serde_json::from_slice(&fs::read(&config_path)?)?;
+    let raw = fs::read(&config_path)
+        .with_context(|| format!("failed to read config file '{}'", config_path.display()))?;
+    let config: Config = serde_json::from_slice(&raw)
+        .with_context(|| format!("failed to parse config file '{}'", config_path.display()))?;
     match config {
         Config::Create(create) => create.wrap(),
         Config::Attach(attach) => attach.wrap(),

src/caps.rs

@@ -325,7 +325,7 @@ pub struct CapResult {
 fn capget(result: &mut CapInternalResult) -> anyhow::Result<()> {
     unsafe {
         if syscall(libc::SYS_capget, &result.header, &result.data) < 0 {
-            Err(anyhow!("capget(2) failed"))
+            Err(anyhow!("capget(2) failed: {}", io::Error::last_os_error()))
         } else {
             Ok(())
         }
@@ -334,8 +334,8 @@ fn capget(result: &mut CapInternalResult) -> anyhow::Result<()> {
 
 fn capset(result: &CapInternalResult) -> anyhow::Result<()> {
     unsafe {
-        let err = syscall(libc::SYS_capset, &result.header, &result.data);
-        if err < 0 {
+        if syscall(libc::SYS_capset, &result.header, &result.data) < 0 {
+            let os_err = io::Error::last_os_error();
             let effective =
                 ((result.data[0].effective as u64) << 32) | result.data[1].effective as u64;
             let permitted =
@@ -344,10 +344,8 @@ fn capset(result: &CapInternalResult) -> anyhow::Result<()> {
                 ((result.data[0].inheritable as u64) << 32) | result.data[1].inheritable as u64;
 
             Err(anyhow!(
-                "capset(2) failed: {:x} {:x} {:x} (error = {err})",
-                effective,
-                permitted,
-                inheritable
+                "capset(2) failed (effective={effective:x} permitted={permitted:x} \
+                 inheritable={inheritable:x}): {os_err}"
             ))
         } else {
             Ok(())

src/mount.rs

@@ -270,7 +270,15 @@ impl Mountable for MountSpec {
         let data_cstr = self
             .data
             .as_ref()
-            .map(|d| CString::new(d.as_str()).unwrap());
+            .map(|d| {
+                CString::new(d.as_str()).map_err(|e| {
+                    anyhow!(
+                        "mount data '{d}' for {} contains an interior NUL byte: {e}",
+                        self.target
+                    )
+                })
+            })
+            .transpose()?;
         let data_ptr = data_cstr
             .as_ref()
             .map(|c| c.as_ptr() as *const libc::c_void)
@@ -330,11 +338,11 @@ impl Mountable for MountSpec {
 
         unsafe {
             if libc::syscall(libc::SYS_pivot_root, dot_p, dot_p) < 0 {
-                bail!("unable to pivot root");
+                bail!("failed to pivot_root: {}", io::Error::last_os_error());
             }
 
             if libc::umount2(dot_p, libc::MNT_DETACH) < 0 {
-                bail!("unable to unmount old root");
+                bail!("failed to unmount old root: {}", io::Error::last_os_error());
             }
         }
 

src/runner.rs

@@ -430,7 +430,10 @@ impl Runner {
             return Ok(code);
         }
 
-        Err(anyhow!("failed to launch/monitor child process"))
+        Err(anyhow!(
+            "styrolite runner '{}' did not exit normally: {status}",
+            self.executable
+        ))
     }
 
     #[cfg(feature = "async")]
@@ -443,7 +446,10 @@ impl Runner {
             return Ok(code);
         }
 
-        Err(anyhow!("failed to launch/monitor child process"))
+        Err(anyhow!(
+            "styrolite runner '{}' did not exit normally: {status}",
+            self.executable
+        ))
     }
 
     /// Replace the current process with the styrolite runner directly.
@@ -459,7 +465,10 @@ impl Runner {
         // That means config_file won't be dropped, so a drop-based cleanup won't happen.
         // If TempFile is delete-on-drop, the file may be left behind.
         let err = command.exec(); // only returns on failure
-        Err(anyhow!(err))
+        Err(anyhow!(
+            "failed to exec styrolite runner '{}': {err}",
+            self.executable
+        ))
     }
 
     #[cfg(not(unix))]

src/wrap.rs

@@ -53,7 +53,10 @@ fn set_process_limit(resource: RLimit, limit: Option<u64>) -> Result<()> {
 
     unsafe {
         if libc::setrlimit(resource, &rlimit) == -1 {
-            Err(anyhow!("failed to set resource limit"))
+            Err(anyhow!(
+                "failed to set resource limit {resource}: {}",
+                Error::last_os_error()
+            ))
         } else {
             Ok(())
         }
@@ -217,20 +220,22 @@ impl CreateRequest {
     }
 
     fn update_hostname(&self) -> Result<()> {
-        let wid = self
-            .identity()
-            .expect("unable to determine a workload identity");
+        let wid = self.identity()?;
         let final_hostname = match &self.hostname {
             Some(hostname) => hostname.to_string(),
             None => format!("styrolite-{wid}"),
         };
-        let final_hostname_cstr =
-            CString::new(final_hostname).expect("unable to parse hostname as valid C string");
+        let final_hostname_cstr = CString::new(final_hostname.clone()).map_err(|e| {
+            anyhow!("hostname '{final_hostname}' contains an interior NUL byte: {e}")
+        })?;
         let final_hostname_ptr = final_hostname_cstr.as_ptr();
 
         unsafe {
             if libc::sethostname(final_hostname_ptr, final_hostname_cstr.count_bytes()) < 0 {
-                Err(anyhow!("failed to set hostname"))
+                Err(anyhow!(
+                    "failed to set hostname to '{final_hostname}': {}",
+                    Error::last_os_error()
+                ))
             } else {
                 Ok(())
             }
@@ -679,7 +684,7 @@ impl ExecutableSpec {
         let executable = self
             .executable
             .clone()
-            .expect("expected executable to be configured");
+            .ok_or_else(|| anyhow!("no executable configured for the workload to run"))?;
 
         let program_cstring = CString::new(executable)?;
         let mut args_cstrings: Vec<_> = if let Some(args) = &self.arguments {
@@ -732,7 +737,31 @@ impl ExecutableSpec {
                 env_charptrs.as_ptr(),
             ) < 0
             {
-                Err(anyhow!("execvpe failed"))
+                // execvpe only returns on failure. Capture errno immediately
+                // (before any other libc call can clobber it) and translate it
+                // into an actionable message. "execvpe failed" with no detail
+                // has repeatedly sent people looking in the wrong place.
+                let err = Error::last_os_error();
+                let program = program_cstring.to_string_lossy();
+                let hint = match err.raw_os_error() {
+                    Some(libc::ENOENT) => format!(
+                        " (is '{program}' installed and on PATH? if you meant to \
+                         pass it as an argument, check the order of the executable \
+                         and its arguments)"
+                    ),
+                    Some(libc::EACCES) => {
+                        format!(
+                            " (is '{program}' marked executable, and are all \
+                                 leading path components accessible?)"
+                        )
+                    }
+                    Some(libc::ENOEXEC) => format!(
+                        " (is '{program}' a valid executable for this architecture, \
+                         or is it a script missing a #! interpreter line?)"
+                    ),
+                    _ => String::new(),
+                };
+                Err(anyhow!("failed to execute '{program}': {err}{hint}"))
             } else {
                 Ok(())
             }