chdir_syscall() and fchdir_syscall() updates (#294)

ve1nard · lind · web-flow · commit c830d628f644 · 2024-07-13T14:29:37.000-04:00
* updated syscall description

* removed refcount and finished the description

* testing chdir

* new tests for chdir and fchdir

* formatted

* removed decrementing refcount for cwd

* fixed according to the review

* merged develop and fixed typos

* fixed according to the review

---------

Co-authored-by: lind &lt;lind@nyu.edu&gt;
diff --git a/src/safeposix/filesystem.rs b/src/safeposix/filesystem.rs
@@ -636,22 +636,3 @@ pub fn incref_root() {
         panic!("Root directory inode was not a directory");
     }
 }
-
-pub fn decref_dir(cwd_container: &interface::RustPathBuf) {
-    if let Some(cwdinodenum) = metawalk(&cwd_container) {
-        if let Inode::Dir(ref mut cwddir) = *(FS_METADATA.inodetable.get_mut(&cwdinodenum).unwrap())
-        {
-            cwddir.refcount -= 1;
-
-            //if the directory has been removed but this cwd was the last open handle to it
-            if cwddir.refcount == 0 && cwddir.linkcount == 0 {
-                FS_METADATA.inodetable.remove(&cwdinodenum);
-            }
-        } else {
-            panic!("Cage had a cwd that was not a directory!");
-        }
-    } else {
-        panic!("Cage had a cwd which did not exist!");
-    } //we probably want to handle this case, maybe cwd should be an inode
-      // number?? Not urgent
-}
diff --git a/src/safeposix/syscalls/fs_calls.rs b/src/safeposix/syscalls/fs_calls.rs
@@ -2381,15 +2381,59 @@ impl Cage {
         }
     }
 
