Fixes for #69900 and #69963

Hi Dmitry,

Thanks for the review :)

Greetings

Anatol

-----Original Message-----
From: Dmitry Stogov [mailto:dmitry@zend.com]
Sent: Tuesday, June 30, 2015 9:45 PM
To: Anatol Belski
Cc: PHP Internals; Pierre Joye; Kalle Sommer Nielsen; Xinchen Hui; Nikita Popov
Subject: [PHP-DEV] Re: Fixes for #69900 and #69963

Hi Anatol,

I don't see any problems committing this.

Thanks. Dmitry.

On Tue, Jun 30, 2015 at 9:41 PM, Anatol Belski anatol.php@belski.net
wrote:

Hi,

regarding the mentioned tickets I've prepared a patch
https://github.com/php/php-src/compare/master...weltling:pipe_blocking
_win
.
Anonymous pipes on Windows

• don't support asynchronous IO
• don't support select()
• have buffer size of 4096 bytes (or even less) which is much easier
  overflown that a buffer of 64kb often to be seen on Unix

These issues was addressed last year, but the tickets mentioned report
about the issues still persisting in PHP5/7. While it's likely a no go
for PHP5, I would like to push this patch into PHP7. Particularly what
it does is allowing the blocking read() again for the edge cases
reported in the listed tickets. It is done adding a context option and
a new win only option for proc_open. Just to illustrate a few -

==== SNIPPET ====
/* here proc_open() will do a blocking read on the child process*/
$process = proc_open(PHP_BINARY.' -f ' . $fl, $descriptorspec, $pipes,
NULL, NULL, array("blocking_pipes" => true)); ==== END SNIPPET ====

==== SNIPPET ====
$in = fopen("php://stdin", "rb", false, stream_context_create(array("pipe"
=> array("blocking" => true))));
+while(!feof($in)){ /* here feof() will do a blocking read on the pipe
+*/
........................
==== END SNIPPET ====

The only drawback on this is that blocking reads on pipes are likely
to cause pipe buffer overflows and thus deadlock, like it was before.
Though, when working with them carefully, this could be avoided and
would bring improvements with several edge cases, almost when working
on CLI. Anyway this patch brings no difference to PHP5 while addressing those
edge cases.

I would like to ask for review here. I've also made a minimal build
with this patch http://windows.php.net/downloads/snaps/ostc/69900/ for
those interested to test. If there are no strong objections, it should
be merged soon.

Thanks

Anatol

Hi Christoph,

Greateful thanks for the checks.

-----Original Message-----
From: Christoph Becker [mailto:cmbecker69@gmx.de]
Sent: Wednesday, July 1, 2015 12:55 AM
To: Anatol Belski; 'PHP Internals'
Cc: 'Pierre Joye'; 'Kalle Sommer Nielsen'; 'Dmitry Stogov'; 'Xinchen Hui'; 'Nikita
Popov'
Subject: [PHP-DEV] Re: Fixes for #69900 and #69963

Hi!

Anatol Belski wrote:

regarding the mentioned tickets I've prepared a patch
https://github.com/php/php-
src/compare/master...weltling:pipe_blocking_win .

Great.

Anonymous pipes on Windows

• don't support asynchronous IO
• don't support select()
• have buffer size of 4096 bytes (or even less) which is much easier
  overflown that a buffer of 64kb often to be seen on Unix

It appears to be possible to set the buffer size by passing a nSize argument > 0 to
CreatePipe[1][2]. Maybe it's reasonable to use 16 or 64 kB by default?
Does it work? To my current experience, the buffer size is not to change manually, the system pipe manager decides it. So even it could work on one machine, it could fail on another. Still one could verify with GetNamedPipeInfo, which didn't show reliable results for me. In general, a bigger pipe buffer size is what makes the Linux implementation less vulnerable. And, because most of the previous behavior, blocking reads are expected for some long running daemons and co. But here's the terrible mix about "read forever" and "dead lock", I don't see a solution as long as we use anonymous pipes on Windows, but this is completely another story to discuss. Thus - the addition of user space args, count with users who know what they're doing, the >99% of usage is covered by what we have now :)

These issues was addressed last year, but the tickets mentioned report
about the issues still persisting in PHP5/7. While it's likely a no go
for PHP5, I would like to push this patch into PHP7. Particularly what
it does is allowing the blocking read() again for the edge cases
reported in the listed tickets. It is done adding a context option and
a new win only option for proc_open. Just to illustrate a few -

==== SNIPPET ====
/* here proc_open() will do a blocking read on the child process*/
$process = proc_open(PHP_BINARY.' -f ' . $fl, $descriptorspec, $pipes,
NULL, NULL, array("blocking_pipes" => true)); ==== END SNIPPET ====

==== SNIPPET ====
$in = fopen("php://stdin", "rb", false, stream_context_create(array("pipe"
=> array("blocking" => true))));
+while(!feof($in)){ /* here feof() will do a blocking read on the pipe
+*/
.........................
==== END SNIPPET ====

The only drawback on this is that blocking reads on pipes are likely
to cause pipe buffer overflows and thus deadlock, like it was before.
Though, when working with them carefully, this could be avoided and
would bring improvements with several edge cases, almost when working
on CLI. Anyway this patch brings no difference to PHP5 while addressing those
edge cases.

I would like to ask for review here. I've also made a minimal build
with this patch http://windows.php.net/downloads/snaps/ostc/69900/ for
those interested to test. If there are no strong objections, it should
be merged soon.

I did some quick tests and the patch works fine. However,
proc_open_bug69900.phpt fails on my machine (Win 7), because 0ms are
reported for all but the first read (microtime issue on older Windows').
Maybe it's better to EXPECT something like "took less than 1 ms"?
Good catch. Or maybe 0%sms ... or alike ... still a platform issue, nothing with PHP itself. The speaking result here is that a persistent connection with pushing small chunks delivers correct, maybe removing timings would suffice as well, please advise.

I have noticed that you've changed the bitfields of struct
php_stdio_stream_data (in plain_wrapper.c). Wouldn't it be better to do:

• unsigned is_pipe_blocking;

•   unsigned is_pipe_blocking:1;
  

•   unsigned _reserved:28;
  

Nope, reserved was there to indicate the alignment (between 32- and 64-bit?). Replacing _reserved does no change to the struct size. Still one could preserve it on non Windows, but it's kinda useless as any further change would need an additional field, so then the order were the question. I was thinking about making it zend_bool, still what other three zend_bools would go into the remaining space? Still could be done if some situation arises, so no loss here - it's encapsulated in a C file, not exposed in a header.

Regards

Anatol