Fcntl_syscall updates #259

ve1nard · 2024-06-07T22:51:45Z

Description

Fixes # (issue)

The following changes include more elaborate comments and new unit tests for fcntl_syscall. Moreover, it was proposed to use "if let" construct instead of "unwrap" to get an entry from a file descriptor table to avoid "panic!" when provided with a file descriptor outside of the allowed range.

Type of change

Small fix for obtaining a file descriptor table entry without "panic!"
More detailed comments for fcntl_syscall
New unit tests for fcntl_syscall

How Has This Been Tested?

To run the tests, we need to run cargo test --lib command inside the safeposix-rust directory.

All the tests are present under this directory: lind_project/src/safeposix-rust/src/tests/fs_tests.rs

Test A - ut_lind_fs_fcntl_valid_args()
Test B - ut_lind_fs_fcntl_invalid_args()
Test C - ut_lind_fs_fcntl_invalid_fd()
Test D - ut_lind_fs_fcntl_dup()

Checklist:

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
Any dependent changes have been added to a pull request and/or merged in other modules (native-client, lind-glibc, lind-project)

JustinCappos

The testing is good. I have a major style comment in the code itself, but much of it looks sound.

src/safeposix/syscalls/fs_calls.rs

JustinCappos · 2024-06-07T23:37:40Z

src/safeposix/syscalls/fs_calls.rs

+        //instead, I propose using the 'if let' construct to be able to report an error to 
+        //the user instead of simply panicking
+        //otherwise, file descriptor table entry is stored in 'checkedfd'
+        if let Ok(checkedfd) = self.get_filedescriptor(fd) {        


So I like the philosophy here, but I think you want to reverse the if statement behavior so that you error out quickly, rather than continually nesting your if statement. This way one can much more easily read the code.

If I write something like "if let Err() = self.get_filedescriptor(fd)", then in the "else", I will have to do pattern matching once again to extract checkedfd, which seems redundant. Should I use match instead?

That would be an option.

I'm not a Rust expert, but code like this looks more "correct" to me. I'm very open to hearing from others, but certainly in Python, C, etc. it is preferred to do it in the style I've written.

JustinCappos · 2024-06-07T23:39:19Z

src/safeposix/syscalls/fs_calls.rs

+                    //currently, O_CLOEXEC is the only defined file descriptor flag, thus only this flag is
+                    //masked when using F_GETFD or F_SETFD
+                    (F_GETFD, ..) => *flags & O_CLOEXEC,
+                    // set the flags but make sure that the flags are valid


I'm not sure how this achieves your goal. Can you say more?

F_GETFD command should return file descriptor flags only, which means that the returned value should not include access mode flags and file status flags, and because O_CLOEXEC is the only implemented file descriptor as of now, we can get it by bit-wise and'ing. Same logic applies to F_SETFD: because O_CLOEXEC is the only existing file descriptor flag, we need to check only it.

JustinCappos · 2024-06-07T23:40:19Z

src/safeposix/syscalls/fs_calls.rs

+                        0 //TO DO: traditional SIGIO behavior
+                    }
+                    (F_SETOWN, arg) if arg >= 0 => {
+                        0 //this would return the PID if positive and the process group if negative,


Would it set this or return it? Why do we do nothing?

When I was talking to Yash, he suggested me that I should not focus on things that are not yet implemented, and that is why I left this part with F_SETOWN and F_GETOWN commands untouched. Should I work on them?

JustinCappos · 2024-06-07T23:40:39Z

src/safeposix/syscalls/fs_calls.rs

+                        0
+                    }
+                    (F_DUPFD, arg) if arg >= 0 => self._dup2_helper(&filedesc_enum, arg, false),
+                    //TO DO: implement. this one is saying get the signals


Which is saying to get the signals? What signals?

JustinCappos · 2024-06-07T23:42:19Z

src/safeposix/syscalls/fs_calls.rs