-    //------------------------------------FCHDIR SYSCALL------------------------------------
+    /// ### Description
+    ///
+    /// The `fchdir_syscall()` function changes the current working
+    /// directory of the calling process to the directory specified
+    /// by an open file descriptor `fd`.
+    ///
+    /// ### Arguments
+    ///
+    /// The `fchdir_syscall()` accepts one argument:
+    /// * `fd` - an open file descriptor that specifies the directory
+    /// to which we want to change the current working directory.
+    ///
+    /// ### Returns
+    ///
+    /// Upon successful completion, zero is returned.
+    /// In case of a failure, an error is returned, and `errno` is set depending
+    /// on the error, e.g. `ENOTDIR`, `EBADF`, etc.
+    ///
+    /// ### Errors
+    ///
+    /// * `EBADF` - fd is not a valid file descriptor.
+    /// * `ENOTDIR` - the open file descriptor fildes does not refer to a directory.
+    /// Other errors, like `EACCES`, `ENOMEM`, etc. are not supported.
+    ///
+    /// ### Panics
+    ///
+    /// * invalid or out-of-bounds file descriptor, calling unwrap() on which
+    /// causes a panic.
+    ///
+    /// To learn more about the syscall and possible error values, see
+    /// [fchdir(2)](https://linux.die.net/man/2/fchdir)
 
     pub fn fchdir_syscall(&self, fd: i32) -> i32 {
+        //BUG
+        //if the provided file descriptor is out of bounds, get_filedescriptor returns
+        //Err(), unwrapping on which  produces a 'panic!'
+        //otherwise, file descriptor table entry is stored in 'checkedfd'
         let checkedfd = self.get_filedescriptor(fd).unwrap();
         let unlocked_fd = checkedfd.read();
-
+        //If a table descriptor entry corresponds to a file, we check if it is
+        //a directory file type. If it is not, we return `A component of path is
+        //not a directory` error.
+        //If it is one of the special file types, we return `Cannot change working
+        //directory on this file descriptor` error.
+        //Finally, if it does not correspond to any file type, we return `Invalid file
+        //descriptor` error.
         let path_string = match &*unlocked_fd {
+            None => return syscall_error(Errno::EBADF, "fchdir", "invalid file descriptor"),
             Some(File(normalfile_filedesc_obj)) => {
                 let inodenum = normalfile_filedesc_obj.inode;
+                //`pathnamefrominodenum` resolves the absolute path of a directory
+                //from its inode and returns None in case any of the path components
+                //is not a directory
                 match pathnamefrominodenum(inodenum) {
                     Some(name) => name,
                     None => {
@@ -2403,31 +2447,76 @@ impl Cage {
             }
             Some(_) => {
                 return syscall_error(
-                    Errno::EACCES,
+                    Errno::ENOTDIR,
                     "fchdir",
-                    "cannot change working directory on this file descriptor",
+                    "the file descriptor does not refer to a directory",
                 )
-            }
-            None => return syscall_error(Errno::EBADF, "fchdir", "invalid file descriptor"),
+            },
         };
-
+        //Obtain the write lock on the current working directory of the cage
+        //and change it to the new directory
         let mut cwd_container = self.cwd.write();
-
-        *cwd_container = interface::RustRfc::new(convpath(path_string.as_str()));
+        *cwd_container = interface::RustRfc::new(normpath(convpath(path_string.as_str()), self));
 
         0 // fchdir success
     }
 
-    //------------------------------------CHDIR SYSCALL------------------------------------
+    /// ### Description
+    ///
+    /// The `chdir_syscall()` function changes the current working
+    /// directory of the calling process to the directory specified
+    /// in `path`, which can be an absolute or a relative pathname.
+    ///
+    /// ### Arguments
+    ///
+    /// The `chdir_syscall()` accepts one argument:
+    /// * `path` - the pathname, to which the current working
+    /// directory shall be changed.
+    ///
+    /// ### Returns
+    ///
+    /// Upon successful completion, zero is returned.
+    /// In case of a failure, an error is returned, and `errno` is set depending
+    /// on the error, e.g. EACCES, ENOENT, etc.
+    ///
+    /// ### Errors
+    ///
+    /// * `ENOTDIR` - a component of `path` is not a directory.
+    /// * `ENOENT` - the directory specified in path does not exist.
+    /// Other errors, like `EACCES`, `ENOMEM`, etc. are not supported.
+    ///
+    /// ### Panics
+    ///
+    /// * when the previous working directory does not exist or does not
+    /// have the directory file type flag, the function panics
+    ///
+    /// To learn more about the syscall and possible error values, see
+    /// [chdir(2)](https://man7.org/linux/man-pages/man2/chdir.2.html)
 
     pub fn chdir_syscall(&self, path: &str) -> i32 {
+        //Convert the provided pathname into an absolute path without `.` or `..`
+        //components.
         let truepath = normpath(convpath(path), self);
-        //Walk the file tree to get inode from path
+        //Perfrom a walk down the file tree starting from the root directory to
+        //obtain an inode number of the file whose pathname was specified.
+        //`None` is returned if one of the following occurs while moving down
+        //the tree: 
+        //1. Accessing a child of a non-directory inode 
+        //2. Accessing a child of a nonexistent parent directory
+        //3. Accessing a nonexistent child
+        //4. Accessing an unexpected component, like `.` or `..` directory reference.
+        //In this case, `The file does not exist` error is returned.
+        //Otherwise, a `Some()` option containing the inode number is returned.
         if let Some(inodenum) = metawalk(&truepath) {
-            if let Inode::Dir(ref mut dir) = *(FS_METADATA.inodetable.get_mut(&inodenum).unwrap()) {
-                //increment refcount of new cwd inode to ensure that you can't remove a
-                // directory while it is the cwd of a cage
-                dir.refcount += 1;
+            //A sanity check to make sure that the last component of the
+            //specified path is indeed a directory
+            if let Inode::Dir(ref mut _dir) = *(FS_METADATA.inodetable.get_mut(&inodenum).unwrap())
+            {
+                //Obtain the write lock on the current working directory of the cage
+                //and change it to the new directory
+                let mut cwd_container = self.cwd.write();
+                *cwd_container = interface::RustRfc::new(truepath);
+                0 //chdir has succeeded!;
             } else {
                 return syscall_error(
                     Errno::ENOTDIR,
@@ -2442,15 +2531,6 @@ impl Cage {
                 "the directory referred to in path does not exist",
             );
         }
-        //at this point, syscall isn't an error
-        let mut cwd_container = self.cwd.write();
-
-        //decrement refcount of previous cwd's inode, to allow it to be removed if no
-        // cage has it as cwd
-        decref_dir(&*cwd_container);
-
-        *cwd_container = interface::RustRfc::new(truepath);
-        0 //chdir has succeeded!;
     }
 
     ///##------------------------------------DUP & DUP2 SYSCALLS------------------------------------
diff --git a/src/safeposix/syscalls/sys_calls.rs b/src/safeposix/syscalls/sys_calls.rs
@@ -39,7 +39,7 @@ use super::net_constants::*;
 use super::sys_constants::*;
 use crate::interface;
 use crate::safeposix::cage::{FileDescriptor::*, *};
-use crate::safeposix::filesystem::{decref_dir, metawalk, Inode, FS_METADATA};
+use crate::safeposix::filesystem::{metawalk, Inode, FS_METADATA};
 use crate::safeposix::net::NET_METADATA;
 use crate::safeposix::shm::SHM_METADATA;
 
@@ -496,7 +496,6 @@ impl Cage {
 
         //get file descriptor table into a vector
         let cwd_container = self.cwd.read();
-        decref_dir(&*cwd_container);
 
         //may not be removable in case of lindrustfinalize, we don't unwrap the remove
         // result
diff --git a/src/tests/fs_tests.rs b/src/tests/fs_tests.rs
@@ -343,28 +343,151 @@ pub mod fs_tests {
     }
 
     #[test]
-    pub fn ut_lind_fs_dir_chdir() {
+    pub fn ut_lind_fs_chdir_valid_args() {
         //acquiring a lock on TESTMUTEX prevents other tests from running concurrently,
         // and also performs clean env setup
         let _thelock = setup::lock_and_init();
 
         let cage = interface::cagetable_getref(1);
 
-        //testing the ability to make and change to directories
+        //Testing the ability to make and change to directories
+        //using absolute and relative and `..` reference
 
         assert_eq!(cage.mkdir_syscall("/subdir1", S_IRWXA), 0);
         assert_eq!(cage.mkdir_syscall("/subdir1/subdir2", S_IRWXA), 0);
-        assert_eq!(cage.mkdir_syscall("/subdir1/subdir2/subdir3", 0), 0);
 
-        assert_eq!(cage.access_syscall("subdir1", F_OK), 0);
+        //Changing to a new current working directory, and then obtaining
+        //the current working directory using `getcwd_syscall()` to see
+        //if it was correctly changed
         assert_eq!(cage.chdir_syscall("subdir1"), 0);
+        let mut buf1 = vec![0u8; 9];
+        let bufptr1: *mut u8 = &mut buf1[0];
+        assert_eq!(cage.getcwd_syscall(bufptr1, 9), 0);
+        assert_eq!(std::str::from_utf8(&buf1).unwrap(), "/subdir1\0");
 
-        assert_eq!(cage.access_syscall("subdir2", F_OK), 0);
+        assert_eq!(cage.chdir_syscall("/subdir1/subdir2"), 0);
         assert_eq!(cage.chdir_syscall(".."), 0);
+        let mut buf1 = vec![0u8; 9];
+        let bufptr1: *mut u8 = &mut buf1[0];
+        assert_eq!(cage.getcwd_syscall(bufptr1, 9), 0);
+        assert_eq!(std::str::from_utf8(&buf1).unwrap(), "/subdir1\0");
+
+        assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS);
+        lindrustfinalize();
+    }
+
+    #[test]
+    pub fn ut_lind_fs_chdir_removeddir() {
+        //acquiring a lock on TESTMUTEX prevents other tests from running concurrently,
+        // and also performs clean env setup
+        let _thelock = setup::lock_and_init();
+
+        let cage = interface::cagetable_getref(1);
+
+        //Checking if removing the current working directory
+        //works correctly
+        assert_eq!(cage.mkdir_syscall("/subdir1", S_IRWXA), 0);
+        assert_eq!(cage.mkdir_syscall("/subdir2", S_IRWXA), 0);
+        assert_eq!(cage.chdir_syscall("subdir1"), 0);
+        assert_eq!(cage.rmdir_syscall("/subdir1"), 0);
+        assert_eq!(cage.chdir_syscall("/subdir2"), 0);
+        assert_eq!(cage.chdir_syscall("subdir1"), -(Errno::ENOENT as i32));
+
+        assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS);
+        lindrustfinalize();
+    }
+
+    #[test]
+    pub fn ut_lind_fs_chdir_invalid_args() {
+        //acquiring a lock on TESTMUTEX prevents other tests from running concurrently,
+        //and also performs clean env setup
+        let _thelock = setup::lock_and_init();
+
+        let cage = interface::cagetable_getref(1);
+
+        let flags: i32 = O_TRUNC | O_CREAT | O_RDWR;
+        let filepath = "/TestFile1";
+        let _fd1 = cage.open_syscall(filepath, flags, 0);
+
+        //Checking if passing a regular file pathname correctly
+        //returns `The last component in path is not a directory` error
+        assert_eq!(cage.chdir_syscall("/TestFile1"), -(Errno::ENOTDIR as i32));
+
+        //Checking if a nonexistent pathname correctly
+        //returns `The directory referred to in path does not exist` error.
+        //`/arbitrarypath` is a pathname that does not correspond to any existing
+        //directory pathname.
+        assert_eq!(
+            cage.chdir_syscall("/arbitrarypath"),
+            -(Errno::ENOENT as i32)
+        );
+
+        assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS);
+        lindrustfinalize();
+    }
+
+    #[test]
+    pub fn ut_lind_fs_fchdir_valid_args() {
+        //acquiring a lock on TESTMUTEX prevents other tests from running concurrently,
+        // and also performs clean env setup
+        let _thelock = setup::lock_and_init();
+
+        let cage = interface::cagetable_getref(1);
+
+        //Testing the ability to make and change to directories
+        //using file descriptors
+
+        assert_eq!(cage.mkdir_syscall("/subdir1", S_IRWXA), 0);
+        assert_eq!(cage.mkdir_syscall("/subdir1/subdir2", S_IRWXA), 0);
+
+        //Retrieving a valid directory file descriptor
+        let fd1 = cage.open_syscall("/subdir1", O_RDWR, S_IRWXA);
+        let fd2 = cage.open_syscall("/subdir1/subdir2", O_RDWR, S_IRWXA);
 
+        //Changing to a new current working directory, and then obtaining
+        //the current working directory using `getcwd_syscall()` to see
+        //if it was correctly changed
         assert_eq!(cage.access_syscall("subdir1", F_OK), 0);
-        assert_eq!(cage.chdir_syscall("/subdir1/subdir2/subdir3"), 0);
-        assert_eq!(cage.access_syscall("../../../subdir1", F_OK), 0);
+        assert_eq!(cage.fchdir_syscall(fd1), 0);
+        let mut buf1 = vec![0u8; 9];
+        let bufptr1: *mut u8 = &mut buf1[0];
+        assert_eq!(cage.getcwd_syscall(bufptr1, 9), 0);
+        assert_eq!(std::str::from_utf8(&buf1).unwrap(), "/subdir1\0");
+
+        assert_eq!(cage.access_syscall("subdir2", F_OK), 0);
+        assert_eq!(cage.fchdir_syscall(fd2), 0);
+        let mut buf2 = vec![0u8; 17];
+        let bufptr2: *mut u8 = &mut buf2[0];
+        assert_eq!(cage.getcwd_syscall(bufptr2, 17), 0);
+        assert_eq!(std::str::from_utf8(&buf2).unwrap(), "/subdir1/subdir2\0");
+
+        assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS);
+        lindrustfinalize();
+    }
+
+    #[test]
+    pub fn ut_lind_fs_fchdir_invalid_args() {
+        //acquiring a lock on TESTMUTEX prevents other tests from running concurrently,
+        //and also performs clean env setup
+        let _thelock = setup::lock_and_init();
+
+        let cage = interface::cagetable_getref(1);
+
+        let flags: i32 = O_TRUNC | O_CREAT | O_RDWR;
+        let filepath = "/TestFile1";
+        let fd1 = cage.open_syscall(filepath, flags, 0);
+
+        //Checking if passing a regular file descriptor correctly
+        //returns `The last component in path is not a directory` error
+        assert_eq!(cage.fchdir_syscall(fd1), -(Errno::ENOTDIR as i32));
+
+        //Checking if passing an invalid file descriptor correctly
+        //results in `Invalid file descriptor` error
+        //Since the file corresponding to file descriptor `fd1` is closed,
+        //and no other file is opened after that, `fd1` file descriptor
+        //should be invalid
+        assert_eq!(cage.close_syscall(fd1), 0);
+        assert_eq!(cage.fchdir_syscall(fd1), -(Errno::EBADF as i32));
 
         assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS);
         lindrustfinalize();
