Open albel727 opened 6 years ago
cc @alexcrichton
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!
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.
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.
Just so that everyone understands Linux descriptor reuse on a concrete example.
https://play.rust-lang.org/?gist=b44be7df66e7dd59ce25209927523782&version=stable
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
.
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.
Triage: code lives here now https://github.com/rust-lang/rust/blob/16957bd4d3a5377263f76ed74c572aad8e4b7e59/src/libstd/io/stdio.rs#L236-L241
I've discovered, that
std::io::stdin/out/err()
streams unconditionally ignoreEBADF
-like IO errors on all platforms. This is done by checking the read/write error in ahandle_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 happilyprintln!()
-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 ignoreEBADF
, but to panic(), before something else unwittingly allocated it with likely disastrous consequences.