+                    _ => syscall_error(
+                        Errno::EINVAL,
+                        "fcntl",
+                        "Arguments provided do not match implemented parameters",


Can we list anything else useful like the arguments themselves?

Thank you! I will include a (cmd, arg) tuple in the error message.

JustinCappos · 2024-06-07T23:43:11Z

src/tests/fs_tests.rs

        assert_eq!(cage.fcntl_syscall(filefd, F_GETFL, 0), 2048);

+        //when provided with 'F_GETFD' or 'F_GETFL' command, 'arg' should be ignored, thus even
+        //negative arg values should produce nomal behavior
+        //However, testing results in two errors


Okay, great! So this test will fail now, but after we fix the code later it will pass, right?

This is my bad: everything works now and the test passes. I just forgot to delete this part of the comment.

Co-authored-by: Justin Cappos <[email protected]>

yashaswi2000 · 2024-06-10T20:29:03Z

Please add documentation about the syscall, its arguments, return values and manpage link above the function definition.

JustinCappos · 2024-06-11T15:20:14Z

src/safeposix/syscalls/fs_calls.rs

@@ -1898,87 +1898,126 @@ impl Cage {
    }

    //------------------------------------FCNTL SYSCALL------------------------------------
+
+    //fcntl performs operations, like returning or setting file status flags,


Looks great!

@yashaswi2000 , do you want to have the rustdoc changes be done in these edits or a different set of PRs? I do think that for new PRs, we likely should include them...

I will refactor these first 3 PRs. and any new ones coming in will follow the template I will share with them.

JustinCappos · 2024-06-11T15:21:50Z

src/safeposix/syscalls/fs_calls.rs

+
+                                    if let Some(ins) = &mut sockhandle.innersocket {
+                                        let fcntlret;
+                                        if arg & O_NONBLOCK == O_NONBLOCK {


Deep nesting. Is there a way we can avoid this?

No not easily. The rustfmt makes it looks worse than it is.

I think clippy will suggest a way to change this pattern:

match fd_table_entry_ptr { Err(()) => { syscall_error(Errno::EBADF, "fcntl", "File descriptor is out of range") }, Ok(checkedfd) => { let mut unlocked_fd = checkedfd.write(); ...

into an if let Ok(checkedfd) = fd_table_entry_ptr {...} in at least some cases. Here, it would unfortunately move the error condition to the end, so while this would reduce indentation, it isn't a 100% win...

This seems fine as it for now but may be part of a later refactor.

rennergade · 2024-06-11T19:03:49Z

src/safeposix/syscalls/fs_calls.rs

-                    &mut sockfdobj.flags
-                }
-            };
+                                &mut sockfdobj.flags


remove extra whitespace

…emoved an extra whitespace

… commented out thefailing test

yashaswi2000

LGTM!

rennergade

Approved!

lind added 3 commits June 6, 2024 18:25

updated comments for fnctl_syscall

5775fcd

implemented checking for out-of-bounds file descriptor and tested it

d1bd972

finished the unit tests set for fcntl_syscall

6dbafda

ve1nard requested review from JustinCappos, rennergade, yashaswi2000 and Yaxuan-w June 7, 2024 22:52

JustinCappos requested changes Jun 7, 2024

View reviewed changes

ve1nard and others added 4 commits June 7, 2024 20:40

Update src/safeposix/syscalls/fs_calls.rs

81dcee4

Co-authored-by: Justin Cappos <[email protected]>

changed the code according to the comments

0b6fd76

updating the changes

477f229

added a 'TO DO' comment for F_GETOWN and F_SETOWN commands

0058a37

added the description for 'fcntl' syscall

0efa0f5

ve1nard requested a review from JustinCappos June 10, 2024 21:05

yashaswi2000 mentioned this pull request Jun 11, 2024

Documentation and testing enhancement for RustPosix #260

Open

JustinCappos approved these changes Jun 11, 2024

View reviewed changes

JustinCappos reviewed Jun 11, 2024

View reviewed changes

rennergade reviewed Jun 11, 2024

View reviewed changes

src/safeposix/syscalls/fs_calls.rs Outdated

&mut sockfdobj.flags

}

};

&mut sockfdobj.flags

Copy link

Contributor

rennergade Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra whitespace

reverted the part with get_filedescriptor to the original state and r…

b4d08c0

…emoved an extra whitespace

ve1nard requested a review from rennergade June 13, 2024 20:34

added a 'BUG' prefix for the 'out -f -ounds ile descriptorr issue and…

a343f13

… commented out thefailing test

yashaswi2000 approved these changes Jun 14, 2024

View reviewed changes

removed the failing test

ddbf304

rennergade approved these changes Jun 14, 2024

View reviewed changes

rennergade merged commit c54156f into develop Jun 14, 2024
1 of 2 checks passed

Fcntl_syscall updates #259

Fcntl_syscall updates #259

Uh oh!

Conversation

ve1nard commented Jun 7, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

JustinCappos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JustinCappos Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ve1nard Jun 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashaswi2000 commented Jun 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yashaswi2000 left a comment

Choose a reason for hiding this comment

Uh oh!

rennergade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JustinCappos Jun 8, 2024 •

edited

Loading

ve1nard Jun 8, 2024 •

edited

Loading