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

Allow to not close stream on rscr dtor in php cli sapi #8833

Merged
merged 1 commit into from
Jul 18, 2022

Conversation

bukka
Copy link
Member

@bukka bukka commented Jun 20, 2022

This change allows not closing stream handle when resource dtor is being done. Fixes #8827

@cmb69
Copy link
Member

cmb69 commented Jun 20, 2022

Ah, nice, thank you! I assume it is best to apply this to "master" only, and revert the changes to PHP-8.0 and PHP-8.1 altogether.

@bukka
Copy link
Member Author

bukka commented Jun 20, 2022

@cmb69 yes agreed this should go to master only and we should revert the previous changes in 8.0 and 8.1 before the rc is tagged. I will need to do a bit more testing first and look to the possible edge cases but should be hopefully ready in the next few weeks.

cmb69 added a commit that referenced this pull request Jun 20, 2022
We revert the commits which caused this regression from the PHP-8.0 and
PHP-8.1 branches for now.  We keep it in "master" because of PR #8833
which may offer a proper fix without BC break.
Copy link
Member

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Thank you @bukka @cmb69

Should we also add some tests for this ? #8827 includes one that checks that STDERR can be closed

@moonlitbugs
Copy link

@arnaud-lb , The test cited in #8827 only checks whether STDERR can be closed from within PHP. The STDERR global shows itself closed. But that is not the problem. The problem is that, when STDERR is closed, file descriptor 2 is open. Ditto STDIN/STDOUT.

As I noted in #8827, php-8.1.8 leaves /dev/fd/{0,1,2} open post-fclose(), despite the NEWS claiming otherwise.

@fds242
Copy link

fds242 commented Jul 6, 2022

@moonlitbugs

The test cited in #8827 only checks whether STDERR can be closed from within PHP.

I fail to see what your issue is with the simple test I provided, since it continues to demonstrate the issue's existence in PHP 8.1.8, just as well as it did in 8.1.7, and also correctly pointing out every version which functioned correctly in the past. It cares not what the fclose() call returns, and instead tests if it php://stderr can be immediately reopened after the close attempt. Which it can, in the broken versions.
In any case, you're welcome to provide a better test.

I chose to test STDERR since what else could be reasonably tested in both a .phpt test and on ev4l.org? You don't control stdin; and stdout should not be messed with, since if you are able to successfully close it, you have also lost the ability for the test to output any results. As the bug has always equally affected all three std handles by design, seeing if the issue exists for stderr ought to be enough in this case.

@moonlitbugs
Copy link

@moonlitbugs

I fail to see what your issue is with the simple test I provided, since it continues to demonstrate the issue's existence in PHP 8.1.8, just as well as it did in 8.1.7

Ah, then I spoke too soon, my bad. I wrote that, as I remember that closing STDERR succeeds, var_dump() confirms, etc., yet the file descriptor itself is still open. Attempts at reference would return false, or throw an error. I was thinking that an external check of the file descriptor (system("lsof ...")) would be necessary. But I can see now that fopen("php://stderr") could work.

@bukka bukka force-pushed the stream_no_rscr_dtor_close branch from df033e3 to 945bd53 Compare July 6, 2022 19:36
@bukka bukka changed the title Allow not close stream on rscr dtor in php cli sapi Allow to not close stream on rscr dtor in php cli sapi Jul 6, 2022
@bukka
Copy link
Member Author

bukka commented Jul 6, 2022

I just realised that I haven't pushed that test so pushed now. It's still not completely ready as I need to check some edge cases.

@bukka bukka force-pushed the stream_no_rscr_dtor_close branch 3 times, most recently from 4de8207 to d543ced Compare July 17, 2022 22:08
@bukka bukka force-pushed the stream_no_rscr_dtor_close branch from d543ced to d922298 Compare July 17, 2022 22:35
@bukka bukka force-pushed the stream_no_rscr_dtor_close branch from d922298 to 0a4a55f Compare July 18, 2022 09:58
@bukka bukka merged commit 0a4a55f into php:master Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: the fix for #8575 makes it impossible to intentionally close std handles
5 participants