From b8a6fabcff933d379fea38916551711ded8da491 Mon Sep 17 00:00:00 2001 From: Pranav Bhatt <42911524+pranav-bhatt@users.noreply.github.com> Date: Wed, 21 Aug 2024 00:23:59 +0530 Subject: [PATCH] Description, Comments and tests for `access_syscall` & `rename_syscall` (#319) * added descriptions and tests for access and rename syscalls * tweaked description * addressed comments * added BUG comments * fixes for failing tests --------- Co-authored-by: Nicholas Renner Co-authored-by: Yashaswi Makula --- src/safeposix/filesystem.rs | 2 +- src/safeposix/syscalls/fs_calls.rs | 91 +++++++++++++++++++++++++++ src/tests/fs_tests.rs | 99 ++++++++++++++++++++++++++++-- 3 files changed, 185 insertions(+), 7 deletions(-) diff --git a/src/safeposix/filesystem.rs b/src/safeposix/filesystem.rs index 8969e5fca..1d99952ae 100644 --- a/src/safeposix/filesystem.rs +++ b/src/safeposix/filesystem.rs @@ -395,7 +395,7 @@ pub fn create_log() { logobj.replace(log_mapobj); } -// Serialize New Metadata to CBOR, write to logfile +/// Serialize New Metadata to CBOR, write to logfile pub fn log_metadata(metadata: &FilesystemMetadata, inodenum: usize) { let serialpair: (usize, Option<&Inode>); let entrybytes; diff --git a/src/safeposix/syscalls/fs_calls.rs b/src/safeposix/syscalls/fs_calls.rs index 06dc2d0d5..e81d21fd8 100644 --- a/src/safeposix/syscalls/fs_calls.rs +++ b/src/safeposix/syscalls/fs_calls.rs @@ -2815,12 +2815,53 @@ impl Cage { } //------------------------------------ACCESS SYSCALL------------------------------------ + /// ### Description + /// + /// `access_syscall` checks the accessibility of the file specified by + /// `path` according to the given `amode`. The assumption while running this + /// command is that the current user owns the file. + /// + /// ### Arguments + /// + /// It accepts two parameters: + /// * `path` - A string slice that specifies the file path for which + /// accessibility is to be checked. + /// * `amode` - A bitmask value specifying the accessibility check to be + /// performed. It can be a combination of the following constants: + /// * `R_OK` - Check for read permission. + /// * `W_OK` - Check for write permission. + /// * `X_OK` - Check for execute (search) permission. + /// + /// ### Returns + /// + /// For a successful call, the return value will be 0, indicating that the + /// requested access is allowed. On error, a negative errno is returned to + /// indicate the error. + /// + /// ### Errors + /// + /// * `ENOENT` - The file specified by `path` does not exist or the path is + /// invalid. + /// * `EACCES` - The requested access would be denied to the file due to + /// insufficient permissions. + /// + /// ### Panics + /// + /// * There are no known panics in this system call. + /// + /// For more detailed description of all the commands and return values, + /// refer to the access syscall man page [here](https://man7.org/linux/man-pages/man2/access.2.html). pub fn access_syscall(&self, path: &str, amode: u32) -> i32 { let truepath = normpath(convpath(path), self); //Walk the file tree to get inode from path if let Some(inodenum) = metawalk(truepath.as_path()) { + // BUG: We don't support F_OK as a valid amode flag, which when passed we need + // to check only whether the file exists or not + + // will not panic since check for existence in table already happened in + // metawalk let inodeobj = FS_METADATA.inodetable.get(&inodenum).unwrap(); //Get the mode bits if the type of the inode is sane @@ -2835,13 +2876,19 @@ impl Cage { //Construct desired access bits (i.e. 0777) based on the amode parameter let mut newmode: u32 = 0; + // if we are checking for execute rights if amode & X_OK == X_OK { + // update newmode with S_IXUSR flags newmode |= S_IXUSR; } + // if we are checking for write rights if amode & W_OK == W_OK { + // update newmode with S_IWUSR flags newmode |= S_IWUSR; } + // if we are checking for read rights if amode & R_OK == R_OK { + // update newmode with S_IRUSR flags newmode |= S_IRUSR; } @@ -2857,6 +2904,7 @@ impl Cage { ) } } else { + // if incase the file isn't found syscall_error( Errno::ENOENT, "access", @@ -4654,6 +4702,38 @@ impl Cage { } //------------------RENAME SYSCALL------------------ + /// ### Description + /// + /// `rename_syscall` renames a file or directory specified by `oldpath` to + /// `newpath`. + /// + /// ### Arguments + /// + /// It accepts two parameters: + /// * `oldpath` - A string slice that specifies the current name and + /// location of the file or directory. + /// * `newpath` - A string slice that specifies the new name and location + /// for the file or directory. + /// + /// ### Returns + /// + /// For a successful call, the return value will be 0, indicating that the + /// rename operation was successful. On error, a negative errno is returned + /// to indicate the error. + /// + /// ### Errors + /// + /// * `ENOENT` - The `oldpath` or `newpath` is null, i.e. not provided. + /// * `EEXIST` - The `oldpath` does not exist. + /// * `EBUSY` - Cannot rename the root directory. + /// * `EOPNOTSUPP` - Cannot move the file or directory to another directory. + /// + /// ### Panics + /// + /// * There are no known panics in this function. + /// + /// For more detailed description of all the commands and return values, + /// refer to the rename syscall man page [here](https://man7.org/linux/man-pages/man2/rename.2.html). pub fn rename_syscall(&self, oldpath: &str, newpath: &str) -> i32 { if oldpath.len() == 0 { @@ -4676,7 +4756,12 @@ impl Cage { // make sure file is not moved to another dir // get inodenum for parent of new path let (_, new_par_inodenum) = metawalkandparent(true_newpath.as_path()); + + // BUG: the rename according to the spec supports moving of files from one + // parent to another, so the below behavior is not as per spec + // check if old and new paths share parent + // if they don't, then that is not allowed if new_par_inodenum != Some(parent_inodenum) { return syscall_error( Errno::EOPNOTSUPP, @@ -4685,7 +4770,9 @@ impl Cage { ); } + // get parent directory inode object let pardir_inodeobj = FS_METADATA.inodetable.get_mut(&parent_inodenum).unwrap(); + if let Inode::Dir(parent_dir) = &*pardir_inodeobj { // add pair of new path and its inodenum to filename-inode dict parent_dir.filename_to_inode_dict.insert( @@ -4707,7 +4794,11 @@ impl Cage { .unwrap() .to_string(), ); + + // drop the ref to the parent dir inode object drop(pardir_inodeobj); + + // log and update metadata log_metadata(&FS_METADATA, parent_inodenum); } NET_METADATA.domsock_paths.insert(true_newpath); diff --git a/src/tests/fs_tests.rs b/src/tests/fs_tests.rs index 7e87ec0fc..36234be39 100644 --- a/src/tests/fs_tests.rs +++ b/src/tests/fs_tests.rs @@ -4480,9 +4480,12 @@ pub mod fs_tests { let mut statdata = StatData::default(); // test out whether an error is output for a non existent fd (1000) - // (ENOENT[-2]) + // (EBADF[-9]) let non_existent_fd = 1000; - assert_eq!(cage.fstat_syscall(non_existent_fd, &mut statdata), -9); + assert_eq!( + cage.fstat_syscall(non_existent_fd, &mut statdata), + syscall_error(Errno::EBADF, "fstat", "test_failure") + ); // setting up directory inode object '/tmp' for testing fstat_syscall with a // directory @@ -4524,7 +4527,10 @@ pub mod fs_tests { assert_eq!(cage.bind_syscall(socketfd, &socket), 0); // Errno::EOPNOTSUPP : -95 - assert_eq!(cage.fstat_syscall(socketfd, &mut statdata), -95); + assert_eq!( + cage.fstat_syscall(socketfd, &mut statdata), + syscall_error(Errno::EOPNOTSUPP, "fstat", "test_failure") + ); // Clean up assert_eq!(cage.close_syscall(socketfd), 0); @@ -4548,7 +4554,7 @@ pub mod fs_tests { // (ENOENT[-2]) assert_eq!( cage.statfs_syscall("non_existent_file_path", &mut fsdata), - syscall_error(Errno::ENOENT, "stat", "test_failure") + syscall_error(Errno::ENOENT, "statfs", "test_failure") ); // setting up inode object "/tmp/generic" for testing statfs_syscall @@ -4576,7 +4582,7 @@ pub mod fs_tests { let non_existent_fd = 1000; assert_eq!( cage.fstatfs_syscall(non_existent_fd, &mut fsdata), - syscall_error(Errno::EBADF, "stat", "test_failure") + syscall_error(Errno::EBADF, "fstatfs", "test_failure") ); // setting up generic inode object "/tmp/generic" for testing fstat_syscall with @@ -4600,7 +4606,7 @@ pub mod fs_tests { // Errno::EBADF : -9 assert_eq!( cage.fstatfs_syscall(socketfd, &mut fsdata), - syscall_error(Errno::EBADF, "stat", "test_failure") + syscall_error(Errno::EBADF, "fstatfs", "test_failure") ); // Clean up @@ -4611,4 +4617,85 @@ pub mod fs_tests { lindrustfinalize(); return; } + + #[test] + pub fn ut_lind_fs_access_syscall_tests() { + // 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); + + // test out whether an error is output for a non existent file path + // (ENOENT[-2]) + assert_eq!( + cage.access_syscall("non_existent_file_path", S_IRWXA), + syscall_error(Errno::ENOENT, "access", "test_failure") + ); + + // setting up inode object "/tmp/generic" for testing statfs_syscall + let generic_path = "/tmp/generic"; + let creat_fd = cage.creat_syscall(generic_path, S_IRWXA); + assert!(creat_fd > 0); + // set mode to some value + assert_eq!(cage.access_syscall(generic_path, S_IRWXA), 0); + + // change mode + assert_eq!(cage.chmod_syscall(generic_path, S_IRGRP), 0); + assert_eq!( + cage.access_syscall(generic_path, S_IRWXA), + syscall_error(Errno::EACCES, "access", "incorrect access") + ); + + lindrustfinalize(); + return; + } + + #[test] + pub fn ut_lind_fs_rename_syscall_tests() { + // 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); + + // test out whether an error is output for a non existent file path + // (EEXIST[-17]) + assert_eq!( + cage.rename_syscall("non_existent_file_path", "non-existent-target"), + syscall_error(Errno::EEXIST, "rename", "test_failure") + ); + + // empty inputs for rename + assert_eq!( + cage.rename_syscall("", ""), + syscall_error(Errno::ENOENT, "rename", "empty inputs") + ); + + // root path provided + assert_eq!( + cage.rename_syscall("/", "rename-to-this"), + syscall_error(Errno::EBUSY, "rename", "cant deal with root") + ); + + // setting up inode object "/tmp/generic" for testing statfs_syscall + let generic_path = "/tmp/generic"; + let creat_fd = cage.creat_syscall(generic_path, S_IRWXA); + assert!(creat_fd > 0); + + // try to rename to different parent + assert_eq!( + cage.rename_syscall("/tmp", generic_path), + syscall_error(Errno::EOPNOTSUPP, "rename", "cant move directories") + ); + + // normal rename + assert_eq!(cage.rename_syscall("/tmp/generic", "/tmp/generic1"), 0); + + // clean up + cage.close_syscall(creat_fd); + + lindrustfinalize(); + return; + } }