Reopen standard file descriptors when they are missing on Unix #75295
Conversation
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
For windows there already is the |
|
Using a no-op placeholder is definitely another option to consider. The presence of /dev/null would no longer required. The check if descriptors are valid at start-up would still be necessary. It is not immediately clear what should AsRawFd return in that case, although -1 might be an option. Furthermore if a placeholder is used it is no longer possible to customize the behaviour by changing the file descriptors directly. AFAICS the Windows implementation no longer uses Maybe::Fake, but always obtains a fresh handle, avoiding the last issue in the process. But I don't see how this could be addressed on Unix while using placeholders, at least not without API additions. I would like to avoid introducing a hard requirement on /dev/null, so I changed the PR to reopen the fds on the best effort basis. |
| if pfd.revents & libc::POLLNVAL == 0 { | ||
| continue; | ||
| } | ||
| if libc::open("/dev/null\0".as_ptr().cast(), libc::O_RDWR, 0) == -1 { |
There was a problem hiding this comment.
| if libc::open("/dev/null\0".as_ptr().cast(), libc::O_RDWR, 0) == -1 { | |
| if libc::open(b"/dev/null\0".as_ptr().cast(), libc::O_RDWR, 0) == -1 { |
|
So, it's definitely sketchy to run a process with file descriptors 0, 1, or 2 closed, for exactly this reason: the next opened file will become one of those file descriptors, which will wreak all manner of havoc. However, assuming the existence or accessibility of devices like As an alternative, which doesn't require any particular device file to exist on the filesystem, we could use pipes:
I think that will work better. It requires a little bit of care (because some of the pipes will initially become the wrong file descriptors), but the following algorithm should work:
|
|
The output pipes would begin to block once they're full, wouldn't they? The advantage of |
|
No, if the other end is closed it'll error. |
|
I don't think a closed pipe would be a good replacement due to SIGPIPE I would find receiving a SIGPIPE in this case to be highly undesirable. |
|
It's also undesirable to run a program without those file descriptors. And it's undesirable to require the hardcoded path |
The current version of this PR uses them on a best-effort basis, i.e. it's not required. |
|
To add another argument for /dev/null. When started in secure mode (setuid, EDIT: Another disadvantage of closed pipe for stdout / stderr is that println! |
|
Re-posting a review comment at the top level: |
|
In context of RFC 1014, I extended test to verify non-panicking behaviour. |
tmiasko
force-pushed
the
fds
branch
3 times, most recently
from
dbdea19 to
7eb4381
Compare
|
The poll on Darwin doesn't set POLLNVAL for closed fds, so I used an alternative implementation there. |
crlf0710
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The child process spawned in the test case segfaults on arm-android. I don't know why yet, but it is unrelated to the changes in std, since the same issue occurs without any changes. |
The syscalls returning a new file descriptors generally use
lowest-numbered file descriptor not currently opened, without any
exceptions for those corresponding to the standard streams.
Previously when any of standard streams has been closed before starting
the application, operations on std::io::{stderr,stdin,stdout} objects
were likely to operate on other logically unrelated file resources
opened afterwards.
Avoid the issue by reopening the standard streams when they are closed.
|
I modified the PR to reuse an existing test case instead of adding a new one. This test case was already ignored on android for presumably the same reasons (closing any two standard streams before spawning a new process is sufficient to cause the segfault). |
|
@bors r+ |
|
📌 Commit 7d98d22 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
|
Something went wrong for Miri on apple targets: I'll submit a PR. |
|
Submitted #77288 |
fix building libstd for Miri on macOS Fixes a Miri regression introduced by rust-lang#75295 Cc @tmiasko @Amanieu
fix building libstd for Miri on macOS Fixes a Miri regression introduced by rust-lang#75295 Cc @tmiasko @Amanieu
|
I've found that this change introduced a regression in the "Rust first then fallback to Python" version of the Mercurial test suite. I was testing if going from Here is the (somewhat convoluted) reproduction, which requires //! Run the executable (not with `cargo play`, it'll eat the redirect)
//! `myexecutable -c 'import sys; sys.stdout.write("hello")' 1>&-`
//! If compiled with 1.41.1, it will output (with an error code of 1):
//!
//! Traceback (most recent call last):
//! File "<string>", line 1, in <module>
//! AttributeError: 'NoneType' object has no attribute 'write'
//!
//! But with 1.48.0, it doesn't output anything and silently eats the error code, exiting with 0.
use std::process::Command;
fn main() {
let mut args_os = std::env::args_os();
args_os.next().expect("expected argv[0] to exist");
let mut c = Command::new("python");
c.args(args_os);
let status = c.status();
match status {
Err(_) => (),
Ok(s) => {
if s.success() {
std::process::exit(1);
}
}
}
}I'm not 100% sure what to think of it yet, but my gut feeling is that eating errors is bad; I'm reporting this now since it's fresh in my mind. |
|
Please file a new issue -- it's unlikely to receive much attention on a closed PR, but an issue will let us appropriately tag and track any potential fixes. |
The syscalls returning a new file descriptors generally return lowest-numbered
file descriptor not currently opened, without any exceptions for those
corresponding to stdin, sdout, or stderr.
Previously when any of standard file descriptors has been closed before starting
the application, operations on std::io::{stderr,stdin,stdout} were likely to
either succeed while being performed on unrelated file descriptor, or fail with
EBADF which is silently ignored.
Avoid the issue by using /dev/null as a replacement when the standard file
descriptors are missing.
The implementation is based on the one found in musl. It was selected among a
few others on the basis of the lowest overhead in the case when all descriptors
are already present (measured on GNU/Linux).
Closes #57728.
Closes #46981.
Closes #60447.
Benefits:
Drawbacks:
Alternatives: