Hi Christoph, Greateful thanks for the checks.
> -----Original Message----- > From: Christoph Becker [mailto:cmbecke...@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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
