Skip to content

Fix #78883: fgets(STDIN) fails on Windows #4961

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.

Sign up for GitHub

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

Jump to bottom
Closed
cmb69 wants to merge 2 commits into from
Closed

Fix #78883: fgets(STDIN) fails on Windows #4961

cmb69 wants to merge 2 commits into from

Conversation

@cmb69
Copy link
Member

@cmb69 cmb69 commented Dec 2, 2019

We add the is_seekable member to php_stdio_stream_data, and prefer
that over is_pipe, since the latter is simply a misnomer. We keep
is_pipe for now for Windows only, though, because we need special
support for pipes there. We also fix the misaligned bitfield which
formerly took 33 bit.

This is the cleaner alternative to PR #4952.

@cmb69
Fix #78883: fgets(STDIN) fails on Windows
ad527d1 
We add the `is_seekable` member to `php_stdio_stream_data`, and prefer
that over `is_pipe`, since the latter is simply a misnomer.  We keep
`is_pipe` for now for Windows only, though, because we need special
support for pipes there.  We also fix the misaligned bitfield which
formerly took 33 bit.
@cmb69 cmb69 mentioned this pull request Dec 2, 2019
Closed
@cmb69
Copy link
Member Author

cmb69 commented Dec 2, 2019

This is an important bugfix for Windows (PHP 7.4.0 can't run any interactive console app), so the PR should be addressed ASAP (IOW, before 7.4.1RC1 is tagged tomorrow). Thanks!

@cmb69 cmb69 added the Bug label Dec 2, 2019
weltling
weltling suggested changes Dec 2, 2019
View reviewed changes
weltling
weltling reviewed Dec 2, 2019
View reviewed changes
@weltling
Copy link
Contributor

weltling commented Dec 2, 2019

Thanks, @cmb69, for checking on this direction. The old code path would be preserved this way, and is_seekable would be used just for the given purpose.

One thing yet - if a stream is created not with *_from_fd, it should have this default to true. To check at the stream allocation time.

Thanks.

@cmb69
Copy link
Member Author

cmb69 commented Dec 2, 2019

Thanks for reviewing @weltling!

if a stream is created not with *_from_fd, it should have this default to true

Sorry, I don't understand. As it's now, _from_file and _from_fd dynamically detect that flags. Do you mean that is_pipe should default to true for these cases?

@weltling
Copy link
Contributor

weltling commented Dec 2, 2019

@cmb69, actually I was talking about is_seekable being true by default. Just for the case, so it's not left uninitialized occasionally. Currently all the code paths seem to initialize it, so looks fine overall.

Thanks.

@cmb69
Copy link
Member Author

cmb69 commented Dec 2, 2019

Thanks! Applied as 996f217.

@cmb69 cmb69 closed this Dec 2, 2019
@cmb69 cmb69 deleted the cmb/fix-78883-2 branch December 2, 2019 15:55
@samdark
Copy link

samdark commented Dec 5, 2019

@weltling any estimate on 7.4.1 release date?

@cmb69
Copy link
Member Author

cmb69 commented Dec 5, 2019

PHP 7.4.1 is scheduled for Dec, 19th.

@MacFJA MacFJA mentioned this pull request Apr 19, 2020
Closed
@kenjis kenjis mentioned this pull request Oct 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@weltling weltling weltling approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cmb69 @weltling @samdark