Add unit tests, refactor functions as per review

Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>

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

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<PathBuf> {
+    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<PathBuf> {
     None
 }
 
+pub fn is_executable(path: &Path) -> Result<bool> {
+    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("/boot/initrd.img");
+        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());
+    }
 }

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"