Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.7k
tac, tail, dd: detect closed stdin before Rust sanitizes it to /dev/null#9664
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
GNU testsuite comparison: |
32ab405 toe0bafa3Comparee0bafa3 tobfdfe19CompareGNU testsuite comparison: |
anastygnome commentedDec 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm not sure we should do this, the shell shouldn't allow that in the first place, this is not compliant with posix. |
ChrisDryden commentedDec 15, 2025
For tracking this would be the same approach of getting the PIPE signal as here:#9657 Maybe my understanding of the discussion is not complete,@anastygnome are you saying that this UUTILS implementation is not Posix compliant and that the GNU Coreutils is compliant in regards to this? Or are you saying that both the GNU Coreutils implementation and this implementation are non-compliant and that we should diverge from the GNU Coreutils implementation |
bfdfe19 tod61c41fCompared61c41f todfabb4bComparecodspeed-hqbot commentedDec 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#9664 willimprove performances by 2.17%Comparing Summary
Benchmarks breakdown
Footnotes
|
GNU testsuite comparison: |
sylvestre commentedDec 15, 2025
impressive! |
collinfunk commentedDec 15, 2025
Where does POSIX disallow This behavior has been supported the Bourne shell for ages. Even Solaris My personal opinion is that Rust really should have a way to disable the messing around it does with signal actions and file descriptors before |
dfabb4b to2c10c5fCompareGNU testsuite comparison: |
anastygnome commentedDec 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
So, to recap the issue Basically this point of the posix standard was ambiguous before 2017. Since 2017, the posix standard says that if any of the 3 standards file descriptiors are closed, we are not in a posix compliant environment. So the question is : Do we consider posix post 2017 as the new norm, or do we add this check as a continuing workaround ? I'll quote the posix standard later but it has been there on older issues. |
| #[cfg(unix)] | ||
| #[allow(clippy::missing_safety_doc)] | ||
| pubunsafeextern"C"fncapture_stdio_state(){ | ||
| use nix::libc; | ||
| unsafe{ | ||
| STDIN_WAS_CLOSED.store( | ||
| libc::fcntl(libc::STDIN_FILENO, libc::F_GETFD) == -1, | ||
| Ordering::Relaxed, | ||
| ); | ||
| STDOUT_WAS_CLOSED.store( | ||
| libc::fcntl(libc::STDOUT_FILENO, libc::F_GETFD) == -1, | ||
| Ordering::Relaxed, | ||
| ); | ||
| STDERR_WAS_CLOSED.store( | ||
| libc::fcntl(libc::STDERR_FILENO, libc::F_GETFD) == -1, | ||
| Ordering::Relaxed, | ||
| ); | ||
| } | ||
| } |
anastygnomeDec 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
For efficiency we should do something similar to the fastpath shown inthis function. It is the function that reopens the FDs in the standard library.
how about this
| #[cfg(unix)] | |
| #[allow(clippy::missing_safety_doc)] | |
| pubunsafeextern"C"fn capture_stdio_state(){ | |
| use nix::libc; | |
| unsafe{ | |
| STDIN_WAS_CLOSED.store( | |
| libc::fcntl(libc::STDIN_FILENO, libc::F_GETFD) == -1, | |
| Ordering::Relaxed, | |
| ); | |
| STDOUT_WAS_CLOSED.store( | |
| libc::fcntl(libc::STDOUT_FILENO, libc::F_GETFD) == -1, | |
| Ordering::Relaxed, | |
| ); | |
| STDERR_WAS_CLOSED.store( | |
| libc::fcntl(libc::STDERR_FILENO, libc::F_GETFD) == -1, | |
| Ordering::Relaxed, | |
| ); | |
| } | |
| } | |
| /// # Safety | |
| /// | |
| /// Must be called very early in process startup, before the runtime may reopen | |
| /// closed standard file descriptors. Uses low-level libc APIs. | |
| /// This code is just checking the state of the three standard fds and has no side-effects. | |
| #[cfg(unix)] | |
| pubunsafeextern"C"fn capture_stdio_state(){ | |
| use nix::libc; | |
| // Prefer poll() where it reliably reports POLLNVAL | |
| #[cfg(not(any( | |
| miri, | |
| target_os ="emscripten", | |
| target_os ="fuchsia", | |
| target_os ="vxworks", | |
| target_os ="macos", | |
| target_os ="ios", | |
| target_os ="watchos", | |
| target_os ="redox", | |
| target_os ="l4re", | |
| target_os ="horizon", | |
| )))] | |
| { | |
| unsafe{ | |
| letmut pfds =[ | |
| libc::pollfd{ | |
| fd: libc::STDIN_FILENO, | |
| events:0, | |
| revents:0, | |
| }, | |
| libc::pollfd{ | |
| fd: libc::STDOUT_FILENO, | |
| events:0, | |
| revents:0, | |
| }, | |
| libc::pollfd{ | |
| fd: libc::STDERR_FILENO, | |
| events:0, | |
| revents:0, | |
| }, | |
| ]; | |
| let ok =loop{ | |
| match libc::poll(pfds.as_mut_ptr(), pfds.len()as_,0){ | |
| -1 =>match*libc::__errno_location(){ | |
| libc::EINTR =>continue, | |
| libc::EINVAL | libc::ENOMEM | libc::EAGAIN =>breakfalse, | |
| _ => libc::abort(), | |
| }, | |
| _ =>breaktrue, | |
| } | |
| }; | |
| if ok{ | |
| STDIN_WAS_CLOSED.store(pfds[0].revents& libc::POLLNVAL !=0,Ordering::Relaxed); | |
| STDOUT_WAS_CLOSED.store(pfds[1].revents& libc::POLLNVAL !=0,Ordering::Relaxed); | |
| STDERR_WAS_CLOSED.store(pfds[2].revents& libc::POLLNVAL !=0,Ordering::Relaxed); | |
| return; | |
| } | |
| } | |
| } | |
| // Fallback: fcntl() | |
| unsafe{ | |
| STDIN_WAS_CLOSED.store( | |
| libc::fcntl(libc::STDIN_FILENO, libc::F_GETFD) == -1, | |
| Ordering::Relaxed, | |
| ); | |
| STDOUT_WAS_CLOSED.store( | |
| libc::fcntl(libc::STDOUT_FILENO, libc::F_GETFD) == -1, | |
| Ordering::Relaxed, | |
| ); | |
| STDERR_WAS_CLOSED.store( | |
| libc::fcntl(libc::STDERR_FILENO, libc::F_GETFD) == -1, | |
| Ordering::Relaxed, | |
| ); | |
| } | |
| } |
anastygnome commentedDec 17, 2025
Also should be done for cat ;) |
ChrisDryden commentedDec 17, 2025
I'm open either way to the approach, was hoping to get a maintainers opinion on which approach they'd be interested in? |
Uh oh!
There was an error while loading.Please reload this page.
Using the same approach that was identified for the pipeline signals, we can retrieve the state of stdin and stdout before the rust runtime overrides the fd to /dev/null
This means that we will be able to both preserve the pre-existing behaviour for passing in /dev/null and also allow us to raise the same error message and code when the closed fd is passed to the stdin or stdout as the GNU implementation.