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

Don't panic when stdout doesn't exist #1014

Merged
merged 4 commits into from
Jun 2, 2015

Conversation

retep998
Copy link
Member

Signed-off-by: Peter Atashian <retep998@gmail.com>

# Detailed design

Change the internal implementation of `println!` `print!` `panic!` and `assert!` to not `panic!` when `stdout` or `stderr` doesn't exist. When getting `stdout` or `stderr` through the `std::io` methods, those versions should continue to return an error if `stdout` or `stderr` doesn't exist.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, does this RFC propose ignoring all errors or just those related to a missing stdout/stderr? It sounds like the latter, but I just wanted to make sure.

Also, do you think that the stdin behavior should remain the same as-is today or act as if it were a io::empty instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It proposes ignoring only a missing stdout/stderr.

I currently don't hold an opinion on what to do with stdin considering there is no convenience macro for it, but if an argument can be made for something better than the current behavior, I'll update the RFC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now there is an unwrap for Windows when getting a handle to the stdin of the process. This RFC would probably alter that to return something akin to io::Empty though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return Result from those methods instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's certainly an alternative! It breaks the signatures as-is today, however.

@nagisa
Copy link
Member

nagisa commented Mar 27, 2015

I’d find it pretty nice if {print,write}{,ln}! returned an io::Result<()>, which doesn’t have to be used. This way println!("hey!") would still work just fine without warnings and people could handle the error if they desired so.

On the other hand, it becomes too easy to ignore other I/O errors that might arise.

@bill-myers
Copy link

What does "stdout does not exist" mean on Unix?

Does it mean that there is no open file with fd 1?

The problem of this situation is that newly opened files will be assigned fd 1 automatically (at least on Linux with both open() and fopen()), and then accidentally get used as stdout if stdout missing is checked more than once, or if C libraries are called, or if the files are not CLOEXEC and processes are spawned.

So it might be worth thinking whether automatically opening /dev/null in fd 1 (and likewise fd 0 and 2) makes sense.

There are problems though with this, namely the fact that it would need to be done at process init to be effective, automatically altering the fd table, and requiring /dev to be present (if /dev is not mounted, one could instead use pipe() and spawn a thread to read from the other end, although this is pretty horrible).

This has potential security implications as well, since running a setuid program with no fd 0/1/2 could make it write data to random opened files (maybe something already mitigates this?).

@alexcrichton alexcrichton self-assigned this Apr 9, 2015
Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
Signed-off-by: Peter Atashian <retep998@gmail.com>
@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label May 22, 2015
@DanielKeep
Copy link

An alternative would be to provide some functions in the standard library to put the standard streams into a "known good" configuration. I don't think panicking on std* not being there is too problematic provided there's a reasonably simple way to stop it from happening. Preferably one that works cross-platform and can dynamically detect the bad condition.

To bring up the situation with Python, I recall jumping through some rather questionable hoops to automatically detect missing stdout/stderr and redirect them to files.

@llogiq
Copy link
Contributor

llogiq commented May 27, 2015

On Linux stdout almost always exists

This does not hold true if our process is daemonized.

My two cents: Doing something automatically is un-rusty, because it will probably be the wrong thing in some situations. Thus leaving a possible (though unlikely) error to the user to handle (as in returning an IoResult) would be the most workable solution. We should also note that this could trigger an unused_result (or some such) warning depending on lint settings – perhaps we should suppress those by default.

@retep998
Copy link
Member Author

I'm against having every single println! return an error stating that stdout doesn't exist, because that's something that is known at startup and should be handled at startup. Besides, how would you handle the error when panic! is unable to print to stderr?

Perhaps a method can be added that tells you the state of stdout stderr and stdin and another that lets you redirect them. This way if they point to nothingness you can choose to redirect the output to a file or something. Of course they'd still default to what this RFC proposes to prevent programs from panicking and then aborting when stdout and stderr aren't there.

@grissiom
Copy link

Thumb up for this RFC. Even stderr and stdin would exists in some situation.

Hides an error from the user which we may want to expose and may lead to people missing panicks occuring in threads.

I don't think this is a problem. If the program is launched without stdout or stderr, than the user is intent to not seeing back traces from the console. If such back trace info is important, the program should restore them in a file manually.

@llogiq
Copy link
Contributor

llogiq commented May 27, 2015

@retep998

because that's something that is known at startup

Wrong. A daemon has standard streams until it daemonizes. Also on POSIX, an attacker may try to disturb a process by closing it's stdio handles from the outside (those are available from /proc).

Besides, how would you handle the error when panic! is unable to print to stderr?

That's for the user to decide. Perhaps log a message to a file. Or do nothing. Or stop the world, because printing a result is the sole reason for this program to run. As I said, there is no right default solution.

Splitting the print call and the was_I_able_to_print() query in two could be an alternative; however that may be prone to race attacks (where an attacker closes the stream between the query and the action).

@grissiom

the user is intent to not seeing back traces from the console

Not necessarily – as I have outlined above, there may be other interferences leading to loss of stdout/in/...

Also library code may print to stdout for some reason – while we certainly do not want it to crash, we should allow it to do something if an error was silenced.

@retep998
Copy link
Member Author

If your program is being attacked by rogue processes that are trying to cut out your stdout, I think you have bigger problems than worrying about stdout.

@llogiq
Copy link
Contributor

llogiq commented May 27, 2015

Probably. My point was that

  1. closing standard streams is not startup related. It can by definition happen anywhere during the process.
  2. you cannot be sure about the reason why a stream is closed / unavailable.

Case in point: I recently had a long-running process (written in Java if you must know) that I had started for the weekend. My colleague needed to change some settings, but it was running in my shell. So he hijacked my console (he has root access to the server) and sent some commands to the process. However doing so, he killed stdout by mistake. The process threw a ClosedChannelException. There went my weekend.

@retep998
Copy link
Member Author

Perhaps a combination of these things?

  1. println! and print! return Results which can be ignored and do not trigger unused_result.
  2. A way to determine the current status of stdout stderr and stdin.
  3. A way to redirect stdout stderr and stdin.
  4. The current behavior of panicking if stdout or stderr do not exist is squelched since I cannot imagine it is ever desirable for your process to unconditionally panic and abort just because they're missing. If you really want to abort (you weirdo), there are multiple ways to do so such as println!(...).unwrap().

@grissiom
Copy link

For panic!, I don't think the back trace is the important part of it. I think the 1st reason panic! exists is abort the program because bad things happen. If the user could get the backtrace, it's cool. If not, that's OK because we have already saved the world.

I think it is always true that stdout is a easy to use but not reliable. Even "printf(3)" could return negative value to indicate errors(but not abort your program).

@llogiq
Copy link
Contributor

llogiq commented May 27, 2015

@retep998 Sounds good.

  1. they should return std::io::Result, in line with reading/writing elsewhere.
  2. could this also be implemented by reading 0 bytes or writing the empty string?
  3. unconditionally or only when it isn't usable? Note: The latter case may be hard to implement but very useful.
  4. I don't think it's as weird as you imagine, e.g. if the sole reason for my (possibly long-running) process' existence is to print! something, I may want it to abort once it finds that it cannot do so.

We also need to answer the question if there are any programs in the wild that depend on print! & friends' panicky behavior – in that case this would be a [insert drumroll here] breaking change...

@retep998
Copy link
Member Author

unconditionally or only when it isn't usable? Note: The latter case may be hard to implement but very useful.

Well there's three options for that.

  1. Unconditionally redirect output. So if you realize you have no stdout, just redirect everything there from then on, even if stdout reappears, just consistently redirect.
  2. Have a fallback that is only used when stdout doesn't exist, and when it does exist just use stdout.
  3. Have a FnOnce that is invoked to get a fallback the moment a write to stdout fails due to no stdout and that fallback is used from there on out so long as stdout doesn't exist. Useful so that you can open a file for redirection only if redirection is needed.

@llogiq
Copy link
Contributor

