diff --git a/crates/libcontainer/src/apparmor.rs b/crates/libcontainer/src/apparmor.rs index b6c9df155..b80982438 100644 --- a/crates/libcontainer/src/apparmor.rs +++ b/crates/libcontainer/src/apparmor.rs @@ -1,17 +1,36 @@ -use anyhow::{Context, Result}; +use crate::utils; use std::{ fs::{self}, path::Path, }; -use crate::utils; +#[derive(Debug, thiserror::Error)] +pub enum AppArmorError { + #[error("failed to apply AppArmor profile")] + ApplyProfile { + path: std::path::PathBuf, + profile: String, + // TODO: fix this after `utils` crate is migrated to `thiserror` + source: anyhow::Error, + }, + #[error("failed to read AppArmor profile: {source} {path}")] + ReadProfile { + path: String, + source: std::io::Error, + }, +} + +type Result = std::result::Result; const ENABLED_PARAMETER_PATH: &str = "/sys/module/apparmor/parameters/enabled"; /// Checks if AppArmor has been enabled on the system. pub fn is_enabled() -> Result { - let aa_enabled = fs::read_to_string(ENABLED_PARAMETER_PATH) - .with_context(|| format!("could not read {ENABLED_PARAMETER_PATH}"))?; + let aa_enabled = + fs::read_to_string(ENABLED_PARAMETER_PATH).map_err(|e| AppArmorError::ReadProfile { + path: ENABLED_PARAMETER_PATH.to_string(), + source: e, + })?; Ok(aa_enabled.starts_with('Y')) } @@ -32,6 +51,14 @@ pub fn apply_profile(profile: &str) -> Result<()> { } fn activate_profile(path: &Path, profile: &str) -> Result<()> { - utils::ensure_procfs(path)?; - utils::write_file(path, format!("exec {profile}")) + utils::ensure_procfs(path).map_err(|err| AppArmorError::ApplyProfile { + path: path.to_owned(), + profile: profile.to_owned(), + source: err, + })?; + utils::write_file(path, format!("exec {profile}")).map_err(|err| AppArmorError::ApplyProfile { + path: path.to_owned(), + profile: profile.to_owned(), + source: err, + }) } diff --git a/crates/libcontainer/src/config.rs b/crates/libcontainer/src/config.rs index c6c3377d5..c9e39eace 100644 --- a/crates/libcontainer/src/config.rs +++ b/crates/libcontainer/src/config.rs @@ -1,15 +1,39 @@ +use crate::utils; +use oci_spec::runtime::{Hooks, Spec}; +use serde::{Deserialize, Serialize}; use std::{ fs, io::{BufReader, BufWriter, Write}, path::{Path, PathBuf}, }; -use anyhow::{Context, Result}; -use serde::{Deserialize, Serialize}; - -use oci_spec::runtime::{Hooks, Spec}; +#[derive(Debug, thiserror::Error)] +pub enum ConfigError { + #[error("failed to save config")] + SaveIO { + source: std::io::Error, + path: PathBuf, + }, + #[error("failed to save config")] + SaveEncode { + source: serde_json::Error, + path: PathBuf, + }, + #[error("failed to parse config")] + LoadIO { + source: std::io::Error, + path: PathBuf, + }, + #[error("failed to parse config")] + LoadParse { + source: serde_json::Error, + path: PathBuf, + }, + #[error("missing linux in spec")] + MissingLinux, +} -use crate::utils; +type Result = std::result::Result; const YOUKI_CONFIG_NAME: &str = "youki_config.json"; @@ -29,7 +53,7 @@ impl<'a> YoukiConfig { cgroup_path: utils::get_cgroup_path( spec.linux() .as_ref() - .context("no linux in spec")? + .ok_or(ConfigError::MissingLinux)? .cgroups_path(), container_id, rootless, @@ -38,19 +62,37 @@ impl<'a> YoukiConfig { } pub fn save>(&self, path: P) -> Result<()> { - let file = fs::File::create(path.as_ref().join(YOUKI_CONFIG_NAME))?; + let file = fs::File::create(path.as_ref().join(YOUKI_CONFIG_NAME)).map_err(|err| { + ConfigError::SaveIO { + source: err, + path: path.as_ref().to_owned(), + } + })?; let mut writer = BufWriter::new(file); - serde_json::to_writer(&mut writer, self)?; - writer.flush()?; + serde_json::to_writer(&mut writer, self).map_err(|err| ConfigError::SaveEncode { + source: err, + path: path.as_ref().to_owned(), + })?; + writer.flush().map_err(|err| ConfigError::SaveIO { + source: err, + path: path.as_ref().to_owned(), + })?; + Ok(()) } pub fn load>(path: P) -> Result { let path = path.as_ref(); - let file = fs::File::open(path.join(YOUKI_CONFIG_NAME))?; + let file = + fs::File::open(path.join(YOUKI_CONFIG_NAME)).map_err(|err| ConfigError::LoadIO { + source: err, + path: path.to_owned(), + })?; let reader = BufReader::new(file); - let config = serde_json::from_reader(reader) - .with_context(|| format!("failed to load config from {path:?}"))?; + let config = serde_json::from_reader(reader).map_err(|err| ConfigError::LoadParse { + source: err, + path: path.to_owned(), + })?; Ok(config) } } diff --git a/crates/libcontainer/src/error.rs b/crates/libcontainer/src/error.rs new file mode 100644 index 000000000..04b9196c3 --- /dev/null +++ b/crates/libcontainer/src/error.rs @@ -0,0 +1,12 @@ +/// UnifiedSyscallError aims to simplify error handling of syscalls in +/// libcontainer. In many occasions, we mix nix::Error, std::io::Error and our +/// own syscall wrappers, which makes error handling complicated. +#[derive(Debug, thiserror::Error)] +pub enum UnifiedSyscallError { + #[error(transparent)] + Io(#[from] std::io::Error), + #[error(transparent)] + Nix(#[from] nix::Error), + #[error(transparent)] + Syscall(#[from] crate::syscall::SyscallError), +} diff --git a/crates/libcontainer/src/hooks.rs b/crates/libcontainer/src/hooks.rs index dd3aa351d..7bf69da02 100644 --- a/crates/libcontainer/src/hooks.rs +++ b/crates/libcontainer/src/hooks.rs @@ -1,29 +1,38 @@ -use anyhow::{bail, Context, Result}; use nix::{sys::signal, unistd::Pid}; use oci_spec::runtime::Hook; use std::{ - collections::HashMap, fmt, io::ErrorKind, io::Write, os::unix::prelude::CommandExt, process, + collections::HashMap, + io::ErrorKind, + io::Write, + os::unix::prelude::CommandExt, + process::{self}, thread, time, }; use crate::{container::Container, utils}; -// A special error used to signal a timeout. We want to differentiate between a -// timeout vs. other error. -#[derive(Debug)] -pub struct HookTimeoutError; -impl std::error::Error for HookTimeoutError {} -impl fmt::Display for HookTimeoutError { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - "hook command timeout".fmt(f) - } + +#[derive(Debug, thiserror::Error)] +pub enum HookError { + #[error("failed to execute hook command")] + CommandExecute(#[source] std::io::Error), + #[error("failed to encode container state")] + EncodeContainerState(#[source] serde_json::Error), + #[error("hook command exited with non-zero exit code: {0}")] + NonZeroExitCode(i32), + #[error("hook command was killed by a signal")] + Killed, + #[error("failed to execute hook command due to a timeout")] + Timeout, + #[error("container state is required to run hook")] + MissingContainerState, + #[error("failed to write container state to stdin")] + WriteContainerState(#[source] std::io::Error), } -pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Result<()> { - if container.is_none() { - bail!("container state is required to run hook"); - } +type Result = std::result::Result; - let state = &container.unwrap().state; +pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Result<()> { + let state = &(container.ok_or(HookError::MissingContainerState)?.state); if let Some(hooks) = hooks { for hook in hooks { @@ -53,7 +62,7 @@ pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Re .envs(envs) .stdin(process::Stdio::piped()) .spawn() - .with_context(|| "Failed to execute hook")?; + .map_err(HookError::CommandExecute)?; let hook_process_pid = Pid::from_raw(hook_process.id() as i32); // Based on the OCI spec, we need to pipe the container state into // the hook command through stdin. @@ -67,13 +76,13 @@ pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Re // error, in the case that the hook command is waiting for us to // write to stdin. let encoded_state = - serde_json::to_string(state).context("failed to encode container state")?; + serde_json::to_string(state).map_err(HookError::EncodeContainerState)?; if let Err(e) = stdin.write_all(encoded_state.as_bytes()) { if e.kind() != ErrorKind::BrokenPipe { // Not a broken pipe. The hook command may be waiting // for us. let _ = signal::kill(hook_process_pid, signal::Signal::SIGKILL); - bail!("failed to write container state to stdin: {:?}", e); + return Err(HookError::WriteContainerState(e)); } } } @@ -100,7 +109,7 @@ pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Re // Kill the process. There is no need to further clean // up because we will be error out. let _ = signal::kill(hook_process_pid, signal::Signal::SIGKILL); - return Err(HookTimeoutError.into()); + return Err(HookError::Timeout); } Err(_) => { unreachable!(); @@ -112,21 +121,12 @@ pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Re match res { Ok(exit_status) => match exit_status.code() { - Some(0) => {} - Some(exit_code) => { - bail!( - "Failed to execute hook command. Non-zero return code. {:?}", - exit_code - ); - } - None => { - bail!("Process is killed by signal"); - } + Some(0) => Ok(()), + Some(exit_code) => Err(HookError::NonZeroExitCode(exit_code)), + None => Err(HookError::Killed), }, - Err(e) => { - bail!("Failed to execute hook command: {:?}", e); - } - } + Err(e) => Err(HookError::CommandExecute(e)), + }?; } } @@ -136,7 +136,7 @@ pub fn run_hooks(hooks: Option<&Vec>, container: Option<&Container>) -> Re #[cfg(test)] mod test { use super::*; - use anyhow::{bail, Result}; + use anyhow::{bail, Context, Result}; use oci_spec::runtime::HookBuilder; use serial_test::serial; use std::{env, fs}; @@ -221,14 +221,14 @@ mod test { Ok(_) => { bail!("The test expects the hook to error out with timeout. Should not execute cleanly"); } + Err(HookError::Timeout) => {} Err(err) => { - // We want to make sure the error returned is indeed timeout - // error. All other errors are considered failure. - if !err.is::() { - bail!("Failed to execute hook: {:?}", err); - } + bail!( + "The test expects the hook to error out with timeout. Got error: {}", + err + ); } - } + }; Ok(()) } diff --git a/crates/libcontainer/src/lib.rs b/crates/libcontainer/src/lib.rs index 28a2402e3..c335e6806 100644 --- a/crates/libcontainer/src/lib.rs +++ b/crates/libcontainer/src/lib.rs @@ -2,6 +2,7 @@ pub mod apparmor; pub mod capabilities; pub mod config; pub mod container; +pub mod error; pub mod hooks; pub mod namespaces; pub mod notify_socket; diff --git a/crates/libcontainer/src/namespaces.rs b/crates/libcontainer/src/namespaces.rs index 3d5495de0..12f82dc10 100644 --- a/crates/libcontainer/src/namespaces.rs +++ b/crates/libcontainer/src/namespaces.rs @@ -7,7 +7,8 @@ //! UTS (hostname and domain information, processes will think they're running on servers with different names), //! Cgroup (Resource limits, execution priority etc.) -use crate::syscall::{syscall::create_syscall, Syscall, SyscallError}; +use crate::error::UnifiedSyscallError; +use crate::syscall::{syscall::create_syscall, Syscall}; use nix::{fcntl, sched::CloneFlags, sys::stat, unistd}; use oci_spec::runtime::{LinuxNamespace, LinuxNamespaceType}; use std::collections; @@ -20,13 +21,7 @@ pub enum NamespaceError { ApplyNamespaceSyscallFailed { namespace: Box, #[source] - err: SyscallError, - }, - #[error("failed to set namespace")] - ApplyNamespaceUnixSyscallFailed { - namespace: Box, - #[source] - err: nix::Error, + err: UnifiedSyscallError, }, } @@ -93,22 +88,20 @@ impl Namespaces { match namespace.path() { Some(path) => { let fd = fcntl::open(path, fcntl::OFlag::empty(), stat::Mode::empty()).map_err( - |err| NamespaceError::ApplyNamespaceUnixSyscallFailed { + |err| NamespaceError::ApplyNamespaceSyscallFailed { namespace: Box::new(namespace.to_owned()), - err, + err: err.into(), }, )?; self.command .set_ns(fd, get_clone_flag(namespace.typ())) .map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed { namespace: Box::new(namespace.to_owned()), - err, + err: err.into(), })?; - unistd::close(fd).map_err(|err| { - NamespaceError::ApplyNamespaceUnixSyscallFailed { - namespace: Box::new(namespace.to_owned()), - err, - } + unistd::close(fd).map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed { + namespace: Box::new(namespace.to_owned()), + err: err.into(), })?; } None => { @@ -116,7 +109,7 @@ impl Namespaces { .unshare(get_clone_flag(namespace.typ())) .map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed { namespace: Box::new(namespace.to_owned()), - err, + err: err.into(), })?; } } diff --git a/crates/libcontainer/src/notify_socket.rs b/crates/libcontainer/src/notify_socket.rs index b2083fed0..a97e0ccac 100644 --- a/crates/libcontainer/src/notify_socket.rs +++ b/crates/libcontainer/src/notify_socket.rs @@ -1,4 +1,3 @@ -use anyhow::{bail, Context, Result}; use nix::unistd::{self, close}; use std::env; use std::io::prelude::*; @@ -8,29 +7,67 @@ use std::path::{Path, PathBuf}; pub const NOTIFY_FILE: &str = "notify.sock"; +#[derive(Debug, thiserror::Error)] +pub enum NotifyListenerError { + #[error("failed to chdir while creating notify socket")] + Chdir { source: nix::Error, path: PathBuf }, + #[error("invalid path: {0}")] + InvalidPath(PathBuf), + #[error("failed to bind notify socket: {name}")] + Bind { + source: std::io::Error, + name: String, + }, + #[error("failed to connect to notify socket: {name}")] + Connect { + source: std::io::Error, + name: String, + }, + #[error("failed to get cwd")] + GetCwd(#[source] std::io::Error), + #[error("failed to accept notify listener")] + Accept(#[source] std::io::Error), + #[error("failed to close notify listener")] + Close(#[source] nix::errno::Errno), + #[error("failed to read notify listener")] + Read(#[source] std::io::Error), + #[error("failed to send start container")] + SendStartContainer(#[source] std::io::Error), +} + +type Result = std::result::Result; + pub struct NotifyListener { socket: UnixListener, } impl NotifyListener { pub fn new(socket_path: &Path) -> Result { - // unix domain socket has a maximum length of 108, different from - // normal path length of 255. due to how docker create the path name + // Unix domain socket has a maximum length of 108, different from + // normal path length of 255. Due to how docker create the path name // to the container working directory, there is a high chance that - // the full absolute path is over the limit. to work around this + // the full absolute path is over the limit. To work around this // limitation, we chdir first into the workdir where the socket is, // and chdir back after the socket is created. - let workdir = socket_path.parent().unwrap(); - let socket_name = socket_path.file_name().unwrap(); - let cwd = unistd::getcwd().context("failed to get cwd")?; - unistd::chdir(workdir).context(format!( - "failed to chdir into {}", - workdir.to_str().unwrap() - ))?; - let stream = UnixListener::bind(socket_name) - .context(format!("failed to bind {}", socket_name.to_str().unwrap()))?; - unistd::chdir(&cwd) - .context(format!("failed to chdir back to {}", cwd.to_str().unwrap()))?; + let workdir = socket_path + .parent() + .ok_or_else(|| NotifyListenerError::InvalidPath(socket_path.to_owned()))?; + let socket_name = socket_path + .file_name() + .ok_or_else(|| NotifyListenerError::InvalidPath(socket_path.to_owned()))?; + let cwd = env::current_dir().map_err(NotifyListenerError::GetCwd)?; + unistd::chdir(workdir).map_err(|e| NotifyListenerError::Chdir { + source: e, + path: workdir.to_owned(), + })?; + let stream = UnixListener::bind(socket_name).map_err(|e| NotifyListenerError::Bind { + source: e, + name: socket_name.to_str().unwrap().to_owned(), + })?; + unistd::chdir(&cwd).map_err(|e| NotifyListenerError::Chdir { + source: e, + path: cwd, + })?; Ok(Self { socket: stream }) } @@ -39,17 +76,19 @@ impl NotifyListener { match self.socket.accept() { Ok((mut socket, _)) => { let mut response = String::new(); - socket.read_to_string(&mut response)?; + socket + .read_to_string(&mut response) + .map_err(NotifyListenerError::Read)?; log::debug!("received: {}", response); } - Err(e) => bail!("accept function failed: {:?}", e), + Err(e) => Err(NotifyListenerError::Accept(e))?, } Ok(()) } pub fn close(&self) -> Result<()> { - close(self.socket.as_raw_fd())?; + close(self.socket.as_raw_fd()).map_err(NotifyListenerError::Close)?; Ok(()) } } @@ -67,12 +106,32 @@ impl NotifySocket { pub fn notify_container_start(&mut self) -> Result<()> { log::debug!("notify container start"); - let cwd = env::current_dir()?; - unistd::chdir(self.path.parent().unwrap())?; - let mut stream = UnixStream::connect(self.path.file_name().unwrap())?; - stream.write_all(b"start container")?; + let cwd = env::current_dir().map_err(NotifyListenerError::GetCwd)?; + let workdir = self + .path + .parent() + .ok_or_else(|| NotifyListenerError::InvalidPath(self.path.to_owned()))?; + unistd::chdir(workdir).map_err(|e| NotifyListenerError::Chdir { + source: e, + path: workdir.to_owned(), + })?; + let socket_name = self + .path + .file_name() + .ok_or_else(|| NotifyListenerError::InvalidPath(self.path.to_owned()))?; + let mut stream = + UnixStream::connect(socket_name).map_err(|e| NotifyListenerError::Connect { + source: e, + name: socket_name.to_str().unwrap().to_owned(), + })?; + stream + .write_all(b"start container") + .map_err(NotifyListenerError::SendStartContainer)?; log::debug!("notify finished"); - unistd::chdir(&cwd)?; + unistd::chdir(&cwd).map_err(|e| NotifyListenerError::Chdir { + source: e, + path: cwd, + })?; Ok(()) } } diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index dd6b210fb..13703e3b7 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -54,7 +54,7 @@ fn readonly_path(path: &Path, syscall: &dyn Syscall) -> Result<()> { None, ) { match err { - SyscallError::MountFailed { errno, .. } => { + SyscallError::Mount { source: errno } => { // ignore error if path is not exist. if matches!(errno, nix::errno::Errno::ENOENT) { return Ok(()); @@ -92,7 +92,7 @@ fn masked_path(path: &Path, mount_label: &Option, syscall: &dyn Syscall) None, ) { match err { - SyscallError::MountFailed { errno, .. } => match errno { + SyscallError::Mount { source: errno } => match errno { nix::errno::Errno::ENOENT => { log::warn!("masked path {:?} not exist", path); } @@ -740,13 +740,8 @@ mod tests { .downcast_ref::() .unwrap(); mocks.set_ret_err(ArgName::Mount, || { - Err(SyscallError::MountFailed { - mount_source: None, - mount_target: PathBuf::new(), - fstype: None, - flags: MsFlags::empty(), - data: None, - errno: nix::errno::Errno::ENOENT, + Err(SyscallError::Mount { + source: nix::errno::Errno::ENOENT, }) }); @@ -763,13 +758,8 @@ mod tests { .downcast_ref::() .unwrap(); mocks.set_ret_err(ArgName::Mount, || { - Err(SyscallError::MountFailed { - mount_source: None, - mount_target: PathBuf::new(), - fstype: None, - flags: MsFlags::empty(), - data: None, - errno: nix::errno::Errno::ENOTDIR, + Err(SyscallError::Mount { + source: nix::errno::Errno::ENOTDIR, }) }); @@ -795,13 +785,8 @@ mod tests { .downcast_ref::() .unwrap(); mocks.set_ret_err(ArgName::Mount, || { - Err(SyscallError::MountFailed { - mount_source: None, - mount_target: PathBuf::new(), - fstype: None, - flags: MsFlags::empty(), - data: None, - errno: nix::errno::Errno::ENOTDIR, + Err(SyscallError::Mount { + source: nix::errno::Errno::ENOTDIR, }) }); @@ -832,13 +817,8 @@ mod tests { .downcast_ref::() .unwrap(); mocks.set_ret_err(ArgName::Mount, || { - Err(SyscallError::MountFailed { - mount_source: None, - mount_target: PathBuf::new(), - fstype: None, - flags: MsFlags::empty(), - data: None, - errno: nix::errno::Errno::UnknownErrno, + Err(SyscallError::Mount { + source: nix::errno::Errno::UnknownErrno, }) }); diff --git a/crates/libcontainer/src/rootfs/mount.rs b/crates/libcontainer/src/rootfs/mount.rs index 418baee89..35ac2a7a1 100644 --- a/crates/libcontainer/src/rootfs/mount.rs +++ b/crates/libcontainer/src/rootfs/mount.rs @@ -419,7 +419,7 @@ impl Mount { self.syscall .mount(Some(&*src), dest, typ, mount_option_config.flags, Some(&*d)) { - if let SyscallError::MountFailed { errno, .. } = err { + if let SyscallError::Mount { source: errno, .. } = err { if !matches!(errno, Errno::EINVAL) { bail!("mount of {:?} failed. {}", m.destination(), errno); } diff --git a/crates/libcontainer/src/signal.rs b/crates/libcontainer/src/signal.rs index f7134715a..547098600 100644 --- a/crates/libcontainer/src/signal.rs +++ b/crates/libcontainer/src/signal.rs @@ -1,6 +1,5 @@ //! Returns *nix signal enum value from passed string -use anyhow::{bail, Context, Result}; use nix::sys::signal::Signal as NixSignal; use std::convert::TryFrom; @@ -8,11 +7,18 @@ use std::convert::TryFrom; #[derive(Debug)] pub struct Signal(NixSignal); +#[derive(Debug, thiserror::Error)] +pub enum SignalError { + #[error("invalid signal: {0}")] + InvalidSignal(T), +} + impl TryFrom<&str> for Signal { - type Error = anyhow::Error; + type Error = SignalError; fn try_from(s: &str) -> Result { use NixSignal::*; + Ok(match s.to_ascii_uppercase().as_str() { "1" | "HUP" | "SIGHUP" => SIGHUP, "2" | "INT" | "SIGINT" => SIGINT, @@ -45,18 +51,18 @@ impl TryFrom<&str> for Signal { "29" | "IO" | "SIGIO" => SIGIO, "30" | "PWR" | "SIGPWR" => SIGPWR, "31" | "SYS" | "SIGSYS" => SIGSYS, - _ => bail! {"{} is not a valid signal", s}, + _ => return Err(SignalError::InvalidSignal(s.to_string())), }) .map(Signal) } } impl TryFrom for Signal { - type Error = anyhow::Error; + type Error = SignalError; fn try_from(value: i32) -> Result { NixSignal::try_from(value) - .with_context(|| format!("{value} is not a valid signal")) + .map_err(|_| SignalError::InvalidSignal(value)) .map(Signal) } } diff --git a/crates/libcontainer/src/syscall/linux.rs b/crates/libcontainer/src/syscall/linux.rs index c9f07ed2e..7665a6e8a 100644 --- a/crates/libcontainer/src/syscall/linux.rs +++ b/crates/libcontainer/src/syscall/linux.rs @@ -216,7 +216,7 @@ impl LinuxSyscall { .map_err(|_| SyscallError::NotProcfs(PROCFS_FD_PATH.to_string()))?; let fds: Vec = fs::read_dir(PROCFS_FD_PATH) - .map_err(SyscallError::GetOpenFdsFailed)? + .map_err(SyscallError::GetOpenFds)? .filter_map(|entry| match entry { Ok(entry) => Some(entry.path()), Err(_) => None, @@ -250,11 +250,8 @@ impl Syscall for LinuxSyscall { // open the path as directory and read only let newroot = open(path, OFlag::O_DIRECTORY | OFlag::O_RDONLY, Mode::empty()).map_err(|errno| { - SyscallError::PivotRootFailed { - errno, - msg: format!("failed to open {:?}", path), - path: format!("{:?}", path), - } + log::error!("failed to open {:?}", path); + SyscallError::PivotRoot { source: errno } })?; // make the given path as the root directory for the container @@ -265,10 +262,9 @@ impl Syscall for LinuxSyscall { // this path. This is done, as otherwise, we will need to create a separate temporary directory under the new root path // so we can move the original root there, and then unmount that. This way saves the creation of the temporary // directory to put original root directory. - pivot_root(path, path).map_err(|errno| SyscallError::PivotRootFailed { - errno, - msg: format!("failed to pivot root to {:?}", path), - path: format!("{:?}", path), + pivot_root(path, path).map_err(|errno| { + log::error!("failed to pivot root to {:?}", path); + SyscallError::PivotRoot { source: errno } })?; // Make the original root directory rslave to avoid propagating unmount event to the host mount namespace. @@ -280,26 +276,23 @@ impl Syscall for LinuxSyscall { MsFlags::MS_SLAVE | MsFlags::MS_REC, None::<&str>, ) - .map_err(|errno| SyscallError::PivotRootFailed { - errno, - msg: "failed to make root directory rslave".to_string(), - path: format!("{:?}", path), + .map_err(|errno| { + log::error!("failed to make original root directory rslave"); + SyscallError::PivotRoot { source: errno } })?; // Unmount the original root directory which was stacked on top of new root directory // MNT_DETACH makes the mount point unavailable to new accesses, but waits till the original mount point // to be free of activity to actually unmount // see https://man7.org/linux/man-pages/man2/umount2.2.html for more information - umount2("/", MntFlags::MNT_DETACH).map_err(|errno| SyscallError::PivotRootFailed { - errno, - msg: "failed to unmount old root directory".to_string(), - path: format!("{:?}", path), + umount2("/", MntFlags::MNT_DETACH).map_err(|errno| { + log::error!("failed to unmount old root directory"); + SyscallError::PivotRoot { source: errno } })?; // Change directory to the new root - fchdir(newroot).map_err(|errno| SyscallError::PivotRootFailed { - errno, - msg: "failed to change directory to new root".to_string(), - path: format!("{:?}", path), + fchdir(newroot).map_err(|errno| { + log::error!("failed to change directory to new root"); + SyscallError::PivotRoot { source: errno } })?; Ok(()) @@ -307,23 +300,23 @@ impl Syscall for LinuxSyscall { /// Set namespace for process fn set_ns(&self, rawfd: i32, nstype: CloneFlags) -> Result<()> { - nix::sched::setns(rawfd, nstype).map_err(SyscallError::SetNamespaceFailed)?; + nix::sched::setns(rawfd, nstype).map_err(SyscallError::SetNamespace)?; Ok(()) } /// set uid and gid for process fn set_id(&self, uid: Uid, gid: Gid) -> Result<()> { prctl::set_keep_capabilities(true).map_err(|errno| { - SyscallError::PrctlSetKeepCapabilitesFailed { + SyscallError::PrctlSetKeepCapabilites { errno: nix::errno::from_i32(errno), value: true, } })?; // args : real *id, effective *id, saved set *id respectively unistd::setresgid(gid, gid, gid) - .map_err(|errno| SyscallError::SetRealGidFailed { errno, gid })?; + .map_err(|errno| SyscallError::SetRealGid { errno, gid })?; unistd::setresuid(uid, uid, uid) - .map_err(|errno| SyscallError::SetRealUidFailed { errno, uid })?; + .map_err(|errno| SyscallError::SetRealUid { errno, uid })?; // if not the root user, reset capabilities to effective capabilities, // which are used by kernel to perform checks @@ -332,7 +325,7 @@ impl Syscall for LinuxSyscall { capabilities::reset_effective(self)?; } prctl::set_keep_capabilities(false).map_err(|errno| { - SyscallError::PrctlSetKeepCapabilitesFailed { + SyscallError::PrctlSetKeepCapabilites { errno: nix::errno::from_i32(errno), value: false, } @@ -343,7 +336,7 @@ impl Syscall for LinuxSyscall { /// Disassociate parts of execution context // see https://man7.org/linux/man-pages/man2/unshare.2.html for more information fn unshare(&self, flags: CloneFlags) -> Result<()> { - unshare(flags).map_err(SyscallError::UnshareFailed) + unshare(flags).map_err(SyscallError::Unshare) } /// Set capabilities for container process @@ -371,7 +364,7 @@ impl Syscall for LinuxSyscall { /// Sets hostname for process fn set_hostname(&self, hostname: &str) -> Result<()> { - sethostname(hostname).map_err(|errno| SyscallError::SetHostnameFailed { + sethostname(hostname).map_err(|errno| SyscallError::SetHostname { errno, hostname: hostname.to_string(), }) @@ -385,12 +378,12 @@ impl Syscall for LinuxSyscall { let res = unsafe { setdomainname(ptr, len) }; match res { 0 => Ok(()), - -1 => Err(SyscallError::SetDomainnameFailed { + -1 => Err(SyscallError::SetDomainname { errno: nix::errno::Errno::last(), domainname: domainname.to_string(), }), - _ => Err(SyscallError::SetDomainnameFailed { + _ => Err(SyscallError::SetDomainname { errno: nix::errno::Errno::UnknownErrno, domainname: domainname.to_string(), }), @@ -412,7 +405,7 @@ impl Syscall for LinuxSyscall { Errno::result(res) .map(drop) - .map_err(|errno| SyscallError::SetRlimitFailed { + .map_err(|errno| SyscallError::SetRlimit { errno, rlimit: rlimit.typ(), })?; @@ -454,10 +447,7 @@ impl Syscall for LinuxSyscall { } fn chroot(&self, path: &Path) -> Result<()> { - unistd::chroot(path).map_err(|errno| SyscallError::ChrootFailed { - errno, - path: path.to_path_buf(), - })?; + unistd::chroot(path).map_err(|errno| SyscallError::Chroot { source: errno })?; Ok(()) } @@ -470,48 +460,35 @@ impl Syscall for LinuxSyscall { flags: MsFlags, data: Option<&str>, ) -> Result<()> { - mount(source, target, fstype, flags, data).map_err(|errno| SyscallError::MountFailed { - errno, - mount_source: source.map(|p| p.to_path_buf()), - mount_target: target.to_path_buf(), - fstype: fstype.map(|s| s.to_string()), - flags, - data: data.map(|s| s.to_string()), - }) + mount(source, target, fstype, flags, data) + .map_err(|errno| SyscallError::Mount { source: errno }) } fn symlink(&self, original: &Path, link: &Path) -> Result<()> { - symlink(original, link).map_err(|err| SyscallError::SymlinkFailed { - err, - old_path: original.to_path_buf(), - new_path: link.to_path_buf(), + symlink(original, link).map_err(|err| { + log::error!("failed to create symlink from {original:?} to {link:?}: {err}"); + SyscallError::Symlink { source: err } }) } fn mknod(&self, path: &Path, kind: SFlag, perm: Mode, dev: u64) -> Result<()> { - mknod(path, kind, perm, dev).map_err(|errno| SyscallError::MknodFailed { - errno, - path: path.to_path_buf(), - kind, - perm, - dev, + mknod(path, kind, perm, dev).map_err(|errno| { + log::error!( + "failed to mknod {path:?}, kind {kind:?}, perm {perm:?}, dev {dev:?}: {errno}" + ); + SyscallError::Mknod { source: errno } }) } fn chown(&self, path: &Path, owner: Option, group: Option) -> Result<()> { - chown(path, owner, group).map_err(|errno| SyscallError::ChownFailed { - errno, - path: path.to_path_buf(), - owner, - group, + chown(path, owner, group).map_err(|errno| { + log::error!("failed to chown {path:?} to {owner:?}:{group:?}: {errno}"); + SyscallError::Chown { source: errno } }) } fn set_groups(&self, groups: &[Gid]) -> Result<()> { - setgroups(groups).map_err(|errno| SyscallError::SetGroupsFailed { - groups: groups.to_vec(), - errno, - }) + setgroups(groups).map_err(|errno| SyscallError::SetGroups { source: errno }) } fn close_range(&self, preserve_fds: i32) -> Result<()> { @@ -530,7 +507,7 @@ impl Syscall for LinuxSyscall { // kernel 5.11. If the kernel is older we emulate close_range in userspace. Self::emulate_close_range(preserve_fds) } - Err(e) => Err(SyscallError::CloseRangeFailed { + Err(e) => Err(SyscallError::CloseRange { preserve_fds, errno: e, }), @@ -568,11 +545,7 @@ impl Syscall for LinuxSyscall { match result { Ok(_) => Ok(()), - Err(e) => Err(SyscallError::MountSetattrFailed { - pathname: pathname.to_path_buf(), - flags, - errno: e, - }), + Err(e) => Err(SyscallError::MountSetattr { source: e }), } } } diff --git a/crates/libcontainer/src/syscall/mod.rs b/crates/libcontainer/src/syscall/mod.rs index 238fdac4f..e83f27b23 100644 --- a/crates/libcontainer/src/syscall/mod.rs +++ b/crates/libcontainer/src/syscall/mod.rs @@ -12,105 +12,76 @@ pub use syscall::Syscall; pub enum SyscallError { #[error("unexpected mount attr option: {0}")] UnexpectedMountAttrOption(String), - #[error("set keep capabilities to {value} returned {errno}")] - PrctlSetKeepCapabilitesFailed { + #[error("set keep capabilities to {value}")] + PrctlSetKeepCapabilites { + #[source] errno: nix::errno::Errno, value: bool, }, - #[error("set hostname to {hostname} returned {errno}")] - SetHostnameFailed { + #[error("set hostname to {hostname}")] + SetHostname { + #[source] errno: nix::errno::Errno, hostname: String, }, - #[error("set domainname to {domainname} returned {errno}")] - SetDomainnameFailed { + #[error("set domainname to {domainname}")] + SetDomainname { + #[source] errno: nix::errno::Errno, domainname: String, }, #[error("{0} is not an actual procfs")] NotProcfs(String), - #[error("failed to get open fds: {0}")] - GetOpenFdsFailed(std::io::Error), + #[error("failed to get open fds")] + GetOpenFds(#[source] std::io::Error), #[error("failed to pivot root")] - PivotRootFailed { - path: String, - msg: String, - errno: nix::errno::Errno, - }, + PivotRoot { source: nix::errno::Errno }, #[error("failed to setns: {0}")] - SetNamespaceFailed(nix::errno::Errno), - #[error("failed to set real gid to {gid}: {errno}")] - SetRealGidFailed { + SetNamespace(nix::errno::Errno), + #[error("failed to set real gid to {gid}")] + SetRealGid { + #[source] errno: nix::errno::Errno, gid: nix::unistd::Gid, }, #[error("failed to set real uid to {uid}: {errno}")] - SetRealUidFailed { + SetRealUid { + #[source] errno: nix::errno::Errno, uid: nix::unistd::Uid, }, #[error("failed to unshare: {0}")] - UnshareFailed(nix::errno::Errno), + Unshare(nix::errno::Errno), #[error("failed to set capabilities: {0}")] - SetCapsFailed(#[from] caps::errors::CapsError), - #[error("failed to set rlimit {rlimit:?}: {errno}")] - SetRlimitFailed { + SetCaps(#[from] caps::errors::CapsError), + #[error("failed to set rlimit {rlimit:?}")] + SetRlimit { + #[source] errno: nix::errno::Errno, rlimit: oci_spec::runtime::LinuxRlimitType, }, - #[error("failed to chroot to {path:?}: {errno}")] - ChrootFailed { - path: std::path::PathBuf, - errno: nix::errno::Errno, - }, + #[error("failed to chroot: {source}")] + Chroot { source: nix::errno::Errno }, #[error("mount failed")] - MountFailed { - mount_source: Option, - mount_target: std::path::PathBuf, - fstype: Option, - flags: nix::mount::MsFlags, - data: Option, - errno: nix::errno::Errno, - }, + Mount { source: nix::errno::Errno }, #[error("symlink failed")] - SymlinkFailed { - old_path: std::path::PathBuf, - new_path: std::path::PathBuf, - err: std::io::Error, - }, + Symlink { source: std::io::Error }, #[error("mknod failed")] - MknodFailed { - path: std::path::PathBuf, - kind: nix::sys::stat::SFlag, - perm: nix::sys::stat::Mode, - dev: nix::sys::stat::dev_t, - errno: nix::errno::Errno, - }, + Mknod { source: nix::errno::Errno }, #[error("chown failed")] - ChownFailed { - path: std::path::PathBuf, - owner: Option, - group: Option, - errno: nix::errno::Errno, - }, + Chown { source: nix::errno::Errno }, #[error("setgroups failed")] - SetGroupsFailed { - groups: Vec, - errno: nix::errno::Errno, - }, + SetGroups { source: nix::errno::Errno }, #[error("close range failed")] - CloseRangeFailed { + CloseRange { preserve_fds: i32, + #[source] errno: syscalls::Errno, }, #[error("invalid filename: {0:?}")] InvalidFilename(std::path::PathBuf), #[error("mount_setattr failed")] - MountSetattrFailed { - pathname: std::path::PathBuf, - flags: u32, - errno: syscalls::Errno, - }, + MountSetattr { source: syscalls::Errno }, } type Result = std::result::Result;