diff --git a/src/safeposix/syscalls/fs_calls.rs b/src/safeposix/syscalls/fs_calls.rs index 320278498..99eedb809 100644 --- a/src/safeposix/syscalls/fs_calls.rs +++ b/src/safeposix/syscalls/fs_calls.rs @@ -2607,9 +2607,15 @@ impl Cage { None => {} } if dir_inode_obj.linkcount == 2 && dir_inode_obj.refcount == 0 { - //removing the file from the metadata - FS_METADATA.inodetable.remove(&inodenum); + //The reference to the inode has to be dropped to avoid + //deadlocking because the remove() method will need to + //acquire a reference to the same inode from the + //filesystem's inodetable. + //The inodetable represents a Rust DashMap that deadlocks + //when trying to get a reference to its entry while holding any sort + //of reference into it. drop(inodeobj); + FS_METADATA.inodetable.remove(&inodenum); log_metadata(&FS_METADATA, inodenum); } } @@ -3382,27 +3388,83 @@ impl Cage { 0 } - //------------------RMDIR SYSCALL------------------ + /// ### Description + /// + /// The `rmdir_syscall()` deletes a directory whose name is given by `path`. + /// The directory shall be removed only if it is an empty directory. + /// + /// ### Arguments + /// + /// The `rmdir_syscall()` accepts one argument: + /// * `path` - the path to the directory that shall be removed. It can be + /// either + /// relative or absolute (symlinks are not supported). + /// + /// ### Returns + /// + /// Upon successful completion, 0 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 + /// + /// * `ENOENT` - `path` is an empty string or names a nonexistent directory + /// * `EBUSY` - `path` names a root directory that cannot be removed + /// * `ENOEMPTY` - `path` names a non-empty directory, + /// * `EPERM` - the directory to be removed or its parent directory + /// does not allow write permission + /// * `ENOTDIR` - `path` is not a directory + /// Other errors, like `EACCES`, `EINVAL`, etc. are not supported. + /// + /// ### Panics + /// A panic occurs when the directory to be removed does not have `S_IFDIR"` + /// (directory file type flag) set or when parent inode is not a directory. + /// + /// To learn more about the syscall, error values, etc., + /// see [rmdir(3)](https://linux.die.net/man/3/rmdir) pub fn rmdir_syscall(&self, path: &str) -> i32 { if path.len() == 0 { - return syscall_error(Errno::ENOENT, "rmdir", "Given path is null"); + return syscall_error(Errno::ENOENT, "rmdir", "Given path is an empty string"); } + //Convert the provided pathname into an absolute path without `.` or `..` + //components. let truepath = normpath(convpath(path), self); - // try to get inodenum of input path and its parent + //Perfrom a walk down the file tree starting from the root directory to + //obtain an inode number of the file whose pathname was specified and + //its parent directory's inode. match metawalkandparent(truepath.as_path()) { (None, ..) => syscall_error(Errno::ENOENT, "rmdir", "Path does not exist"), - (Some(_), None) => { - // path exists but parent does not => path is root dir - syscall_error(Errno::EBUSY, "rmdir", "Cannot remove root directory") - } + //The specified directory exists, but its parent does not, + //which means it is a root directory that cannot be removed + (Some(_), None) => syscall_error(Errno::EBUSY, "rmdir", "Cannot remove root directory"), (Some(inodenum), Some(parent_inodenum)) => { + //If the parent directory of the directory that shall be removed + //doesn't allow write permission, the removal cannot be performed + if let Inode::Dir(ref mut parent_dir) = + *(FS_METADATA.inodetable.get_mut(&parent_inodenum).unwrap()) + { + // check if parent directory has write permissions + if parent_dir.mode as u32 & (S_IWOTH | S_IWGRP | S_IWUSR) == 0 { + return syscall_error( + Errno::EPERM, + "rmdir", + "Parent directory does not have write permission", + ); + } + } + //Getting a mutable reference to an inode struct that corresponds to + //the directory that shall be removed let mut inodeobj = FS_METADATA.inodetable.get_mut(&inodenum).unwrap(); match &mut *inodeobj { - // make sure inode matches a directory + //A sanity check to make sure the inode matches a directory Inode::Dir(ref mut dir_obj) => { + //When a new empty directory is created, its linkcount + //is set to 3. Thus, any empty directory should have a + //linkcount of 3. Otherwise, it is a non-empty directory + //that cannot be removed if dir_obj.linkcount > 3 { return syscall_error( Errno::ENOTEMPTY, @@ -3410,36 +3472,85 @@ impl Cage { "Directory is not empty", ); } + //A sanity check to make sure that the correct directory + //file type flag is set if !is_dir(dir_obj.mode) { panic!("This directory does not have its mode set to S_IFDIR"); } - // check if dir has write permission + //The directory to be removed should allow write permission if dir_obj.mode as u32 & (S_IWOTH | S_IWGRP | S_IWUSR) == 0 { return syscall_error( Errno::EPERM, "rmdir", - "Directory does not have write permission", + "Directory does not allow write permission", ); } + //The directory cannot be removed if one or more processes + //have the directory open, which corresponds to a non-zero + //reference count let remove_inode = dir_obj.refcount == 0; + //Any new empty directory has a linkcount of 3. + //If the refcount of the directory is 0, it means + //that there are no open file descriptors for that directory, + //and it can be safely removed both from the filesystem + //and the parent directory's inode table. + //However, if there exists an open file descriptor for that + //directory, it cannot be removed from the filesystem because + //otherwise, the process that has the directory open + //will end up calling `close_syscall()` on a nonexistent directory. + //At the same time, after `rmdir_syscall()` is called on the directory, + //no new files can be created inside it, even if the directory is open + //by some process. To prevent creating new files inside the directory, + //we delete its entry from the parent directory's inode table. + //This way, in case there is an open file descriptor for the directory + //to be removed, by deleting the directory's entry from its parent + //directory's inode table and keeping the directory's entry in the + //filesystem table, we both disallow the creation of any files inside + //that directory and prevent the process that has the directory + //open from closing a nonexistent directory. + //Setting the directory's linkcount as 2 works as a flag for + //the `close_syscall()` to mark the directory that needs to be + //removed from the filesystem when its last open file descriptor + //is closed because it could not be removed at the time of calling + //`rmdir_syscall()` because of some open file descriptor. if remove_inode { dir_obj.linkcount = 2; - } // linkcount for an empty directory after rmdir must be 2 + } + + //The mutable reference to the inode has to be dropped because + //remove_from_parent_dir() method will need to acquire an immutable + //reference to the parent directory's inode from the filesystem's + //inodetable. The inodetable represents a Rust DashMap that deadlocks + //when trying to get a reference to its entry while holding any sort + //of reference into it. drop(inodeobj); + //`remove_from_parent_dir()` helper function returns 0 if an + //entry corresponding to the specified directory was + //successfully removed from the filename-inode dictionary + //of its parent. + //If the parent directory does not allow write permission, + //`EPERM` is returned. + //As a sanity check, if the parent inode specifies a + //non-directory type, the funciton panics let removal_result = Self::remove_from_parent_dir(parent_inodenum, &truepath); if removal_result != 0 { return removal_result; } - // remove entry of corresponding inodenum from inodetable + //Remove entry of corresponding inodenum from the filesystem + //inodetable if remove_inode { FS_METADATA.inodetable.remove(&inodenum).unwrap(); } - + //Log is used to store all the changes made to the filesystem. After + //the cage is closed, all the collected changes are serialized and + //the state of the underlying filesystem is persisted. This allows us + //to avoid serializing and persisting filesystem state after every + //`rmdir_syscall()`. log_metadata(&FS_METADATA, parent_inodenum); log_metadata(&FS_METADATA, inodenum); 0 // success diff --git a/src/tests/fs_tests.rs b/src/tests/fs_tests.rs index 873cdb5da..4ba99bc41 100644 --- a/src/tests/fs_tests.rs +++ b/src/tests/fs_tests.rs @@ -1547,15 +1547,190 @@ pub mod fs_tests { } #[test] - pub fn ut_lind_fs_rmdir() { + pub fn ut_lind_fs_rmdir_normal() { + //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); + + //We create a new parent directory `/parent_dir` + //and its child directory '/parent_dir/dir` both + //with the required write permission flags, thus + //calling `rmdir_syscall()`on the child directory + //should result in a normal behavior + let path = "/parent_dir/dir"; + assert_eq!(cage.mkdir_syscall("/parent_dir", S_IRWXA), 0); + assert_eq!(cage.mkdir_syscall(path, S_IRWXA), 0); + assert_eq!(cage.rmdir_syscall(path), 0); + //To check if the child directory was successfully + //removed, we call `open_syscall()` on it, and see + //if it correctly returns `Path does not exist` error + assert_eq!( + cage.open_syscall(path, O_TRUNC, S_IRWXA), + -(Errno::ENOENT as i32) + ); + + assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS); + lindrustfinalize(); + } + + #[test] + pub fn ut_lind_fs_rmdir_empty_path() { //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); + //Trying to remove a directory by providing an empty string + //should return `Given path is an empty string` error + assert_eq!(cage.rmdir_syscall(""), -(Errno::ENOENT as i32)); + + assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS); + lindrustfinalize(); + } + + #[test] + pub fn ut_lind_fs_rmdir_nonexist_dir() { + //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); + + //We create a new parent directory `/parent_dir` + //However, we never create its child directory + //'/parent_dir/dir`, thus calling `rmdir_syscall()` + //on this child directory should return + //`Path does not exist` error let path = "/parent_dir/dir"; assert_eq!(cage.mkdir_syscall("/parent_dir", S_IRWXA), 0); + assert_eq!(cage.rmdir_syscall(path), -(Errno::ENOENT as i32)); + + assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS); + lindrustfinalize(); + } + + #[test] + pub fn ut_lind_fs_rmdir_root() { + //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); + + //Trying to remove the root directory should return + //`Cannot remove root directory` error + assert_eq!(cage.rmdir_syscall("/"), -(Errno::EBUSY as i32)); + + assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS); + lindrustfinalize(); + } + + #[test] + pub fn ut_lind_fs_rmdir_nonempty_dir() { + //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); + + //We create a new parent directory `/parent_dir` and + //its child directory '/parent_dir/dir`, thus calling `rmdir_syscall()` + //on the parent directory should return `Directory is not empty` error + let path = "/parent_dir/dir"; + assert_eq!(cage.mkdir_syscall("/parent_dir", S_IRWXA), 0); + assert_eq!(cage.mkdir_syscall(path, S_IRWXA), 0); + assert_eq!( + cage.rmdir_syscall("/parent_dir"), + -(Errno::ENOTEMPTY as i32) + ); + + assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS); + lindrustfinalize(); + } + + #[test] + pub fn ut_lind_fs_rmdir_nowriteperm_child_dir() { + //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); + + //We create a new parent directory `/parent_dir` with all write permission + //flags and its child directory '/parent_dir/dir` without any write + //permision flags, thus calling `rmdir_syscall()`on the child directory + //should return `Directory does not allow write permission` error + //because the directory cannot be removed if it does not allow + //write permission + let path = "/parent_dir/dir"; + assert_eq!(cage.mkdir_syscall("/parent_dir", S_IRWXA), 0); + assert_eq!(cage.mkdir_syscall(path, 0), 0); + assert_eq!(cage.rmdir_syscall(path), -(Errno::EPERM as i32)); + + assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS); + lindrustfinalize(); + } + + #[test] + pub fn ut_lind_fs_rmdir_nowriteperm_parent_dir() { + //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); + + //We create a new parent directory `/parent_dir` with all write permission + //flags (to be able to create its child directory) and its child directory + //'/parent_dir/dir` with all write permision flags. + let path = "/parent_dir/dir"; + assert_eq!(cage.mkdir_syscall("/parent_dir", S_IRWXA), 0); + assert_eq!(cage.mkdir_syscall(path, S_IRWXA), 0); + //Now, we change the parent directories write permission flags to 0, + //thus calling `rmdir_syscall()`on the child directory + //should return `Directory does not allow write permission` error + //because the directory cannot be removed if its parent directory + //does not allow write permission + assert_eq!( + cage.chmod_syscall("/parent_dir", S_IRUSR | S_IRGRP | S_IROTH), + 0 + ); + assert_eq!(cage.rmdir_syscall(path), -(Errno::EPERM as i32)); + + assert_eq!(cage.exit_syscall(EXIT_SUCCESS), EXIT_SUCCESS); + lindrustfinalize(); + } + + #[test] + //BUG: + //The correct behavior of the `rmdir_syscall()` when called on a directory + //whose path includes a component that does not allow search permission + //(the read flag) is to return with `EACCES` error. + //However, the `metawalkandparent())` helper function used + //to retrieve the inodes of the directory to be removed and its parent + //directory does not check for search permission. Thus, the following test + //will not return any errors and run normally even though the parent + //directory does not grand search permission. + pub fn ut_lind_fs_search_permission_bug_with_rmdir() { + //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); + + //Creating the parent directory that does not allow search permission + //by excluding any read flags and specifying only write flags + //to be able to delete the child directory. + let path = "/parent_dir/dir"; + assert_eq!( + cage.mkdir_syscall("/parent_dir", S_IWUSR | S_IWGRP | S_IWOTH), + 0 + ); + //Creating the child directory with all the required flags + //and then deleting it. Because of the bug described above, + //removing the directory will not return any errors. assert_eq!(cage.mkdir_syscall(path, S_IRWXA), 0); assert_eq!(cage.rmdir_syscall(path), 0); diff --git a/src/tools/lib_fs_utils.rs b/src/tools/lib_fs_utils.rs index a21ce9be0..39041afa4 100644 --- a/src/tools/lib_fs_utils.rs +++ b/src/tools/lib_fs_utils.rs @@ -308,6 +308,17 @@ pub fn lind_deltree(cage: &Cage, path: &str) { cage.unlink_syscall(path); return; } else { + //Parent directory's write flag should be set before + //iterating through the child directories, so that they + //could be removed. + //This is important for unit tests where a non empty + //parent directory ends up having write flags off at + //the end of the test. The next unit test's call to + //`lind_deltree()` will not be able to remove the child + //directories if the parent directory's write flags + //are not set before iterating through the child + //directories. + cage.chmod_syscall(path, S_IRWXA); //remove all children recursively visit_children(cage, path, None, |childcage, childpath, isdir, _| { if isdir { @@ -316,9 +327,7 @@ pub fn lind_deltree(cage: &Cage, path: &str) { childcage.unlink_syscall(childpath); } }); - //remove specified directory now that it is empty - cage.chmod_syscall(path, S_IRWXA); cage.rmdir_syscall(path); } } else {