Rust's stdio should not ignore EBADF error on non-windows platforms

Opened by albel727 at 2023-01-26 20:40:53

I've discovered, that std::io::stdin/out/err() streams unconditionally ignore EBADF-like IO errors on all platforms. This is done by checking the read/write error in a handle_ebadf() function.

https://github.com/rust-lang/rust/blob/1ccb50eaa670f86b69e7a64484a8c97e13169183/src/libstd/io/stdio.rs#L123-L128

It appears, that this behavior was first introduced here https://github.com/rust-lang/rust/commit/a7bbd7da4eb98127104fdfd415ad6c746f7e2a12

The commit clearly has Windows in mind, where it appears the standard streams may be unavailable. But on Linux, the streams are expected to be always present, so there's no reason to ignore EBADF in the first place, as it indicates that something is very wrong.

Not only that, but due to file descriptor reuse behavior on Unixes, if descriptors 0/1/2 are not open, sometimes the very next calls to open() will allocate them. This means, that a program running without properly preallocated 0/1/2 descriptors may start happily println!()-ing over its own sqlite database, or send private execution logs across a tcp connection.

So, if std::io::stdout/err() happens to discover that something yanked the descriptors from under program's feet, the proper response is not to silently ignore EBADF, but to panic(), before something else unwittingly allocated it with likely disastrous consequences.

cc @alexcrichton
Eduard-Mihai Burtescu at 2018-01-08 13:06:21
There's not really anything we can do about reuse, we can't protect against that at all. What we can do, however, is protect against accidentally misconfigured dameons or things like that. If you spawn a process that doesn't actually have stdin/stdout/stderr then it's consistent with Windows to ignore that error. Naturally in libstd, however, we go to great lengths to ensure that all spawned processes have a stdout/stdin/stderr.

I don't think this has convinced me (personally) that we should change this behavior, but if you've got an example of how this could come up in practice we could always bring it up with the broader libs team!
Alex Crichton at 2018-01-08 17:23:46
then it's consistent with Windows to ignore that error

It's not "consistent", because there is zero similarity between situations on Windows and Linux, and so comparing them for consistency here makes no sense.

For Windows running without streams is not an exceptional situation. On the contrary, it's overwhelmingly common for GUI apps to be without console output. It makes sense to not panic there, because the dangerous descriptor reuse problem doesn't occur, and there exists GetStdHandle() which, unlike on Linux, allows to explicitly find out whether you have a console without having to execute a write() first.

On Linux such situation is nothing but exceptional. It would be very rarely that EBADF-ignoring logic would be even triggered, but in 99.99% cases when it does, it will not be because the Rust user wanted it, and it will eventually end in something bad. It will not be helping anyone to silence that error as opposed to panicking before a Rust user's database is corrupted.

Descriptor reuse can't be prevented, but that doesn't mean we shouldn't attempt to mitigate obviously dangerous situations instead papering over enormous platform differences just because outward similarity between EBADF and ERROR_INVALID_HANDLE looks beautifully harmonious. Reality is such that Windows consoles are peculiar, and other platforms should not have to be affected for it.

For the 0.01% cases when the user would be actually developing an app that would be OK with encountering closed stdio descriptors on Linux, he will either do anything to NOT use println!(), and so any objections against changing its logic are moot, or he would rather benefit from an explicit stdio silencing mechanism, so as to be freely able to do anything he wants with the descriptors, without being afraid of a stray println!().

Such explicit mechanism which would also help in cases such as https://github.com/rust-lang/rust/issues/33736#issuecomment-220437217

E.g. if there was a way to reset streams to Maybe::Fake and back

https://github.com/rust-lang/rust/blob/master/src/libstd/io/stdio.rs#L102

then the "GetStdHandle caching", as well as the whole "panicking on Windows due to lacking console" thing that motivated the original "EBADF ignoring" commit, would not be a problem.

On a related note, I may also argue that instead of current Windows's ERROR_INVALID_HANDLE ignoring there should be rather some logic that would permanently switch stdio to Maybe::Fake if GetStdHandle() returns INVALID_HANDLE_VALUE.

I also suspect that in cases when GetStdHandle() does return a valid handle, then further transient ERROR_INVALID_HANDLE coming from write() would be normally due to Windows user closing the console window, something that on Linux would be analogous NOT to EBADF, but to EPIPE, and so the consistency argument is moot on that account too.

