Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust programs started with closed stdio streams may corrupt files|sockets|etc #57728

Closed
theastrallyforged opened this issue Jan 18, 2019 · 8 comments · Fixed by #75295
Closed
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@theastrallyforged
Copy link

For example, if a program is started with ./foo >&- (stdout closed, stdin and stderr open), println!()s fail silently (despite strace showing that write(2) clearly returns -1 and EBADF). If a file is opened and then a println!() is attempted, it silently writes into the file (which is probably not what was intended).

Example 1:

fn main() {
    println!("Hello, world!");
}

Invocation: target/debug/hello-world >&-
Expected result: panic, with an error message relating to being unable to write to stdout, exit code 101
Observed result: nothing (no output on stderr, exit code 0)

Example 2:

fn main() {
    let file = std::fs::File::create("foo").unwrap();
    println!("Hello, world!");
    drop(file);
}

Invocation: target/debug/hello-file >&-
Expected result: panic (as before), empty file ./foo
Observed result: no panic, ./foo contains Hello, world!

Rust version: 1.30.0
Platform: x86_64-unknown-linux-gnu

It looks like any Rust program which is started with one or more closed stdio streams, opens a file (or socket, or…), and subsequently tries to read|write said stdio stream will inadvertently read|write the file instead (messing up the seek position, not to mention causing data corruption). This seems like terribly bad behavior. ;-;

@theastrallyforged theastrallyforged changed the title Rust programs misbehave when they're started with closed stdio Rust programs started with closed stdio streams may corrupt files|sockets|etc Jan 18, 2019
@redengin
Copy link

it appears that this is the conventional expectation of OS kernels.

https://stackoverflow.com/questions/22367920/is-it-possible-that-linux-file-descriptor-0-1-2-not-for-stdin-stdout-and-stderr

While I don't believe this should be allowed by the OS kernels, making Rust ensure that it doesn't use "reserved" file descriptors would break programs that rely on this corner-case.

As this really is an OS level behavior, I think this needs input from OS Security architecture (SELinux, AppArmor, etc.)

@theastrallyforged
Copy link
Author

Kernel compliance or no, it doesn't seem okay to me that a program can open a fs::File, try to write to io::Stdout, and, through no fault of its own, inadvertently corrupt the file.

@bluss
Copy link
Member

bluss commented Jan 18, 2019

This seems to be a known problem of the C/POSIX platforms at least as described in Secure Programming Cookbook for C and C++ chapter 1.5

@Centril Centril added O-linux Operating system: Linux O-x86_64 Target: x86-64 processors (like x86_64-*) T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 20, 2019
@sfackler sfackler removed O-linux Operating system: Linux O-x86_64 Target: x86-64 processors (like x86_64-*) labels Jan 20, 2019
@sfackler
Copy link
Member

I don't think there's anything we can do here. On Unix platforms, stdout is by definition file descriptor 2.

@Amanieu
Copy link
Member

Amanieu commented Jan 20, 2019

We could check at startup whether FD 0/1/2 are open using fcntl(F_GETFD) (which returns EBADF if the FD is closed).

I am not sure what we can do with this information, but I am not very familiar with the I/O parts of the standard library.

@theastrallyforged
Copy link
Author

If during startup we discover that any of those streams are closed, we can set a flag in the relevant std::io structures to indicate that all accesses should return Err without attempting a syscall.

@theastrallyforged
Copy link
Author

theastrallyforged commented Jan 24, 2019

As a data point, cat (GNU cat on my Ubuntu system) doesn't have this issue (it doesn't output anything, but at least it doesn't try to write to its input files). strace shows that everything that opens a file gets an FD of at least 3:

(Contrast the Rust programs, where the first thing to open a file gets FD 1.)

At the end, when it actually gets to opening foo, it gets an FD of 3 as well. No idea where FD 1 is coming from given that 1. it was closed by the shell (>-) and 2. the output doesn't appear on my terminal but 3. strace shows that the writes (to FD 1) succeed.

As I've just realized and as Amanieu has pointed out, >- is not how you close stdout.

cat is still unaffected; cat foo >&- on my system produces cat: standard output: Bad file descriptor. It looks like it just detects that by doing fstat(1, _). Not sure how useful that is to us.

@Amanieu
Copy link
Member

Amanieu commented Jan 24, 2019

You need to use >&-, not >-.

RalfJung added a commit to RalfJung/rust that referenced this issue Sep 25, 2020
Reopen standard file descriptors when they are missing on Unix

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 rust-lang#57728.
Closes rust-lang#46981.
Closes rust-lang#60447.

Benefits:
* Makes applications robust in the absence of standard file descriptors.
* Upholds IntoRawFd / FromRawFd safety contract (which was broken previously).

Drawbacks:
* Additional syscall during startup.
* The standard descriptors might have been closed intentionally.
* Requires /dev/null.

Alternatives:
* Check if stdin, stdout, stderr are opened and provide no-op substitutes in std::io::{stdin,stdout,stderr} without reopening them directly.
* Leave the status quo, expect robust applications to reopen them manually.
@bors bors closed this as completed in db7ee7c Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants