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

Handle closed stdio streams #4685

Closed
wants to merge 3 commits into from
Closed

Handle closed stdio streams #4685

wants to merge 3 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Sep 6, 2019

Detect closed stdio streams on startup and open a dummy stream for
them on which all operations fail. This avoids random FDs being
adopted as stdio streams.

Fixes bug #74252.

@nikic
Copy link
Member Author

nikic commented Sep 6, 2019

cc @kelunik @bwoebi

Overall I kind of get the impression that closed stdio streams are really not supported, even at the C level (the stdin etc FILE handles point to closed FDs).

I think this is going to make STDIN etc behave in a somewhat more predictable way, but there are probably enough other places that will exhibit broken behavior (things directly using stdout for example, or inheriting FDs in proc_open or whatever).

@nikic
Copy link
Member Author

nikic commented Sep 6, 2019

We might be better off not defining STDIN and friends at all if the FDs are closed.

@bwoebi
Copy link
Member

bwoebi commented Sep 6, 2019

I think it's better to leave STDIN defined. there's no value in distinguishing between stdin closed at program start or closed after a few ms. It would just add a burden of an additional check - feof() isn't enough then.
If it's that broken, then we should maybe set stdin to a fake closed resource.
I would like to keep the details of this out of userland.

@kelunik
Copy link
Member

kelunik commented Sep 7, 2019

I also think that STDIN should be defined even if the stream is closed. LGTM.

@nikic
Copy link
Member Author

nikic commented Sep 9, 2019

Unfortunately it looks like STDIN still gets overridden on CI. Something is opening an FD before the validity check runs.

Detect closed stdio streams on startup and open a dummy stream for
them on which all operations fail. This avoids random FDs being
adopted as stdio streams.

Fixes bug #74252.
@nikic
Copy link
Member Author

nikic commented Sep 17, 2019

I'm now checking stdio stream validity directly on entry into main() and something is still overwriting FD 0. Probably some library we link is opening /dev/urandom or so during startup.

I think at this point this issue is simply "Won't Fix". Don't close stdio streams, the behavior is ill-defined. Redirect to /dev/null instead.

@nikic nikic closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants