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

[fds242]

Description

On Unix-like systems, closing a std file handle has the useful property that the next file handle you open will inherit its place. This provides a very simple and effective way to reroute stdin/stdout/stderr during run time.

This has historically also been possible through PHP-CLI scripts, and as a result has become common practice, oft-repeated boilerplate code for using pcntl_fork() in particular, where it is almost always desirable for the forked children to stop using the parent's stdin/stdout/stderr.

This used to be as simple as, for example:

<?php
fclose(STDIN);
fclose(STDOUT);
fclose(STDERR);
$in = fopen('/dev/null', 'r');
$out = fopen('stdout-now-goes-here.txt', 'a');
$err = fopen('stderr-now-goes-here.txt', 'a');

The fix for an unrelated issue in #8575, and unfortunately included in both PHP 8.1.7 and PHP 8.0.20, also makes it impossible for PHP code to willfully close the std file handles anymore. This breaks a whole swath of past code that depended on this functionality, a rather unwelcome and unexpected change for minor releases.

Here's a test:

--TEST--
std handles can be deliberately closed
--SKIPIF--
if (php_sapi_name() != "cli") {
	die("skip CLI only");
}
if (substr(PHP_OS, 0, 3) == 'WIN') {
	die("skip not for Windows");
}
--FILE--
<?php
fclose(STDERR);
var_dump(@fopen('php://stderr', 'a'));
?>
--EXPECT--
bool(false)

With PHP 8.1.7 or 8.0.20 this now results in the following output instead:

resource(5) of type (stream)

3v4l link: https://3v4l.org/OgYWN

PHP Version

PHP 8.1.7

Operating System

Linux, macOS

[cmb69]

This is about #8571, I assume.

[cmb69]

IMO, we should revert the fixes for now, so we can ship PHP 8.0.21RC1 and 8.1.8RC1 without the regression, and then reconsider how to best address #8575.

[bukka]

I agree. #8571 is a regression and should be reverted.

[cmb69]

I have reverted the commits from PHP-8.0 and PHP-8.1; "master" will be addressed by #8833.

[moonlitbugs]

The NEWS file from PHP 8.1.8 states:

- CLI:
  . Fixed GH-8827 (Intentionally closing std handles no longer possible). (cmb)

This code does not appear to have been tested, as the commits made to sapi/cli/php_cli.c have no effect.
Calling fclose(STDIN); leaves file descriptor 0 open. Ditto STDOUT, STDERR.

Once again, I have replaced sapi/cli/php_cli.c with the same file extracted from php-8.1.6.tar.gz, and with this revert, STDIN/STDOUT/STDERR can now be closed.

[fds242]

As I understand, the fix for now was intended to be just that– zero new code, backing out the patch that caused the issue, and going back to the code from the previous, working versions.

Alas, I can indeed confirm that while the reversal in the PHP 8.0 branch appears to have been successful, in the PHP 8.1 branch it was not, and still includes the offending change.
See the code in cli_register_file_handles indiscriminately applying the PHP_STREAM_FLAG_NO_CLOSE flag to all the std handles.

[cmb69]

Yes, this revert was bad. Sorry! I'll give that another try ASAP. Feel free to open another bug report about this issue, since we can't re-use this ticket number.