From 7f17c2793be5948749cc0316c8f7b79b21930d9d Mon Sep 17 00:00:00 2001 From: Yashodhan Joshi Date: Mon, 28 Nov 2022 10:58:32 +0530 Subject: [PATCH] Add unit tests, refactor functions as per review Signed-off-by: Yashodhan Joshi --- .../src/process/container_init_process.rs | 14 ++---- crates/libcontainer/src/utils.rs | 49 ++++++++++++++++++- scripts/oci_integration_tests.sh | 2 +- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/crates/libcontainer/src/process/container_init_process.rs b/crates/libcontainer/src/process/container_init_process.rs index f4c85b4782..78aa074e74 100644 --- a/crates/libcontainer/src/process/container_init_process.rs +++ b/crates/libcontainer/src/process/container_init_process.rs @@ -15,7 +15,6 @@ use nix::unistd::setsid; use nix::unistd::{self, Gid, Uid}; use oci_spec::runtime::{LinuxNamespaceType, Spec, User}; use std::collections::HashMap; -use std::os::unix::fs::PermissionsExt; use std::os::unix::io::AsRawFd; use std::{ env, fs, @@ -395,8 +394,8 @@ pub fn container_init_process( } } - // this checks if the binary to run actually exists and if we have permissions to - // run it. Taken from https://github.com/opencontainers/runc/blob/main/libcontainer/standard_init_linux.go#L195-L206 + // this checks if the binary to run actually exists and if we have permissions to run it. + // Taken from https://github.com/opencontainers/runc/blob/25c9e888686773e7e06429133578038a9abc091d/libcontainer/standard_init_linux.go#L195-L206 if let Some(args) = proc.args() { let path_var = { let mut ret: &str = ""; @@ -405,7 +404,7 @@ pub fn container_init_process( ret = var; } } - ret.trim_start_matches("PATH=") + ret }; let executable_path = utils::get_executable_path(&args[0], path_var); match executable_path { @@ -414,12 +413,7 @@ pub fn container_init_process( args[0] ), Some(path) => { - let metadata = path.metadata()?; - let permissions = metadata.permissions(); - // we have to check if it a file, as dir have there executable bit set, - // and check if x in rwx is unset. Logic is taken - // from https://docs.rs/is_executable/latest/src/is_executable/lib.rs.html#90 - if !metadata.is_file() || permissions.mode() & 0o001 == 0 { + if !utils::is_executable(&path)? { bail!("file {:?} does not have executable permission set", path); } } diff --git a/crates/libcontainer/src/utils.rs b/crates/libcontainer/src/utils.rs index d6189b7737..004cbabbad 100644 --- a/crates/libcontainer/src/utils.rs +++ b/crates/libcontainer/src/utils.rs @@ -332,11 +332,12 @@ pub fn get_temp_dir_path(test_name: &str) -> PathBuf { } pub fn get_executable_path(name: &str, path_var: &str) -> Option { + let paths = path_var.trim_start_matches("PATH="); // if path has / in it, we have to assume absolute path, as per runc impl if name.contains('/') && PathBuf::from(name).exists() { return Some(PathBuf::from(name)); } - for path in path_var.split(':') { + for path in paths.split(':') { let potential_path = PathBuf::from(path).join(name); if potential_path.exists() { return Some(potential_path); @@ -345,6 +346,16 @@ pub fn get_executable_path(name: &str, path_var: &str) -> Option { None } +pub fn is_executable(path: &Path) -> Result { + use std::os::unix::fs::PermissionsExt; + let metadata = path.metadata()?; + let permissions = metadata.permissions(); + // we have to check if the path is file and the execute bit + // is set. In case of directories, the execute bit is also set, + // so have to check if this is a file or not + Ok(metadata.is_file() && permissions.mode() & 0o001 != 0) +} + #[cfg(test)] pub(crate) mod test_utils { use crate::process::channel; @@ -519,4 +530,40 @@ mod tests { PathBuf::from(&test_root_dir).join("somepath/passwd") ); } + + #[test] + fn test_get_executable_path() { + let non_existing_abs_path = "/some/non/existent/absolute/path"; + let existing_abs_path = "/usr/bin/sh"; + let existing_binary = "sh"; + let non_existing_binary = "non-existent"; + let path_value = "PATH=/usr/bin:/bin"; + + assert_eq!( + get_executable_path(existing_abs_path, path_value), + Some(PathBuf::from(existing_abs_path)) + ); + assert_eq!(get_executable_path(non_existing_abs_path, path_value), None); + + assert_eq!( + get_executable_path(existing_binary, path_value), + Some(PathBuf::from("/usr/bin/sh")) + ); + + assert_eq!(get_executable_path(non_existing_binary, path_value), None); + } + + #[test] + fn test_is_executable() { + let executable_path = PathBuf::from("/bin/sh"); + let directory_path = PathBuf::from("/tmp"); + // a file guaranteed to be on linux and not executable + let non_executable_path = PathBuf::from("/proc/self/syscall"); + let non_existent_path = PathBuf::from("/some/non/existent/path"); + + assert!(is_executable(&non_existent_path).is_err()); + assert!(is_executable(&executable_path).unwrap()); + assert!(is_executable(&non_executable_path).unwrap()); + assert!(!is_executable(&directory_path).unwrap()); + } } diff --git a/scripts/oci_integration_tests.sh b/scripts/oci_integration_tests.sh index 36cb43df5a..f043216a7a 100755 --- a/scripts/oci_integration_tests.sh +++ b/scripts/oci_integration_tests.sh @@ -54,7 +54,7 @@ test_cases=( "linux_seccomp/linux_seccomp.t" "linux_sysctl/linux_sysctl.t" "linux_uid_mappings/linux_uid_mappings.t" - #"misc_props/misc_props.t" runc also fails this, check out https://github.com/containers/youki/pull/1347#issuecomment-1315332775 + # "misc_props/misc_props.t" runc also fails this, check out https://github.com/containers/youki/pull/1347#issuecomment-1315332775 "mounts/mounts.t" # This test case passed on local box, but not on Github Action. `runc` also fails on Github Action, so likely it is an issue with the test. # "pidfile/pidfile.t"