You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I was hoping that I could get #488 into shape as well, as well as implement a passable API for Solarish's event ports API so that this crate can be used in polling for all of the *nix backends.
Also, if we're doing breaking changes, is there a way that we can adjust dup2 and dup3's APIs? Having to take an OwnedFd as a parameter hurts them, especially in the case of dup3, where having an option for CLOEXEC is beneficial.
Regarding dup, Alan noticed this too: nix-rust/nix#1863 (comment). I couldn't figure out a good way to both require ownership and not force you to pass in the OwnedFd, so I ended up adding
on top of the existing dup2 (and same for dup3). That kind of sucks, but it lets us have two safe interfaces that can be used at different levels of abstraction. Would be cool if there was a better way though!
Regarding #493, I just need a final sign-off and I'll send a PR.
Even though it will handle closing the FD for you if it's owned? @ids1024 argued the same position, so I'm probably in the minority and would be fine with it being unsafe. I don't see a way to let you create create FDs out of thin air without raw FDs (well you could indirectly with BorrowedFd, but that would be icky), so that's why I landed on that solution.
Rust code is allowed to assume that the identity of a file descriptor does not change without someone having access to the &mut OwnedFd. This is important for safety in several use cases, like mmapping a memfd you previously sealed.
Yeah, OwnedFd::from_raw_fd for an fd that isn't open isn't quite right. You want to avoid dropping it, and technically just constructing an OwnedFd from an invalid fd may be considered unsound, though it's unlikely to be implemented in a way that could cause an issues.
If this is needed, having a variant of the function that's unsafe and takes a RawFd is probably best.
If you have a RawFd which is invalid, meaning it's forged and potentially aliasing an OwnedFd somewhere else in the process, then doing a dup2_raw on it is unsound, in general. Constructing a new OwnedFd isn't the problem.
It gets ambiguous in the case where you're doing dup2's in preparation for an exec, and you know all the fds you want to pass to the exec, and you want to just overwrite any other fds that might be in the way. Ordinarily that should be considered unsound. At the same time, this is the main use case for dup2. So we need some rule to say it's ok in this specific case, and we don't yet have it.
It shouldn't be unsound as implemented, or probably in any reasonable future implementation of std, but I'm just noting that you technically violate what OwnedFd::from_raw_fd says is required for safety:
Safety
The fd passed in must be a valid and open file descriptor.
Safety
The resource pointed to by fd must be open and suitable for assuming ownership. The resource must not require any cleanup other than close.
And it just isn't quite right semantically, since an fd that isn't open isn't what an OwnedFd is supposed to represent.
And it just isn't quite right semantically, since an fd that isn't open isn't what an OwnedFd is supposed to represent.
It isn't known to be sound to pass in what you believe is a fd that isn't open to dup2_raw, because in general you don't know what file descriptors are open elsewhere in the process. So the OwnedFd::from_raw_fd requirement reflects what is currently known to be sound.
That doesn't cover everything that people expect to be able to do with dup2_raw, but that's where #497 comes in.
On a different but breaking-ish topic, are we sure we want statx to to check for its own availability? I'm not a big fan of that because it bumps up codegen for everyone even if they don't care about compatibility with 4.x kernels. The rationale for this check is that the stdlib does this, so rustix should too. I don't believe this transitivity holds. The stdlib does this because it must be able to stat and would like to do so more efficiently when possible. On the other hand, Rustix offers all the stat functions, so developers choose which ones they want.
I think if statx gets special treatment, then a minimum kernel version should be chosen and all syscalls (such as io uring) should be using this pattern. But again, I don't think that's a good idea since everyone will have different min kernel versions and it'll be a perf hit. If people really want this, I think the better approach would be to write a wrapper library that does this.
Creating a whole extra library just to do "don't call statx again if it's just going to ENOSYS", a common need, is inconvenient. Would it work for you to rename the existing statx to statx_if_avail or so, and then add a new regular statx which doesn't do the extra code?
(io_uring is a different situation because if you don't have io_uring, you typically take a completely different path and never try to call it again)
io_uring is a different situation because if you don't have io_uring, you typically take a completely different path and never try to call it again
Fair point, but what about openat2 or copy_file_range?
And yeah, I agree that a sperate lib would be a pain, but duplicating every function in the public API also seems unpleasant. Maybe we can use features for this? You could say "assume kernel x.x or later". Or maybe we can provide a macro keyed by the name of the syscall that wraps an arbitrary function returning the rustix result?
I think I'd vote for either the feature gate idea or yours (though I worry about the API complexity).
I'd be okay with an "assume kernel version 4.XX" feature that elides this check. Although, what happens when Rust's minimum Linux version is eventually bumped to the point where it's above 4.XX? Then, would this feature just become a no-op?
Thanks for bringing this up. With #495 changing from being a definition-of-safety issue to merely being an ergonomics issue, I no longer thing it needs to block 0.37. And I think that's the last blocker we've identified here.
I've now released a 0.37.0-alpha.0 prerelease to get the process started. If anyone tries this version and finds a bug or something missing please report it!
I'm also hoping to catch up on reviews, and if I can do that soon enough then we can get them in before releasing 0.37 too.
I don't know. This is not my build. I get all time segfault and try strace... and this is fragment of strace log
Alpine Linux
I try simple import numpy in python and get sigfault.
I've now released a v0.37.0-alpha.1 with all the PRs on my radar for including in the 0.37 release. If there's anything else we should include, please post about it here! I plan to do a 0.37 release soon.
Activity
notgull commentedon Jan 3, 2023
I was hoping that I could get #488 into shape as well, as well as implement a passable API for Solarish's event ports API so that this crate can be used in
pollingfor all of the *nix backends.Also, if we're doing breaking changes, is there a way that we can adjust
dup2anddup3's APIs? Having to take anOwnedFdas a parameter hurts them, especially in the case ofdup3, where having an option forCLOEXECis beneficial.SUPERCILEX commentedon Jan 3, 2023
Regarding dup, Alan noticed this too: nix-rust/nix#1863 (comment). I couldn't figure out a good way to both require ownership and not force you to pass in the
OwnedFd, so I ended up addingon top of the existing dup2 (and same for dup3). That kind of sucks, but it lets us have two safe interfaces that can be used at different levels of abstraction. Would be cool if there was a better way though!
Regarding #493, I just need a final sign-off and I'll send a PR.
sunfishcode commentedon Jan 3, 2023
By I/O safety, that dup2_raw should be unsafe, because it operates on raw fds (AsRawFd/IntoRawFd are not trustable).
SUPERCILEX commentedon Jan 3, 2023
Even though it will handle closing the FD for you if it's owned? @ids1024 argued the same position, so I'm probably in the minority and would be fine with it being unsafe. I don't see a way to let you create create FDs out of thin air without raw FDs (well you could indirectly with BorrowedFd, but that would be icky), so that's why I landed on that solution.
sunfishcode commentedon Jan 3, 2023
RawFd implements AsRawFd and IntoRawFd and it's just an i32 so you can pass in 42 and have it clobbered without knowing who owns 42.
Rustix's answer here is that you can OwnedFd::from_raw_fd if you really want to dup2 to an arbitrary integer.
sunfishcode commentedon Jan 3, 2023
Rust code is allowed to assume that the identity of a file descriptor does not change without someone having access to the
&mut OwnedFd. This is important for safety in several use cases, like mmapping a memfd you previously sealed.SUPERCILEX commentedon Jan 4, 2023
I don't think that's a good solution in the face of errors since you'll need to handle ManuallyDrop and friends yourself.
This is a compelling argument for dup2_raw being unsafe, I agree now, thx. :)
ids1024 commentedon Jan 4, 2023
Yeah,
OwnedFd::from_raw_fdfor an fd that isn't open isn't quite right. You want to avoid dropping it, and technically just constructing anOwnedFdfrom an invalid fd may be considered unsound, though it's unlikely to be implemented in a way that could cause an issues.If this is needed, having a variant of the function that's
unsafeand takes aRawFdis probably best.sunfishcode commentedon Jan 4, 2023
If you have a
RawFdwhich is invalid, meaning it's forged and potentially aliasing anOwnedFdsomewhere else in the process, then doing adup2_rawon it is unsound, in general. Constructing a newOwnedFdisn't the problem.It gets ambiguous in the case where you're doing dup2's in preparation for an exec, and you know all the fds you want to pass to the exec, and you want to just overwrite any other fds that might be in the way. Ordinarily that should be considered unsound. At the same time, this is the main use case for dup2. So we need some rule to say it's ok in this specific case, and we don't yet have it.
sunfishcode commentedon Jan 4, 2023
I filed #497 to discuss I/O safety for this dup2 issue.
ids1024 commentedon Jan 4, 2023
It shouldn't be unsound as implemented, or probably in any reasonable future implementation of std, but I'm just noting that you technically violate what
OwnedFd::from_raw_fdsays is required for safety:And it just isn't quite right semantically, since an fd that isn't open isn't what an
OwnedFdis supposed to represent.sunfishcode commentedon Jan 4, 2023
It isn't known to be sound to pass in what you believe is a fd that isn't open to dup2_raw, because in general you don't know what file descriptors are open elsewhere in the process. So the
OwnedFd::from_raw_fdrequirement reflects what is currently known to be sound.That doesn't cover everything that people expect to be able to do with dup2_raw, but that's where #497 comes in.
SUPERCILEX commentedon Jan 4, 2023
On a different but breaking-ish topic, are we sure we want statx to to check for its own availability? I'm not a big fan of that because it bumps up codegen for everyone even if they don't care about compatibility with 4.x kernels. The rationale for this check is that the stdlib does this, so rustix should too. I don't believe this transitivity holds. The stdlib does this because it must be able to stat and would like to do so more efficiently when possible. On the other hand, Rustix offers all the stat functions, so developers choose which ones they want.
I think if statx gets special treatment, then a minimum kernel version should be chosen and all syscalls (such as io uring) should be using this pattern. But again, I don't think that's a good idea since everyone will have different min kernel versions and it'll be a perf hit. If people really want this, I think the better approach would be to write a wrapper library that does this.
Thoughts?
sunfishcode commentedon Jan 4, 2023
Yeah, I'm open to making changes here.
Creating a whole extra library just to do "don't call statx again if it's just going to
ENOSYS", a common need, is inconvenient. Would it work for you to rename the existing statx to statx_if_avail or so, and then add a new regular statx which doesn't do the extra code?(io_uring is a different situation because if you don't have io_uring, you typically take a completely different path and never try to call it again)
SUPERCILEX commentedon Jan 5, 2023
Fair point, but what about openat2 or copy_file_range?
And yeah, I agree that a sperate lib would be a pain, but duplicating every function in the public API also seems unpleasant. Maybe we can use features for this? You could say "assume kernel x.x or later". Or maybe we can provide a macro keyed by the name of the syscall that wraps an arbitrary function returning the rustix result?
I think I'd vote for either the feature gate idea or yours (though I worry about the API complexity).
notgull commentedon Jan 5, 2023
I'd be okay with an "assume kernel version 4.XX" feature that elides this check. Although, what happens when Rust's minimum Linux version is eventually bumped to the point where it's above 4.XX? Then, would this feature just become a no-op?
SUPERCILEX commentedon Jan 5, 2023
Yeah, though I think we could actually just remove the feature since a MSRV bump is a breaking change anyway.
SUPERCILEX commentedon Jan 6, 2023
Ok, I gave the feature gate idea a go: #501
SUPERCILEX commentedon Feb 7, 2023
@sunfishcode Do you think it might be worth releasing 0.37 with what we've got and punting dup + other changes to 0.38? We've already accumulated 6 breaking changes which feels appropriate for a release: https://github.com/bytecodealliance/rustix/pulls?q=label%3A0.37
sunfishcode commentedon Feb 7, 2023
Thanks for bringing this up. With #495 changing from being a definition-of-safety issue to merely being an ergonomics issue, I no longer thing it needs to block 0.37. And I think that's the last blocker we've identified here.
I've now released a 0.37.0-alpha.0 prerelease to get the process started. If anyone tries this version and finds a bug or something missing please report it!
I'm also hoping to catch up on reviews, and if I can do that soon enough then we can get them in before releasing 0.37 too.
SUPERCILEX commentedon Feb 7, 2023
Awesome, thanks! I just upgraded SUPERCILEX/fuc@1c310e0 and it's looking good.
Unpublished commentedon Feb 13, 2023
I'd like to see #536 as well in a 0.37 release
magicse commentedon Feb 28, 2023
Hi there. Plese could you explain me. It this normal sitation with statx ?
sunfishcode commentedon Feb 28, 2023
@magicse Are you using rustix?
magicse commentedon Feb 28, 2023
I don't know. This is not my build. I get all time segfault and try strace... and this is fragment of strace log
Alpine Linux
I try simple import numpy in python and get sigfault.
LingMan commentedon Mar 1, 2023
@magicse: Numpy is written in Python and C. Not in Rust. No clue what causes your problem, but you're talking to the wrong people.
sunfishcode commentedon Mar 2, 2023
I've now released a v0.37.0-alpha.1 with all the PRs on my radar for including in the 0.37 release. If there's anything else we should include, please post about it here! I plan to do a 0.37 release soon.
sunfishcode commentedon Mar 3, 2023
I've now released 0.37! See the release notes for a summary of new features and changes.