Fix #78883: fgets(STDIN) fails on Windows

[cmb69]

If a character file is detected as pipe, we have to set the Windows
only flag is_pipe_blocking.

[cmb69]

cc @weltling, @nikic

[nikic]

Is it possible to write a test for this somehow?

[cmb69]

I certainly would like to have a regression test for this (this bug should not have hit a GA version), but I'm afraid we can't have a PHPT, since as soon as STDIN is coming from a pipe, the bug would not trigger (STDIN must be FILE_TYPE_CHAR to exhibit the bug). Maybe @weltling or @KalleZ have an idea how this could be tested, nonetheless.

[weltling]

It seems to me, that the detection is not quite correct in first place.

self->is_pipe = file_type == FILE_TYPE_PIPE || file_type == FILE_TYPE_CHAR;

FILE_TYPE_PIPE is only what is relevant to tell whether it's a pipe. In this case, seems detect_is_pipe name is not matching with what is actually returned. To tell whether something is a real tty, it's probably better to use php_win32_console_fileno_is_console.

Making a pipe blocking by default also seems not the right way - it was especially done this way to avoid race conditions where reads would block forever due to the pipe buffering. Especially this concerns proc_open, as it can only use anonymous pipes.

I'd suggest to rather to revisit the condition above to rework the handling for ensuring stdio is not seekable. Perhaps introduce a new flag like self->is_seekable. Handling stdio as a pipe is definitely not a correct behavior, unless it actually is a pipe.

Thanks.

[cmb69]

Thanks @weltling!

I fully agree that the naming is unfortunate (of course, a character device is not a pipe), but it seems to me that @nikic's fix (9ec61e4) is basically correct, since php_stdio_stream_data.is_pipe is almost always (see next paragraph) used to guard from seeking or telling, especially since there are several cases where failing ftell() calls are not properly detected (and according to the Microsoft documentation the return value is undefined when ftell() is called on devices incapable of seeking). Since that fix sets the is_pipe flag for character devices on non Windows systems, it seems to me that it makes sense to also do so on Windows.

The only case where is_pipe is not used to guard from seeking or telling, is the code where we're peeking the pipe to avoid blocking. Since PeekNamedPipe() must not be called on character device files, this PR sets the is_pipe_blocking flag, but only for character devices. So yes, the naming is not correct, but unless I'm missing something, the code does the right things.

It might be sensible to do the renaming right away (e.g. is_pipe could become is_not_seekable), but I wouldn't mind to do that for master only, and also to revise the is_process_pipe which currently seems to always have the same value as is_pipe (and as such would be superfluous).

[weltling]

@cmb69 thanks for checking.

Especially for the windows implementation, it is inconsistent and will lead to issues on other ends. For example, anonymous/named pipes with regard to non blocking can use distinct handling. Separating these flags instead of overloading them will make this implementation future proof.

PeekNamedPipe() must not be called on character device files

This is the exact point. Because it checks for is_pipe before, and that is broken by the condition change. Putting more hotfixes on this will introduce more inconsistency.

Thanks.

[cmb69]

Thanks @weltling! I have now submitted PR #4961, which is indeed cleaner. Changing the php_stdio_stream_data structure doesn't seem to be an issue, since it is used only internally.

[divinity76]

as for testing it, i suppose we could use proc_open() to start a cmd.exe and manipulate cmd.exe through stdin to start php -r 'var_dump(fgets(STDIN));' , give it a second to execute, then terminate it and inspect stdout? something like

<?php
declare(strict_types=1);
$descriptorspec = array(
   0 => array("pipe", "rb"),
   1 => array("pipe", "wb"),
   //2 => array("file", "stderr.txt", "ab")
);
$pipes=[];
$cmd=proc_open("cmd.exe",$descriptorspec,$pipes);
fwrite($pipes[0],"php -r ".escapeshellarg("var_dump(fgets(STDIN));")."\x0A"); // 0x0A should be enter key, but i can't actually test it now
sleep(1);
proc_terminate($proc);
fwrite($pipes[0],"exit\x0A");
fclose($pipes[0]);
$stdout=stream_get_contents($pipes[1]);
fclose($pipes[1]);
proc_close($proc);
var_dump( false===strpos($stdout,"bool(false)"));

.. untested (the only windows system i can test on now has PHP installed through Cygwin, which is a linux-y build of php, not suitable for testing this), but something like that may work? (also idk how to actually send SIGINT to the child, something like ctrl+C, that's what i attempted with proc_terminate($proc); )

[cmb69]

@divinity76, communicating via pipes won't exhibit the bug.

[divinity76]

@cmb69 well did some fiddling, this script prints bool(true) on php 7.4.0, and prints bool(false) on 7.4.1, probably because it identifies this bug:

<?php
declare(strict_types=1);
$descriptorspec = array(
   0 => array("pipe", "rb"),
   1 => array("pipe", "wb"),
   //2 => array("file", "stderr.txt", "ab")
);
$pipes=[];
$cmd=proc_open('cmd.exe "/c START ^"^" /WAIT php -r ^"var_dump(fgets(STDIN));"',$descriptorspec,$pipes);
if(!$cmd){
	var_dump(error_get_last());
	throw new \RuntimeException("failed to start cmd.exe");
}
$cmdpid=proc_get_status($cmd)['pid'];
sleep(1);
$bug_is_present=!proc_get_status($cmd)['running'];
if(!$bug_is_present){
	// if the bug is not present, it will hang waiting for stdin,
	// thus cmd is still running and we should kill it
	shell_exec("taskkill /T /F /PID {$cmdpid}");
}
var_dump($bug_is_present);
fclose($pipes[0]);
fclose($pipes[1]);
proc_close($cmd);

[cmb69]

Thanks for this great solution! I've added the test as 09e76cb.