Convert common syscalls to safe I/O types #1863

SUPERCILEX · 2022-11-13T07:03:47Z

Works towards #1750. Fixes #1850.

I went the lazy route and pulled the plug... trying to feature gate everything seemed way too complicated.

asomers · 2022-12-03T23:43:53Z

There's a lot of stuff here. Could you please annotate #1750 to say which modules you're converting?

src/unistd.rs

Signed-off-by: Alex Saveau <[email protected]>

1908: Move some pure formatting changes out of #1863 r=asomers a=SUPERCILEX Co-authored-by: Alex Saveau <[email protected]>

SUPERCILEX · 2022-12-05T00:32:45Z

Wow that was painful. 😅 I think this is finally ready for review.

asomers

I haven't reviewed the tests yet. But the src changes look mostly good. The one big problem I see is that you removed the Option from all of the *at functions. I don't think we want to do that.

src/dir.rs

src/fcntl.rs

src/lib.rs

src/pty.rs

asomers · 2022-12-05T04:21:53Z

src/unistd.rs

@@ -438,37 +452,43 @@ pub fn dup(oldfd: RawFd) -> Result<RawFd> {
 /// specified fd instead of allocating a new one.  See the man pages for more
 /// detail on the exact behavior of this function.
 #[inline]
-pub fn dup2(oldfd: RawFd, newfd: RawFd) -> Result<RawFd> {
-    let res = unsafe { libc::dup2(oldfd, newfd) };
+pub fn dup2<Fd: AsFd>(oldfd: &Fd, newfd: &mut OwnedFd) -> Result<()> {


Clever, taking OwnedFd by borrow. But what if the user wants to supply an integer that doesn't correspond to any currently open file? That was possible with the old interface.

Dang, you're right. Will fix.

I couldn't figure out how to let you both pass in an fd by reference and by value, so I ended up adding dup*_raw variants.

so I ended up adding dup*_raw variants.

This is something unfortunate, but I think this unfortunate approach is the only way to do it, rustix does not provide something similar to unsafe fn dup2_raw(), so one cannot specify a fd that is still not open.

And interestingly, rustix provides 3 helper functions to use dup2 with Stdin, Stdout and Stderr: https://docs.rs/rustix/latest/rustix/stdio/fn.dup2_stdin.html

src/unistd.rs

SUPERCILEX · 2022-12-08T18:47:10Z

Also at @asomers any chance this PR can be prioritized over the other I/O safety ones? Otherwise I'm going to be stuck constantly resolving merge conflicts.

asomers

There's another possibility for the *at functions. The cap-std crate treats them as methods on an already-created file descriptor object, like this:

let etc = Dir::open_ambient_dir("/etc", ambient_authority())?;
let passwd = etc.open("passwd")?;

I think that's better than any of the proposals here. The only problems with cap-std are that it's too big. It pulls in too many dependencies, and it tries to provide a platform-independent abstraction layer. Should we do something similar?

src/lib.rs

1913: feat: I/O safety for 'sys/inotify' r=asomers a=SteveLauC #### What this PR does: 1. Changes the `fd` field of `struct Inotify` from `RawFd` to `OwnedFd` 2. Changes the interfaces of functions in the `impl Inotify {}` > The type of `self` changes from `Self` to `&mut Self`. From: ```rust pub fn add_watch<P: ?Sized + NixPath>( self, path: &P, mask: AddWatchFlags, ) -> Result<WatchDescriptor> pub fn rm_watch(self, wd: WatchDescriptor) -> Result<()> pub fn read_events(self) -> Result<Vec<InotifyEvent>> ``` To: ```rust pub fn add_watch<P: ?Sized + NixPath>( &mut self, path: &P, mask: AddWatchFlags, ) -> Result<WatchDescriptor> pub fn rm_watch(&mut self, wd: WatchDescriptor) -> Result<()> pub fn read_events(&mut self) -> Result<Vec<InotifyEvent>> ``` In the previous implementation, these functions can take `self` by value as `struct Inotify` [was `Copy`](https://docs.rs/nix/latest/nix/sys/inotify/struct.Inotify.html#impl-Copy-for-Inotify). With the changes in `1` applied, `struct Inotify` is no longer `Copy`, so we have to take `self` by reference. ------- Blocks until the merge of #1863 as this PR needs `read(2)` to be I/O-safe. 1926: feat: I/O safety for 'sys/sendfile' r=asomers a=SteveLauC #### What this PR does: 1. Adds I/O safety for module `sys/sendfile`. 1927: feat: I/O safety for 'sys/statvfs' r=asomers a=SteveLauC #### What this PR does: 1. Adds I/O safety for module `sys/statvfs`. 1931: feat: I/O safety for 'sys/uid' & 'sched' r=asomers a=SteveLauC #### What this PR does: Adds I/O safety for modules: 1. `sys/uio` 2. `sched` 1933: feat: I/O safety for 'sys/timerfd' r=asomers a=SteveLauC #### What this PR does: 1. Adds I/O safety for module `sys/timerfd`. Co-authored-by: Steve Lau <[email protected]>

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2022-12-10T04:59:17Z

There's another possibility for the *at functions. The cap-std crate treats them as methods on an already-created file descriptor object

I had considered this a while back and think the conclusion was that I didn't like it because it'd pollute the i32 namespace. Now it would pollute AsFds (which is definitely better than all i32s, but still). Over at rustix, we're talking about potentially having a DirFd type to also prevent using FDs opened for files instead of directories, but I need to do some more research. That would combine nicely with extension methods.

Not sure where that leaves this PR... maybe we can keep AsDirFd and punt the discussion for now or I could try pulling the *at changes out of this PR?

1935: Formatting only changes for #1928 and #1863 r=asomers a=SUPERCILEX Co-authored-by: Alex Saveau <[email protected]>

asomers · 2022-12-10T18:14:15Z

There's another possibility for the *at functions. The cap-std crate treats them as methods on an already-created file descriptor object

I had considered this a while back and think the conclusion was that I didn't like it because it'd pollute the i32 namespace. Now it would pollute AsFds (which is definitely better than all i32s, but still). Over at rustix, we're talking about potentially having a DirFd type to also prevent using FDs opened for files instead of directories, but I need to do some more research. That would combine nicely with extension methods.

Not sure where that leaves this PR... maybe we can keep AsDirFd and punt the discussion for now or I could try pulling the *at changes out of this PR?

Yeah, I wasn't thinking about adding a method to AsFd, but about adding a newtype for open directories. However, that would be suboptimal on Illumos, where you can use openat on a regular file to edit extended attributes. I see that you're a Rustix contributor too. So I've got to ask, why use both Nix and Rustix? I've generally thought of them as providing equivalent functionality. And also, why bother with both?

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX · 2022-12-10T22:31:24Z

Yeah, I wasn't thinking about adding a method to AsFd, but about adding a newtype for open directories.

Gotya, cool. I'm planning on playing around with that tomorrow. Regarding Illumos, I'd prefer doing the nice thing for everybody else and adding a cfg exception for them.

So I've got to ask, why use both Nix and Rustix?

A month ago, I just used Nix, but over the past few weeks, I've fully migrated over to rustix for the perf improvements of raw syscalls. So why stick around? Originally, it was because I had open PRs here and it felt wrong to close everything and be like, "So long, suckers!" 😆 Anyway, now I'm sticking around because between you and Dan we're generating some interesting ideas that I don't think would have come about without the cross pollination.

notgull · 2022-12-11T02:22:44Z

This smells like it should be a newtype to me. Make it implement AsFd or From and it should work.

ids1024 · 2022-12-16T04:11:50Z

src/unistd.rs

+}
+
+#[inline]
+pub fn dup2_raw<Fd1: AsFd, Fd2: AsRawFd + IntoRawFd>(


If this operates on a RawFd it should be unsafe. Otherwise it could be passed a fd corresponding to a file open elsewhere, and break soundness assumptions in the code using it.

No, that's why it requires IntoRawFd. If the call fails, you had to pass in your FD by value which means it gets dropped inside dup2_raw. If it succeeds, we give you back the same fd. Of course if you don't want the safety stuff, then you pass in a RawFd and IntoRawFd will do nothing.

In the current Rust version RawFd implements AsRawFd and IntoRawFd, so this API can easily be used with an arbitrary integer. So you could for instance call close(dup2_raw(some_fd, 0).unwrap()). Allowing safe code to close stdin or any other fd. This shouldn't be possible in safe code without an OwnedFd.

This function is only safe if the caller either owns fd2, or knows the fd isn't used.

So I think the function should be an unsafe fn, with a doc comment describing when it is safe.

Maybe? When to require unsafe is unclear IMO. BTW, your example can just be close(0)—no need for dup. I think our goal should be to not break BorrowedFd or OwnedFd. If you want to do weird stuff with RawFd, then I feel like that's up to you.

https://rust-lang.github.io/rfcs/3128-io-safety.html

Functions accepting arbitrary raw I/O handle values (RawFd, RawHandle, or RawSocket) should be unsafe if they can lead to any I/O being performed on those handles through safe APIs.

Basically any function that can operate on a RawFd should be unsafe. So to conform to IO safety, close should either be unsafe, or it should take a OwnedFd and consume it.

This is definitely awkward for a crate like nix though, since conforming to this necessitates breaking changes to basically all of the API. Perhaps these functions in nix should have been unsafe already, but ideally Rust would have had clear rules about IO safety before 1.0.

This is definitely awkward for a crate like nix though

Yeah, I think that's the crux of the matter. Nix is trying to be more low-level than the stdlib, so requiring unsafe is probably going to be annoying. I also think you can argue that we're following the definition. It says "can lead to" which to me means that if you store the RawFd somewhere and then use it later (like in a BorrowedFd or File), the function that does the storing should be marked unsafe, but if you immediately use the RawFd then you're good. Otherwise anytime a RawFd is even mentioned you'd need unsafe which doesn't make sense to me.

All of that said, I wouldn't be opposed to marking close as unsafe. Interestingly enough, dup is perfectly safe according to "Rust code has I/O safety if it's not possible for that code to cause invalid I/O operations" because anything you throw at dup will work (if the fd doesn't exist, it'll be allocated, if it does exist, it'll be closed).

Other than the breaking API changes, and interop with other things using RawFd, it seems like it should be fairly ergonomic to work with a safe API that uses OwnedFd/BorrowedFd/AsFd and mostly doesn't expose RawFd anywhere in the API (dup2_raw is a special case here).

Interestingly enough, dup is perfectly safe according to "Rust code has I/O safety if it's not possible for that code to cause invalid I/O operations"Interestingly enough, dup is perfectly safe according to "Rust code has I/O safety if it's not possible for that code to cause invalid I/O operations"

This is subtle, but a safe dup taking a RawFd and returning an OwnedFd could be used to trigger memory unsafety with the sort of things envisioned in the IO safety RFC. In particular:

And in specialized circumstances, violating I/O safety could even lead to violating memory safety. For example, in theory it should be possible to make a safe wrapper around an mmap of a file descriptor created by Linux's memfd_create system call and pass &[u8]s to safe Rust, since it's an anonymous open file which other processes wouldn't be able to access. However, without I/O safety, and without permanently sealing the file, other code in the program could accidentally call write or ftruncate on the file descriptor, breaking the memory-safety invariants of &[u8].

So safe code shouldn't be able to write or truncate an arbitrary fd, which could break immutability assumptions of something mmapping that fd. dup is also a problem, because safe code could write or truncate given an OwnedFd, which will have the same issue as write or truncate to the original fd.

Sure, but then everything is unsafe because you can just open /proc/self/mem. I think we have to take a pragmatic approach here and making dup unsafe because someone might be messing around with mmap is overkill.

SUPERCILEX · 2022-12-17T00:14:08Z

@asomers see bytecodealliance/rustix#478 (comment). I want to try your idea, but not in this PR.

Regarding this PR: please let me know what needs to change for it to be merged (I'll resolve conflicts then). I think AsDirFd should be left in, though I'd be willing to temporarily rip it out. I can also drop the dup commit, but I think as long as the commits are merged as is this PR is fine and we can just do fixups before release if necessary.

Regarding AsDirFd, I'll make a new PR after this one is merged that replaces it with the idea discussed here and in rustix. Regarding making openat an extension method, I think that's a bad idea: there's no symmetry to be found. If we make openat an extension method, why stop there? Open can be an extension on paths. Close can be an extension on FDs. But then what do you do about methods that have multiple paths/fds or mix the two? The whole thing becomes a mess, so let's just keep it simple and use top-level functions.

This gives us file descriptor types. nix-rust/nix#1863 BUG=b:233174528 TEST=CQ Change-Id: Ida81204a41d9e2f8b81afa3d0eb729e81a853f95 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/adhd/+/5599982 Tested-by: [email protected] <[email protected]> Commit-Queue: Li-Yu Yu <[email protected]> Reviewed-by: Chih-Yang Hsia <[email protected]>

SteveLauC · 2024-06-10T08:04:07Z

Close as this has been superseded by other I/O safety PRs: https://github.com/nix-rust/nix/tree/d60f710c31394b3ee7af2a1be0f37a713a9565bd/changelog

SUPERCILEX mentioned this pull request Nov 15, 2022

Take all AsFds by ref? bytecodealliance/rustix#446

Closed

asomers added this to the 0.27.0 milestone Nov 19, 2022

SUPERCILEX force-pushed the io_safety branch from c0cfd85 to 6713b09 Compare December 3, 2022 22:55

SUPERCILEX mentioned this pull request Dec 4, 2022

Implement I/O Safety #1750

Open

29 tasks

asomers reviewed Dec 4, 2022

View reviewed changes

src/unistd.rs Outdated Show resolved Hide resolved

SUPERCILEX added a commit to SUPERCILEX/nix that referenced this pull request Dec 4, 2022

Move some pure formatting changes out of nix-rust#1863

6ba8274

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX added a commit to SUPERCILEX/nix that referenced this pull request Dec 4, 2022

Move some pure formatting changes out of nix-rust#1863

59f66d1

Signed-off-by: Alex Saveau <[email protected]>

bors bot added a commit that referenced this pull request Dec 4, 2022

Merge #1908

40c8535

1908: Move some pure formatting changes out of #1863 r=asomers a=SUPERCILEX Co-authored-by: Alex Saveau <[email protected]>

SUPERCILEX force-pushed the io_safety branch 13 times, most recently from 88b6ba9 to 760ca08 Compare December 5, 2022 00:17

SteveLauC mentioned this pull request Dec 5, 2022

feat: I/O safety for 'sys/inotify' #1913

Merged

asomers requested changes Dec 5, 2022

View reviewed changes

SUPERCILEX force-pushed the io_safety branch 5 times, most recently from a56ddf2 to 3a11e20 Compare December 8, 2022 06:44

SUPERCILEX force-pushed the io_safety branch 3 times, most recently from 676185c to 4da7e7f Compare December 8, 2022 18:18

SUPERCILEX force-pushed the io_safety branch from 4da7e7f to f4a1c15 Compare December 8, 2022 19:00

asomers reviewed Dec 9, 2022

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

SUPERCILEX force-pushed the io_safety branch from f4a1c15 to f1af649 Compare December 9, 2022 21:13

SUPERCILEX added a commit to SUPERCILEX/nix that referenced this pull request Dec 9, 2022

Formatting only changes for nix-rust#1928 and nix-rust#1863

c5cc9dd

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX force-pushed the io_safety branch 2 times, most recently from fa23312 to db93263 Compare December 10, 2022 04:50

bors bot added a commit that referenced this pull request Dec 10, 2022

Merge #1935

8ecd7bc

1935: Formatting only changes for #1928 and #1863 r=asomers a=SUPERCILEX Co-authored-by: Alex Saveau <[email protected]>

SUPERCILEX added 3 commits December 10, 2022 14:09

Convert common syscalls to safe I/O types

30b32c4

Signed-off-by: Alex Saveau <[email protected]>

Support type-safe path FDs

c6af957

Signed-off-by: Alex Saveau <[email protected]>

Rewrite dup implementations to allow passing in RawFd

e89b47a

Signed-off-by: Alex Saveau <[email protected]>

SUPERCILEX force-pushed the io_safety branch from db93263 to e89b47a Compare December 10, 2022 22:10

ids1024 reviewed Dec 16, 2022

View reviewed changes

SUPERCILEX mentioned this pull request Jan 3, 2023

0.37 release planning bytecodealliance/rustix#496

Closed

SteveLauC mentioned this pull request Sep 25, 2023

unistd: make getpeereid take AsFd rather than a raw FD #2137

Merged

SteveLauC mentioned this pull request Oct 4, 2023

Add unistd::close_range() #2153

Closed

3 tasks

SteveLauC modified the milestones: 0.27.0, 0.30.0 Jun 9, 2024

SteveLauC closed this Jun 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert common syscalls to safe I/O types #1863

Convert common syscalls to safe I/O types #1863

SUPERCILEX commented Nov 13, 2022

asomers commented Dec 3, 2022

SUPERCILEX commented Dec 5, 2022

asomers left a comment

asomers Dec 5, 2022

SUPERCILEX Dec 5, 2022

SUPERCILEX Dec 8, 2022

SteveLauC Jun 9, 2024

SUPERCILEX commented Dec 8, 2022

asomers left a comment

SUPERCILEX commented Dec 10, 2022

asomers commented Dec 10, 2022

SUPERCILEX commented Dec 10, 2022

notgull commented Dec 11, 2022

ids1024 Dec 16, 2022

SUPERCILEX Dec 16, 2022

ids1024 Dec 18, 2022

SUPERCILEX Dec 18, 2022

ids1024 Dec 18, 2022

SUPERCILEX Dec 20, 2022

ids1024 Dec 20, 2022

SUPERCILEX Dec 21, 2022

SUPERCILEX commented Dec 17, 2022

SteveLauC commented Jun 10, 2024

Convert common syscalls to safe I/O types #1863

Convert common syscalls to safe I/O types #1863

Conversation

SUPERCILEX commented Nov 13, 2022

asomers commented Dec 3, 2022

SUPERCILEX commented Dec 5, 2022

asomers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Dec 8, 2022

asomers left a comment

Choose a reason for hiding this comment

SUPERCILEX commented Dec 10, 2022

asomers commented Dec 10, 2022

SUPERCILEX commented Dec 10, 2022

notgull commented Dec 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SUPERCILEX commented Dec 17, 2022

SteveLauC commented Jun 10, 2024