Merge pull request #740 from Szymongib/fix/handle-relative-paths

Handle relative paths

crates/libcontainer/src/container/builder.rs

@@ -1,7 +1,9 @@
 use crate::syscall::Syscall;
+use anyhow::{Context, Result};
 use std::path::PathBuf;
 
 use super::{init_builder::InitContainerBuilder, tenant_builder::TenantContainerBuilder};
+
 pub struct ContainerBuilder<'a> {
     /// Id of the container
     pub(super) container_id: String,
@@ -28,8 +30,8 @@ pub struct ContainerBuilder<'a> {
 /// use libcontainer::syscall::syscall::create_syscall;
 ///
 /// ContainerBuilder::new("74f1a4cb3801".to_owned(), create_syscall().as_ref())
-/// .with_root_path("/run/containers/youki")
-/// .with_pid_file(Some("/var/run/docker.pid"))
+/// .with_root_path("/run/containers/youki").expect("invalid root path")
+/// .with_pid_file(Some("/var/run/docker.pid")).expect("invalid pid file")
 /// .with_console_socket(Some("/var/run/docker/sock.tty"))
 /// .as_init("/var/run/docker/bundle")
 /// .build();
@@ -101,11 +103,14 @@ impl<'a> ContainerBuilder<'a> {
     /// # use libcontainer::syscall::syscall::create_syscall;
     ///
     /// ContainerBuilder::new("74f1a4cb3801".to_owned(), create_syscall().as_ref())
-    /// .with_root_path("/run/containers/youki");
+    /// .with_root_path("/run/containers/youki").expect("invalid root path");
     /// ```
-    pub fn with_root_path<P: Into<PathBuf>>(mut self, path: P) -> Self {
-        self.root_path = path.into();
-        self
+    pub fn with_root_path<P: Into<PathBuf>>(mut self, path: P) -> Result<Self> {
+        let path = path.into();
+        self.root_path = path
+            .canonicalize()
+            .with_context(|| format!("failed to canonicalize root path {:?}", path))?;
+        Ok(self)
     }
 
     /// Sets the pid file which will be used to write the pid of the container
@@ -117,11 +122,19 @@ impl<'a> ContainerBuilder<'a> {
     /// # use libcontainer::syscall::syscall::create_syscall;
     ///
     /// ContainerBuilder::new("74f1a4cb3801".to_owned(), create_syscall().as_ref())
-    /// .with_pid_file(Some("/var/run/docker.pid"));
+    /// .with_pid_file(Some("/var/run/docker.pid")).expect("invalid pid file");
     /// ```
-    pub fn with_pid_file<P: Into<PathBuf>>(mut self, path: Option<P>) -> Self {
-        self.pid_file = path.map(|p| p.into());
-        self
+    pub fn with_pid_file<P: Into<PathBuf>>(mut self, path: Option<P>) -> Result<Self> {
+        self.pid_file = if let Some(p) = path {
+            let path = p.into();
+            Some(
+                path.canonicalize()
+                    .with_context(|| format!("failed to canonicalize pid file path {:?}", path))?,
+            )
+        } else {
+            None
+        };
+        Ok(self)
     }
 
     /// Sets the console socket, which will be used to send the file descriptor
@@ -156,3 +169,42 @@ impl<'a> ContainerBuilder<'a> {
         self
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::container::builder::ContainerBuilder;
+    use crate::syscall::syscall::create_syscall;
+    use crate::utils::TempDir;
+    use anyhow::{Context, Result};
+    use std::path::PathBuf;
+
+    #[test]
+    fn test_builder_failable_functions() -> Result<()> {
+        let root_path_temp_dir = TempDir::new("root_path").context("failed to create temp dir")?;
+        let pid_file_temp_dir = TempDir::new("pid_file").context("failed to create temp dir")?;
+        let syscall = create_syscall();
+
+        ContainerBuilder::new("74f1a4cb3801".to_owned(), syscall.as_ref())
+            .with_root_path(root_path_temp_dir.path())?
+            .with_pid_file(Some(pid_file_temp_dir.path()))?
+            .with_console_socket(Some("/var/run/docker/sock.tty"))
+            .as_init("/var/run/docker/bundle");
+
+        // Accept None pid file.
+        ContainerBuilder::new("74f1a4cb3801".to_owned(), syscall.as_ref())
+            .with_pid_file::<PathBuf>(None)?;
+
+        let invalid_path_builder =
+            ContainerBuilder::new("74f1a4cb3801".to_owned(), syscall.as_ref())
+                .with_root_path("/not/existing/path");
+        assert!(invalid_path_builder.is_err());
+
+        let invalid_pid_file_builder =
+            ContainerBuilder::new("74f1a4cb3801".to_owned(), syscall.as_ref())
+                .with_root_path(root_path_temp_dir.path())?
+                .with_pid_file(Some("/not/existing/path"));
+        assert!(invalid_pid_file_builder.is_err());
+
+        Ok(())
+    }
+}

crates/libcontainer/src/container/init_builder.rs

@@ -39,10 +39,14 @@ impl<'a> InitContainerBuilder<'a> {
 
     /// Creates a new container
     pub fn build(self) -> Result<Container> {
-        let spec = self.load_spec()?;
-        let container_dir = self.create_container_dir()?;
-
-        let mut container = self.create_container_state(&container_dir)?;
+        let spec = self.load_spec().context("failed to load spec")?;
+        let container_dir = self
+            .create_container_dir()
+            .context("failed to create container dir")?;
+
+        let mut container = self
+            .create_container_state(&container_dir)
+            .context("failed to create container state")?;
         container
             .set_systemd(self.use_systemd)
             .set_annotations(spec.annotations().clone());
@@ -66,7 +70,9 @@ impl<'a> InitContainerBuilder<'a> {
 
         let rootless = Rootless::new(&spec)?;
         let config = YoukiConfig::from_spec(&spec, container.id(), rootless.is_some())?;
-        config.save(&container_dir)?;
+        config
+            .save(&container_dir)
+            .context("failed to save config")?;
 
         let mut builder_impl = ContainerBuilderImpl {
             init: true,
@@ -97,7 +103,8 @@ impl<'a> InitContainerBuilder<'a> {
             bail!("container {} already exists", self.base.container_id);
         }
 
-        utils::create_dir_all(&container_dir)?;
+        utils::create_dir_all(&container_dir).context("failed to create container dir")?;
+
         Ok(container_dir)
     }
 

crates/youki/src/commands/create.rs

@@ -13,9 +13,9 @@ use liboci_cli::Create;
 pub fn create(args: Create, root_path: PathBuf, systemd_cgroup: bool) -> Result<()> {
     let syscall = create_syscall();
     ContainerBuilder::new(args.container_id.clone(), syscall.as_ref())
-        .with_pid_file(args.pid_file.as_ref())
+        .with_pid_file(args.pid_file.as_ref())?
         .with_console_socket(args.console_socket.as_ref())
-        .with_root_path(root_path)
+        .with_root_path(root_path)?
         .with_preserved_fds(args.preserve_fds)
         .as_init(&args.bundle)
         .with_systemd(systemd_cgroup)

crates/youki/src/commands/exec.rs

@@ -7,9 +7,9 @@ use liboci_cli::Exec;
 pub fn exec(args: Exec, root_path: PathBuf) -> Result<()> {
     let syscall = create_syscall();
     ContainerBuilder::new(args.container_id.clone(), syscall.as_ref())
-        .with_root_path(root_path)
+        .with_root_path(root_path)?
         .with_console_socket(args.console_socket.as_ref())
-        .with_pid_file(args.pid_file.as_ref())
+        .with_pid_file(args.pid_file.as_ref())?
         .as_tenant()
         .with_cwd(args.cwd.as_ref())
         .with_env(args.env.clone().into_iter().collect())

crates/youki/src/commands/run.rs

@@ -7,9 +7,9 @@ use liboci_cli::Run;
 pub fn run(args: Run, root_path: PathBuf, systemd_cgroup: bool) -> Result<()> {
     let syscall = create_syscall();
     let mut container = ContainerBuilder::new(args.container_id.clone(), syscall.as_ref())
-        .with_pid_file(args.pid_file.as_ref())
+        .with_pid_file(args.pid_file.as_ref())?
         .with_console_socket(args.console_socket.as_ref())
-        .with_root_path(root_path)
+        .with_root_path(root_path)?
         .with_preserved_fds(args.preserve_fds)
         .as_init(&args.bundle)
         .with_systemd(systemd_cgroup)

crates/youki/src/commands/state.rs

@@ -1,15 +1,12 @@
-use std::fs;
 use std::path::PathBuf;
 
 use anyhow::Result;
 
-use libcontainer::container::Container;
+use crate::commands::load_container;
 use liboci_cli::State;
 
 pub fn state(args: State, root_path: PathBuf) -> Result<()> {
-    let root_path = fs::canonicalize(root_path)?;
-    let container_root = root_path.join(&args.container_id);
-    let container = Container::load(container_root)?;
+    let container = load_container(root_path, &args.container_id)?;
     println!("{}", serde_json::to_string_pretty(&container.state)?);
     std::process::exit(0);
 }

crates/youki/src/main.rs

@@ -130,10 +130,15 @@ fn main() -> Result<()> {
 }
 
 fn determine_root_path(root_path: Option<PathBuf>) -> Result<PathBuf> {
+    let uid = getuid().as_raw();
+
     if let Some(path) = root_path {
+        if !path.exists() {
+            create_dir_all_with_mode(&path, uid, Mode::S_IRWXU)?;
+        }
+        let path = path.canonicalize()?;
         return Ok(path);
     }
-    let uid = getuid().as_raw();
 
     if !rootless_required() {
         let path = get_default_not_rootless_path();
@@ -202,13 +207,23 @@ mod tests {
     use std::fs;
     use std::fs::Permissions;
     use std::os::unix::fs::PermissionsExt;
-    use std::path::PathBuf;
+    use std::path::{Path, PathBuf};
 
     #[test]
     fn test_determine_root_path_use_specified_by_user() -> Result<()> {
+        // Create directory if it does not exist and return absolute path.
         let specified_path = get_temp_dir_path("provided_path");
-        let path = determine_root_path(Some(specified_path.clone()))
-            .context("failed with specified path")?;
+        // Make sure directory does not exist.
+        remove_dir(&specified_path)?;
+        let non_abs_path = specified_path.join("../provided_path");
+        let path = determine_root_path(Some(non_abs_path)).context("failed with specified path")?;
+        assert_eq!(path, specified_path);
+
+        // Return absolute path if directory exists.
+        let specified_path = get_temp_dir_path("provided_path2");
+        let _temp_dir = TempDir::new(&specified_path).context("failed to create temp dir")?;
+        let non_abs_path = specified_path.join("../provided_path2");
+        let path = determine_root_path(Some(non_abs_path)).context("failed with specified path")?;
         assert_eq!(path, specified_path);
 
         Ok(())
@@ -298,4 +313,11 @@ mod tests {
 
         Ok(())
     }
+
+    fn remove_dir(path: &Path) -> Result<()> {
+        if path.exists() {
+            fs::remove_dir(path).context("failed to remove directory")?;
+        }
+        Ok(())
+    }
 }