But I would welcome @retep998 's validation on these matters.
albel727 at 2018-01-08 19:23:00
To reiterate, because reading your message again I began to doubt that you understand the implications, and you appear to have asked for examples when the problem can come up in practice.

What we can do, however, is protect against accidentally misconfigured dameons or things like that. If you spawn a process that doesn't actually have stdin/stdout/stderr then

on Linux the process will invariably kill your kittens and set your house on fire. You just don't do that on Linux, ever. A daemon that is accidentally started like that simply has no way of NOT breaking everything in very ingenious ways.

You should consider yourself lucky if the very first remote TCP connection that it opens doesn't contain hacker's input for something that it would believe to be entered by local user on the console in interactive mode. You will be even more lucky if the very second file that it opens is not a sqlite production database, into which it will eventually print an error about how it can't read the database. Linux descriptor reuse logic means, that if that daemon, e.g. goes through a list of files and prints a string to stdout for every one of them, then each and every one of them may be consistently opened as descriptor 1 and promptly corrupted.

You can't paper over any such "daemon misconfiguration" by hiding errors. Period.

The standard way to start silent daemons on Linux includes providing /dev/null devices for stdio. It would take some considerably unlikely and unfortunate misconfiguration to make any linux daemon manager to provide nothing to a daemon, and the user should be made immediately aware of any such misconfiguration by promptly crashing.
albel727 at 2018-01-08 20:09:20
Just so that everyone understands Linux descriptor reuse on a concrete example.

https://play.rust-lang.org/?gist=b44be7df66e7dd59ce25209927523782&version=stable
albel727 at 2018-01-08 20:38:57
On a related note, I may also argue that instead of current Windows's ERROR_INVALID_HANDLE ignoring there should be rather some logic that would permanently switch stdio to Maybe::Fake if GetStdHandle() returns INVALID_HANDLE_VALUE.

I also suspect that in cases when GetStdHandle() does return a valid handle, then further transient ERROR_INVALID_HANDLE coming from write() would be normally due to Windows user closing the console window, something that on Linux would be analogous NOT to EBADF, but to EPIPE, and so the consistency argument is moot on that account too.

But I would welcome @retep998 's validation on these matters.

Currently libstd does not cache the result of GetStdHandle. This means that each time a read or write is performed, it will call GetStdHandle which means you can call SetStdHandle and Rust will use the new value. This is why I am opposed to permanently switching to Maybe::Fake because even if GetStdHandle doesn't return a valid value at the moment, it certainly can in the future.

If a process starts without a console the handles returned by GetStdHandle are all 0 which is a safe sentinel value. If AllocConsole is then called to give the process a console, it will implicitly call SetStdHandle to assign the new handles, and code calling println! will automatically start printing to the new console.

If the user manually closes the console window, then it generates a CTRL_CLOSE_EVENT which simply causes the process to abort so there's no opportunity to have invalid handles.

If the user calls FreeConsole then the handles are closed, but more notably, SetStdHandle is not implicitly called, and GetStdHandle now returns dangling handles which could be reused and point to anything and cause horrible things. Basically, never call FreeConsole unless you're absolutely certain no other threads are printing (or even panicking!) and make sure you also call SetStdHandle at the same time to assign a safe value such as INVALID_HANDLE_VALUE.
Peter Atashian at 2018-01-08 21:35:31
If a process starts without a console the handles returned by GetStdHandle are all 0 which is a safe sentinel value. If AllocConsole is then called to give the process a console, it will implicitly call SetStdHandle to assign the new handles, and code calling println! will automatically start printing to the new console.

So, from this I gather that if the console is allocated and so GetStdHandle() returns a non-sentinel value, then any ERROR_INVALID_HANDLE is an actual "output failed to reach console" error as opposed to a mere artifact of Windows not having a console ready. In which case user should probably be notified of that error, just like he would for, say, EPIPE on Linux, and the current code always ignoring ERROR_INVALID_HANDLE incorrectly conflates two situations.
albel727 at 2018-01-08 22:39:18
Triage: code lives here now https://github.com/rust-lang/rust/blob/16957bd4d3a5377263f76ed74c572aad8e4b7e59/src/libstd/io/stdio.rs#L236-L241
Steve Klabnik at 2020-06-30 23:13:28