diff --git a/crates/libcgroups/src/common.rs b/crates/libcgroups/src/common.rs index 4052944f4..8da8c575c 100644 --- a/crates/libcgroups/src/common.rs +++ b/crates/libcgroups/src/common.rs @@ -27,14 +27,16 @@ pub const DEFAULT_CGROUP_ROOT: &str = "/sys/fs/cgroup"; #[cfg(feature = "systemd")] #[inline] -fn is_true_root() -> bool { +fn is_true_root() -> Result { if !nix::unistd::geteuid().is_root() { - return false; + return Ok(false); } let uid_map_path = "/proc/self/uid_map"; - let content = std::fs::read_to_string(uid_map_path) - .unwrap_or_else(|_| panic!("failed to read {}", uid_map_path)); - content.contains("4294967295") + let content = std::fs::read_to_string(uid_map_path).map_err(|e| WrappedIoError::Read { + err: e, + path: uid_map_path.into(), + })?; + Ok(content.contains("4294967295")) } pub trait CgroupManager { type Error; @@ -423,7 +425,7 @@ fn create_systemd_cgroup_manager( ); } - let use_system = is_true_root(); + let use_system = is_true_root().map_err(systemd::manager::SystemdManagerError::WrappedIo)?; tracing::info!( "systemd cgroup manager with system bus {} will be used", diff --git a/crates/libcgroups/src/stats.rs b/crates/libcgroups/src/stats.rs index de9491eb5..34edbd745 100644 --- a/crates/libcgroups/src/stats.rs +++ b/crates/libcgroups/src/stats.rs @@ -216,6 +216,8 @@ pub fn supported_page_sizes() -> Result, SupportedPageSizesError> { } let dir_name = hugetlb_entry.file_name(); + // this name should always be valid utf-8, + // so can unwrap without any checks let dir_name = dir_name.to_str().unwrap(); sizes.push(extract_page_size(dir_name)?); diff --git a/crates/libcgroups/src/systemd/dbus_native/dbus.rs b/crates/libcgroups/src/systemd/dbus_native/dbus.rs index 3f5a46c72..2aed47baf 100644 --- a/crates/libcgroups/src/systemd/dbus_native/dbus.rs +++ b/crates/libcgroups/src/systemd/dbus_native/dbus.rs @@ -46,16 +46,14 @@ fn parse_dbus_address(env_value: String) -> Result { // as per spec, the env var can have multiple addresses separated by ; let addr_list: Vec<_> = env_value.split(';').collect(); for addr in addr_list { - if addr.starts_with("unix:path=") { - let s = addr.strip_prefix("unix:path=").unwrap(); + if let Some(s) = addr.strip_prefix("unix:path=") { if !std::path::PathBuf::from(s).exists() { continue; } return Ok(s.to_owned()); } - if addr.starts_with("unix:abstract=") { - let s = addr.strip_prefix("unix:abstract=").unwrap(); + if let Some(s) = addr.strip_prefix("unix:abstract=") { return Ok(s.to_owned()); } } @@ -105,12 +103,19 @@ fn get_actual_uid() -> Result { .stdin(std::process::Stdio::null()) .stdout(std::process::Stdio::piped()) .spawn() - .map_err(|e| DbusError::BusAddressError(format!("error in running busctl {:?}", e)))? + .map_err(|e| DbusError::BusctlError(format!("error in running busctl {:?}", e)))? .wait_with_output() - .map_err(|e| DbusError::BusAddressError(format!("error in busctl {:?}", e)))?; + .map_err(|e| DbusError::BusctlError(format!("error from busctl execution {:?}", e)))?; let stdout = String::from_utf8_lossy(&output.stdout); - let found = stdout.lines().find(|s| s.starts_with("OwnerUID=")).unwrap(); + let found = + stdout + .lines() + .find(|s| s.starts_with("OwnerUID=")) + .ok_or(DbusError::BusctlError( + "could not find OwnerUID from busctl".into(), + ))?; + let uid = found .trim_start_matches("OwnerUID=") .parse::() diff --git a/crates/libcgroups/src/systemd/dbus_native/message.rs b/crates/libcgroups/src/systemd/dbus_native/message.rs index b31714cf9..79331c670 100644 --- a/crates/libcgroups/src/systemd/dbus_native/message.rs +++ b/crates/libcgroups/src/systemd/dbus_native/message.rs @@ -188,7 +188,7 @@ impl Header { .into()); } let ret = HeaderValue::U32(u32::from_le_bytes( - buf[*ctr..*ctr + 4].try_into().unwrap(), // we ca unwrap here as we know 4 byte buffer will satisfy [u8;4] + buf[*ctr..*ctr + 4].try_into().unwrap(), // we can unwrap here as we know 4 byte buffer will satisfy [u8;4] )); *ctr += 4; ret diff --git a/crates/libcgroups/src/systemd/dbus_native/utils.rs b/crates/libcgroups/src/systemd/dbus_native/utils.rs index 9d37aefd2..53402e484 100644 --- a/crates/libcgroups/src/systemd/dbus_native/utils.rs +++ b/crates/libcgroups/src/systemd/dbus_native/utils.rs @@ -40,6 +40,8 @@ pub enum DbusError { MethodCallErr(String), #[error("dbus bus address error: {0}")] BusAddressError(String), + #[error("dbus busctl error")] + BusctlError(String), #[error("could not parse uid from busctl: {0}")] UidError(ParseIntError), } diff --git a/crates/libcgroups/src/v1/manager.rs b/crates/libcgroups/src/v1/manager.rs index 78f95531f..45b46c025 100644 --- a/crates/libcgroups/src/v1/manager.rs +++ b/crates/libcgroups/src/v1/manager.rs @@ -105,7 +105,7 @@ impl Manager { .cgroups()? .into_iter() .find(|c| c.controllers.contains(&subsystem.to_string())) - .unwrap(); + .ok_or(V1ManagerError::SubsystemDoesNotExist)?; let p = if cgroup_path.as_os_str().is_empty() { mount_point.join_safely(Path::new(&cgroup.pathname))? @@ -246,7 +246,9 @@ impl CgroupManager for Manager { }; Ok(Freezer::apply( &controller_opt, - self.subsystems.get(&CtrlType::Freezer).unwrap(), + self.subsystems + .get(&CtrlType::Freezer) + .ok_or(V1ManagerError::SubsystemDoesNotExist)?, )?) } diff --git a/crates/libcgroups/src/v2/io.rs b/crates/libcgroups/src/v2/io.rs index 8266af62a..29908d6f6 100644 --- a/crates/libcgroups/src/v2/io.rs +++ b/crates/libcgroups/src/v2/io.rs @@ -127,10 +127,12 @@ impl Io { fn apply(root_path: &Path, blkio: &LinuxBlockIo) -> Result<(), V2IoControllerError> { if let Some(weight_device) = blkio.weight_device() { for wd in weight_device { - common::write_cgroup_file( - root_path.join(CGROUP_BFQ_IO_WEIGHT), - format!("{}:{} {}", wd.major(), wd.minor(), wd.weight().unwrap()), - )?; + if let Some(weight) = wd.weight() { + common::write_cgroup_file( + root_path.join(CGROUP_BFQ_IO_WEIGHT), + format!("{}:{} {}", wd.major(), wd.minor(), weight), + )?; + } } } if let Some(leaf_weight) = blkio.leaf_weight() { diff --git a/crates/libcontainer/src/container/builder_impl.rs b/crates/libcontainer/src/container/builder_impl.rs index 2d16cb95a..38f2f1ccf 100644 --- a/crates/libcontainer/src/container/builder_impl.rs +++ b/crates/libcontainer/src/container/builder_impl.rs @@ -129,7 +129,12 @@ impl ContainerBuilderImpl { // ourselves to be non-dumpable only breaks things (like rootless // containers), which is the recommendation from the kernel folks. if linux.namespaces().is_some() { - prctl::set_dumpable(false).unwrap(); + prctl::set_dumpable(false).map_err(|e| { + LibcontainerError::Other(format!( + "error in setting dumpable to false : {}", + nix::errno::from_i32(e) + )) + })?; } // This container_args will be passed to the container processes, diff --git a/crates/libcontainer/src/container/container_checkpoint.rs b/crates/libcontainer/src/container/container_checkpoint.rs index a60547348..8f722eeeb 100644 --- a/crates/libcontainer/src/container/container_checkpoint.rs +++ b/crates/libcontainer/src/container/container_checkpoint.rs @@ -13,6 +13,12 @@ use std::os::unix::io::AsRawFd; const CRIU_CHECKPOINT_LOG_FILE: &str = "dump.log"; const DESCRIPTORS_JSON: &str = "descriptors.json"; +#[derive(thiserror::Error, Debug)] +pub enum CheckpointError { + #[error("criu error: {0}")] + CriuError(String), +} + impl Container { pub fn checkpoint(&mut self, opts: &CheckpointOptions) -> Result<(), LibcontainerError> { self.refresh_status()?; @@ -25,8 +31,12 @@ impl Container { return Err(LibcontainerError::IncorrectStatus); } - let mut criu = rust_criu::Criu::new().unwrap(); - + let mut criu = rust_criu::Criu::new().map_err(|e| { + LibcontainerError::Checkpoint(CheckpointError::CriuError(format!( + "error in creating criu struct: {}", + e + ))) + })?; // We need to tell CRIU that all bind mounts are external. CRIU will fail checkpointing // if it does not know that these bind mounts are coming from the outside of the container. // This information is needed during restore again. The external location of the bind @@ -35,7 +45,7 @@ impl Container { let source_spec_path = self.bundle().join("config.json"); let spec = Spec::load(source_spec_path)?; let mounts = spec.mounts().clone(); - for m in mounts.unwrap() { + for m in mounts.unwrap_or_default() { match m.typ().as_deref() { Some("bind") => { let dest = m @@ -90,12 +100,19 @@ impl Container { criu.set_work_dir_fd(work_dir.as_raw_fd()); } - let pid: i32 = self.pid().unwrap().into(); + let pid: i32 = self + .pid() + .ok_or(LibcontainerError::Other( + "container process pid not found in state".into(), + ))? + .into(); // Remember original stdin, stdout, stderr for container restore. let mut descriptors = Vec::new(); for n in 0..3 { let link_path = match fs::read_link(format!("/proc/{pid}/fd/{n}")) { + // it should not have any non utf-8 or non os safe path, + // as we are reading from os , so ok to unwrap Ok(lp) => lp.into_os_string().into_string().unwrap(), Err(..) => "/dev/null".to_string(), }; diff --git a/crates/libcontainer/src/container/container_kill.rs b/crates/libcontainer/src/container/container_kill.rs index 6b5d08540..a2d60b601 100644 --- a/crates/libcontainer/src/container/container_kill.rs +++ b/crates/libcontainer/src/container/container_kill.rs @@ -57,7 +57,9 @@ impl Container { fn kill_one_process>(&self, signal: S) -> Result<(), LibcontainerError> { let signal = signal.into().into_raw(); - let pid = self.pid().unwrap(); + let pid = self.pid().ok_or(LibcontainerError::Other( + "container process pid not found in state".into(), + ))?; tracing::debug!("kill signal {} to {}", signal, pid); diff --git a/crates/libcontainer/src/container/mod.rs b/crates/libcontainer/src/container/mod.rs index 467f61231..c4874d59c 100644 --- a/crates/libcontainer/src/container/mod.rs +++ b/crates/libcontainer/src/container/mod.rs @@ -20,4 +20,5 @@ pub mod state; pub mod tenant_builder; pub use container::CheckpointOptions; pub use container::Container; +pub use container_checkpoint::CheckpointError; pub use state::{ContainerProcessState, ContainerStatus, State}; diff --git a/crates/libcontainer/src/container/tenant_builder.rs b/crates/libcontainer/src/container/tenant_builder.rs index a503cc6ff..0d805a0d8 100644 --- a/crates/libcontainer/src/container/tenant_builder.rs +++ b/crates/libcontainer/src/container/tenant_builder.rs @@ -321,13 +321,11 @@ impl TenantContainerBuilder { process_builder.build()? }; - if container.pid().is_none() { - return Err(LibcontainerError::Other( - "could not retrieve container init pid".into(), - )); - } + let container_pid = container.pid().ok_or(LibcontainerError::Other( + "could not retrieve container init pid".into(), + ))?; - let init_process = procfs::process::Process::new(container.pid().unwrap().as_raw())?; + let init_process = procfs::process::Process::new(container_pid.as_raw())?; let ns = self.get_namespaces(init_process.namespaces()?.0)?; let linux = LinuxBuilder::default().namespaces(ns).build()?; diff --git a/crates/libcontainer/src/error.rs b/crates/libcontainer/src/error.rs index f0555c962..f6efed7db 100644 --- a/crates/libcontainer/src/error.rs +++ b/crates/libcontainer/src/error.rs @@ -60,6 +60,8 @@ pub enum LibcontainerError { CgroupCreate(#[from] libcgroups::common::CreateCgroupSetupError), #[error(transparent)] CgroupGet(#[from] libcgroups::common::GetCgroupSetupError), + #[error[transparent]] + Checkpoint(#[from] crate::container::CheckpointError), // Catch all errors that are not covered by the above #[error("syscall error")] diff --git a/crates/libcontainer/src/notify_socket.rs b/crates/libcontainer/src/notify_socket.rs index 64837ca87..bc0ef5f02 100644 --- a/crates/libcontainer/src/notify_socket.rs +++ b/crates/libcontainer/src/notify_socket.rs @@ -65,6 +65,7 @@ impl NotifyListener { })?; let stream = UnixListener::bind(socket_name).map_err(|e| NotifyListenerError::Bind { source: e, + // ok to unwrap here as OsStr should always be utf-8 compatible name: socket_name.to_str().unwrap().to_owned(), })?; unistd::chdir(&cwd).map_err(|e| NotifyListenerError::Chdir { @@ -142,6 +143,7 @@ impl NotifySocket { let mut stream = UnixStream::connect(socket_name).map_err(|e| NotifyListenerError::Connect { source: e, + // ok to unwrap as OsStr should always be utf-8 compatible name: socket_name.to_str().unwrap().to_owned(), })?; stream diff --git a/crates/libcontainer/src/process/channel.rs b/crates/libcontainer/src/process/channel.rs index 5a03c4840..9683e5ed4 100644 --- a/crates/libcontainer/src/process/channel.rs +++ b/crates/libcontainer/src/process/channel.rs @@ -173,6 +173,10 @@ impl MainReceiver { })?; match msg { Message::InitReady => Ok(()), + // this case in unique and known enough to have a special error format + Message::ExecFailed(err) => Err(ChannelError::ExecError(format!( + "error in executing process : {err}" + ))), msg => Err(ChannelError::UnexpectedMessage { expected: Message::InitReady, received: msg, diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index 46327f781..fb01e0f57 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -323,8 +323,8 @@ pub fn container_init_process( })?; } - let bind_service = - namespaces.get(LinuxNamespaceType::User)?.is_some() || utils::is_in_new_userns(); + let in_user_ns = utils::is_in_new_userns().map_err(InitProcessError::Io)?; + let bind_service = namespaces.get(LinuxNamespaceType::User)?.is_some() || in_user_ns; let rootfs = RootFS::new(); rootfs .prepare_rootfs( @@ -355,6 +355,11 @@ pub fn container_init_process( })?; } + // As we have changed the root mount, from here on + // logs are no longer visible in journalctl + // so make sure that you bubble up any errors + // and do not call unwrap() as any panics would not be correctly logged + rootfs.adjust_root_mount_propagation(linux).map_err(|err| { tracing::error!(?err, "failed to adjust root mount propagation"); InitProcessError::RootFS(err) @@ -785,6 +790,8 @@ fn setup_scheduler(sc_op: &Option) -> Result<()> { } } let mut a = nc::sched_attr_t { + // size of the structure should always be within u32 bounds, + // so this unwrap should never fail size: mem::size_of::().try_into().unwrap(), sched_policy: policy, sched_flags: flags_value, diff --git a/crates/libcontainer/src/process/container_intermediate_process.rs b/crates/libcontainer/src/process/container_intermediate_process.rs index 8feb71ee7..0085416b8 100644 --- a/crates/libcontainer/src/process/container_intermediate_process.rs +++ b/crates/libcontainer/src/process/container_intermediate_process.rs @@ -29,6 +29,8 @@ pub enum IntermediateProcessError { ExecNotify(#[source] nix::Error), #[error(transparent)] MissingSpec(#[from] crate::error::MissingSpecError), + #[error("other error")] + Other(String), } type Result = std::result::Result; @@ -45,8 +47,8 @@ pub fn container_intermediate_process( let spec = &args.spec; let linux = spec.linux().as_ref().ok_or(MissingSpecError::Linux)?; let namespaces = Namespaces::try_from(linux.namespaces().as_ref())?; - let cgroup_manager = - libcgroups::common::create_cgroup_manager(args.cgroup_config.to_owned()).unwrap(); + let cgroup_manager = libcgroups::common::create_cgroup_manager(args.cgroup_config.to_owned()) + .map_err(|e| IntermediateProcessError::Cgroup(e.to_string()))?; // this needs to be done before we create the init process, so that the init // process will already be captured by the cgroup. It also needs to be done @@ -122,19 +124,19 @@ pub fn container_intermediate_process( match container_init_process(&args, &mut main_sender, &mut init_receiver) { Ok(_) => 0, Err(e) => { + tracing::error!("failed to initialize container process: {e}"); + if let Err(err) = main_sender.exec_failed(e.to_string()) { + tracing::error!(?err, "failed sending error to main sender"); + } if let ContainerType::TenantContainer { exec_notify_fd } = args.container_type { let buf = format!("{e}"); if let Err(err) = write(exec_notify_fd, buf.as_bytes()) { tracing::error!(?err, "failed to write to exec notify fd"); - return -1; } if let Err(err) = close(exec_notify_fd) { tracing::error!(?err, "failed to close exec notify fd"); - return -1; } } - - tracing::error!("failed to initialize container process: {e}"); -1 } } @@ -201,7 +203,12 @@ fn setup_userns( tracing::debug!("creating new user namespace"); // child needs to be dumpable, otherwise the non root parent is not // allowed to write the uid/gid maps - prctl::set_dumpable(true).unwrap(); + prctl::set_dumpable(true).map_err(|e| { + IntermediateProcessError::Other(format!( + "error in setting dumpable to true : {}", + nix::errno::from_i32(e) + )) + })?; sender.identifier_mapping_request().map_err(|err| { tracing::error!("failed to send id mapping request: {}", err); err @@ -210,7 +217,12 @@ fn setup_userns( tracing::error!("failed to receive id mapping ack: {}", err); err })?; - prctl::set_dumpable(false).unwrap(); + prctl::set_dumpable(false).map_err(|e| { + IntermediateProcessError::Other(format!( + "error in setting dumplable to false : {}", + nix::errno::from_i32(e) + )) + })?; Ok(()) } diff --git a/crates/libcontainer/src/process/intel_rdt.rs b/crates/libcontainer/src/process/intel_rdt.rs index 66406b6f1..f7527ff71 100644 --- a/crates/libcontainer/src/process/intel_rdt.rs +++ b/crates/libcontainer/src/process/intel_rdt.rs @@ -161,27 +161,27 @@ fn combine_l3_cache_and_mem_bw_schemas( l3_cache_schema: &Option, mem_bw_schema: &Option, ) -> Option { - if l3_cache_schema.is_some() && mem_bw_schema.is_some() { - // Combine the results. Filter out "MB:"-lines from l3_cache_schema - let real_l3_cache_schema = l3_cache_schema.as_ref().unwrap(); - let real_mem_bw_schema = mem_bw_schema.as_ref().unwrap(); - let mut output: Vec<&str> = vec![]; - - for line in real_l3_cache_schema.lines() { - if line.starts_with("MB:") { - continue; + match (l3_cache_schema, mem_bw_schema) { + (Some(ref real_l3_cache_schema), Some(ref real_mem_bw_schema)) => { + // Combine the results. Filter out "MB:"-lines from l3_cache_schema + let mut output: Vec<&str> = vec![]; + + for line in real_l3_cache_schema.lines() { + if line.starts_with("MB:") { + continue; + } + output.push(line); } - output.push(line); + output.push(real_mem_bw_schema); + Some(output.join("\n")) + } + (Some(_), None) => { + // Apprarently the "MB:"-lines don't need to be removed in this case? + l3_cache_schema.to_owned() } - output.push(real_mem_bw_schema); - return Some(output.join("\n")); - } else if l3_cache_schema.is_some() { - // Apprarently the "MB:"-lines don't need to be removed in this case? - return l3_cache_schema.to_owned(); - } else if mem_bw_schema.is_some() { - return mem_bw_schema.to_owned(); + (None, Some(_)) => mem_bw_schema.to_owned(), + (None, None) => None, } - None } #[derive(PartialEq)] diff --git a/crates/libcontainer/src/user_ns.rs b/crates/libcontainer/src/user_ns.rs index 082b23ba6..b23e32e47 100644 --- a/crates/libcontainer/src/user_ns.rs +++ b/crates/libcontainer/src/user_ns.rs @@ -78,6 +78,8 @@ pub enum UserNamespaceError { UnknownUnprivilegedUsernsClone(u8), #[error(transparent)] IDMapping(#[from] MappingError), + #[error(transparent)] + OtherIO(#[from] std::io::Error), } type Result = std::result::Result; @@ -108,6 +110,8 @@ pub enum ValidateSpecError { MountUidMapping(u32), #[error(transparent)] Namespaces(#[from] NamespaceError), + #[error(transparent)] + OtherIO(#[from] std::io::Error), } #[derive(Debug, thiserror::Error)] @@ -217,7 +221,7 @@ impl TryFrom<&Linux> for UserNamespaceConfig { uid_mappings: linux.uid_mappings().to_owned(), gid_mappings: linux.gid_mappings().to_owned(), user_namespace: user_namespace.cloned(), - privileged: !utils::rootless_required(), + privileged: !utils::rootless_required()?, id_mapper: UserNamespaceIDMapper::new(), }) } @@ -281,7 +285,7 @@ fn validate_spec_for_new_user_ns(spec: &Spec) -> std::result::Result<(), Validat .as_ref() .and_then(|process| process.user().additional_gids().as_ref()) { - let privileged = !utils::rootless_required(); + let privileged = !utils::rootless_required()?; match (privileged, additional_gids.is_empty()) { (true, false) => { @@ -421,6 +425,9 @@ fn write_id_mapping( }) .collect(); + // we can be certain here that map_binary will not be None, + // as in the lookup_map_binaries function, we return error + // if there are mappings.len() > 1 and binaries are not present Command::new(map_binary.unwrap()) .arg(pid.to_string()) .args(args) diff --git a/crates/libcontainer/src/utils.rs b/crates/libcontainer/src/utils.rs index a15ed933d..024b970f3 100644 --- a/crates/libcontainer/src/utils.rs +++ b/crates/libcontainer/src/utils.rs @@ -261,17 +261,16 @@ pub fn ensure_procfs(path: &Path) -> Result<(), EnsureProcfsError> { Ok(()) } -pub fn is_in_new_userns() -> bool { +pub fn is_in_new_userns() -> Result { let uid_map_path = "/proc/self/uid_map"; - let content = std::fs::read_to_string(uid_map_path) - .unwrap_or_else(|_| panic!("failed to read {}", uid_map_path)); - !content.contains("4294967295") + let content = std::fs::read_to_string(uid_map_path)?; + Ok(!content.contains("4294967295")) } /// Checks if rootless mode needs to be used -pub fn rootless_required() -> bool { +pub fn rootless_required() -> Result { if !nix::unistd::geteuid().is_root() { - return true; + return Ok(true); } is_in_new_userns() } @@ -279,14 +278,15 @@ pub fn rootless_required() -> bool { /// checks if given spec is valid for current user namespace setup pub fn validate_spec_for_new_user_ns(spec: &Spec) -> Result<(), LibcontainerError> { let config = UserNamespaceConfig::new(spec)?; - + let in_user_ns = is_in_new_userns().map_err(LibcontainerError::OtherIO)?; + let is_rootless_required = rootless_required().map_err(LibcontainerError::OtherIO)?; // In case of rootless, there are 2 possible cases : // we have a new user ns specified in the spec // or the youki is launched in a new user ns (this is how podman does it) // So here, we check if rootless is required, // but we are neither in a new user ns nor a new user ns is specified in spec // then it is an error - if rootless_required() && !is_in_new_userns() && config.is_none() { + if is_rootless_required && !in_user_ns && config.is_none() { return Err(LibcontainerError::NoUserNamespace); } Ok(()) diff --git a/crates/libcontainer/src/workload/default.rs b/crates/libcontainer/src/workload/default.rs index 28f5ef974..f57d9a6a9 100644 --- a/crates/libcontainer/src/workload/default.rs +++ b/crates/libcontainer/src/workload/default.rs @@ -34,7 +34,13 @@ impl Executor for DefaultExecutor { .collect(); unistd::execvp(&cstring_path, &a).map_err(|err| { tracing::error!(?err, filename = ?cstring_path, args = ?a, "failed to execvp"); - ExecutorError::Execution(err.into()) + ExecutorError::Execution( + format!( + "error '{}' executing '{:?}' with args '{:?}'", + err, cstring_path, a + ) + .into(), + ) })?; // After execvp is called, the process is replaced with the container @@ -46,14 +52,18 @@ impl Executor for DefaultExecutor { let proc = spec .process() .as_ref() - .ok_or(ExecutorValidationError::InvalidArg)?; + .ok_or(ExecutorValidationError::ArgValidationError( + "spec did not contain process".into(), + ))?; if let Some(args) = proc.args() { let envs: Vec = proc.env().as_ref().unwrap_or(&vec![]).clone(); let path_vars: Vec<&String> = envs.iter().filter(|&e| e.starts_with("PATH=")).collect(); if path_vars.is_empty() { tracing::error!("PATH environment variable is not set"); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError( + "PATH environment variable is not set".into(), + ))?; } let path_var = path_vars[0].trim_start_matches("PATH="); match get_executable_path(&args[0], path_var) { @@ -62,7 +72,10 @@ impl Executor for DefaultExecutor { executable = ?args[0], "executable for container process not found in PATH", ); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError(format!( + "executable '{}' not found in $PATH", + args[0] + )))?; } Some(path) => match is_executable(&path) { Ok(true) => { @@ -73,7 +86,10 @@ impl Executor for DefaultExecutor { executable = ?path, "executable does not have the correct permission set", ); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError(format!( + "executable '{}' at path '{:?}' does not have correct permissions", + args[0], path + )))?; } Err(err) => { tracing::error!( @@ -81,7 +97,10 @@ impl Executor for DefaultExecutor { ?err, "failed to check permissions for executable", ); - Err(ExecutorValidationError::InvalidArg)?; + Err(ExecutorValidationError::ArgValidationError(format!( + "failed to check permissions for executable '{}' at path '{:?}' : {}", + args[0], path, err + )))?; } }, } diff --git a/crates/libcontainer/src/workload/mod.rs b/crates/libcontainer/src/workload/mod.rs index 722a3cf73..78a3980f6 100644 --- a/crates/libcontainer/src/workload/mod.rs +++ b/crates/libcontainer/src/workload/mod.rs @@ -20,8 +20,8 @@ pub enum ExecutorError { pub enum ExecutorValidationError { #[error("{0} executor can't handle spec")] CantHandle(&'static str), - #[error("invalid argument")] - InvalidArg, + #[error("{0}")] + ArgValidationError(String), } // Here is an explanation about the complexity below regarding to diff --git a/crates/youki/src/main.rs b/crates/youki/src/main.rs index 6a92be8d0..668d2ca6d 100644 --- a/crates/youki/src/main.rs +++ b/crates/youki/src/main.rs @@ -123,6 +123,7 @@ fn main() -> Result<()> { CommonCmd::Exec(exec) => match commands::exec::exec(exec, root_path) { Ok(exit_code) => std::process::exit(exit_code), Err(e) => { + tracing::error!("error in executing command: {:?}", e); eprintln!("exec failed : {e}"); std::process::exit(-1); } @@ -135,6 +136,7 @@ fn main() -> Result<()> { CommonCmd::Run(run) => match commands::run::run(run, root_path, systemd_cgroup) { Ok(exit_code) => std::process::exit(exit_code), Err(e) => { + tracing::error!("error in executing command: {:?}", e); eprintln!("run failed : {e}"); std::process::exit(-1); } @@ -151,6 +153,7 @@ fn main() -> Result<()> { if let Err(ref e) = cmd_result { tracing::error!("error in executing command: {:?}", e); + eprintln!("error in executing command: {:?}", e); } cmd_result } diff --git a/crates/youki/src/rootpath.rs b/crates/youki/src/rootpath.rs index bf362fcad..80c7d49c4 100644 --- a/crates/youki/src/rootpath.rs +++ b/crates/youki/src/rootpath.rs @@ -18,7 +18,7 @@ pub fn determine(root_path: Option) -> Result { return Ok(path); } - if !rootless_required() { + if !rootless_required()? { let path = get_default_not_rootless_path(); create_dir_all_with_mode(&path, uid, Mode::S_IRWXU)?; return Ok(path); diff --git a/crates/youki/src/workload/wasmedge.rs b/crates/youki/src/workload/wasmedge.rs index 4a6aa6111..a4e6dcf25 100644 --- a/crates/youki/src/workload/wasmedge.rs +++ b/crates/youki/src/workload/wasmedge.rs @@ -104,6 +104,8 @@ fn get_args(spec: &Spec) -> &[String] { fn env_to_wasi(spec: &Spec) -> Vec { let default = vec![]; + // below we can be sure that process exists, as otherwise container init process + // function would have returned error at the very start let env = spec .process() .as_ref()