llogiq commented May 27, 2015

  1. unconditional redirection is useful in any event. So this should be implemented anyway.
  2. How would we supply that fallback to the print! macro?
  3. This seems strictly a superset of 2 (if we have a closure, we also can return any pre-existing stream).

@retep998
Copy link
Member Author

It seems there is already a way to redirect stdio via std::io::stdio::set_print (and print! does get redirected by that), although it is both unstable and hidden from the docs. It's also thread-local which may or may not be desirable.

I'm not sure how conditional redirects would work, since the only way to tell if redirecting is needed if an attempt is made to get stdout and it fails. Either you'd have to attempt to get stdout every single time you write (which can't be good for performance), or you'd just have to permanently fallback from then on out (unless the user's code attempts to redirect it back to the actual stdout).

@llogiq
Copy link
Contributor

llogiq commented May 27, 2015

The docs on set_print state that it may well be replaced by a "more general mechanism". A good thing is that we only depend on Stdout and Stderr to impl Write. This offers us a lot of flexibility to wrap, exchange or otherwise mangle the streams (however, we should be careful about what happens e.g. with popen'd processes).

And I think having the fallback install permanently on failure would be acceptable. This could also be implemented by a Write handler that attempts to Write to the default stdout, and on failure set up the fallback channel and repeat the Write (note we also may want to extend this behavior to processes started from, but not necessarily written in rust).

@retep998
Copy link
Member Author

To extend the behavior to processes merely started from a Rust process would involve piping their output to our Rust process (so the Rust process would have to live longer than the child), plus it would no longer be a genuine console handle, which on Windows is a significant difference that would impair the child process's ability to use fancy console features.

@llogiq
Copy link
Contributor

llogiq commented May 27, 2015

Yes, a piped handle is distinctly non-interactive (also under Linux). Isn't that a well-known tradeoff? I suppose one could write a terminal emulation in rust, but it should not reside in std::io.

@Gankra
Copy link
Contributor

Gankra commented May 27, 2015

Note: this PR is in its Final Comment Period as of Yesterday

@alexcrichton alexcrichton added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 27, 2015
@retep998
Copy link
Member Author

I'd really rather avoid by default modifying the behavior of stdio we pass to child processes for the sake of protecting them. Child processes can protect themselves and they should be responsible for cases where they inherit a missing stdout, otherwise we're just getting into the realm of overly obtrusive hand-holding.

As for modifying the return value of println!, as far as I can tell there is no way to allow unused_result for a specific function/macro. Never mind that it would be a breaking change to one of the most used methods in Rust.

I think I'm pretty happy with this RFC just stopping your program from panicking (and then aborting) because someone decided to run your process without a console on Windows or as a daemon. If someone wants to handle the errors then they already have the option of using std::io::Stdout which has methods that return Result. Plus this RFC mandates that the future raw stdio methods will return a Result so you'll know before you even attempt to write whether or not the handles exist so you can take immediate action. Any sort of complex error handling or fallback behavior can be implemented on top of those primitives with some sort of wrapper.

@llogiq
Copy link
Contributor

llogiq commented May 28, 2015

👍 from me then.

@sfackler
Copy link
Member

👍 from me.

Even if the process has stdio set up, there's a small window of time during process shutdown when retrieval of the global handle will fail: https://github.com/rust-lang/rust/blob/master/src/libstd/io/stdio.rs#L241. It seems like it'd make sense to return a sink there as well instead of panicking.

@alexcrichton
Copy link
Member

This RFC looks like it has fairly broad consensus at this point, and it fits into the model of picking a reasonable set of defaults for behavior in the standard library. The standard I/O streams are already locked and buffered, so there's a good deal of machinery happening behind the scenes to start out with, and this is not necessarily adding an undue amount of extra burden. Furthermore, raw access to the stdio streams is still planned which will bypass all of these points, allowing for finer-grained control of the output of a program.

Thanks again for the RFC @retep998!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-input-output Proposals relating to std{in, out, err}. A-panic Panics related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.