Merge pull request #2705 from YJDoc2/fix/improve_logging_errors

Improve error reporting and logging

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<bool, WrappedIoError> {
     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",

crates/libcgroups/src/stats.rs

@@ -216,6 +216,8 @@ pub fn supported_page_sizes() -> Result<Vec<String>, 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)?);

crates/libcgroups/src/systemd/dbus_native/dbus.rs

@@ -46,16 +46,14 @@ fn parse_dbus_address(env_value: String) -> Result<String> {
     // 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<u32> {
         .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::<u32>()

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

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),
 }

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)?,
         )?)
     }
 

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() {

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,

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(),
             };

crates/libcontainer/src/container/container_kill.rs

@@ -57,7 +57,9 @@ impl Container {
 
     fn kill_one_process<S: Into<Signal>>(&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);
 

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};

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()?;
 

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")]

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

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,

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<Scheduler>) -> 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::<nc::sched_attr_t>().try_into().unwrap(),
             sched_policy: policy,
             sched_flags: flags_value,

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<T> = std::result::Result<T, IntermediateProcessError>;
@@ -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(())
 }