Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86979
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.15.18 as permitted sender)
Message-ID: <55931E47.6030209@gmx.de>
Date: Wed, 01 Jul 2015 00:55:03 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: Anatol Belski <anatol.php@belski.net>, 
 'PHP Internals' <internals@lists.php.net>
CC: 'Pierre Joye' <pierre.php@gmail.com>, 
 'Kalle Sommer Nielsen' <kalle@php.net>,
 'Dmitry Stogov' <dmitry@zend.com>, 'Xinchen Hui' <laruence@php.net>, 
 'Nikita Popov' <nikita.ppv@gmail.com>
References: <055b01d0b364$5414b890$fc3e29b0$@belski.net>
In-Reply-To: <055b01d0b364$5414b890$fc3e29b0$@belski.net>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Subject: Re: Fixes for #69900 and #69963
From: cmbecker69@gmx.de (Christoph Becker)

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?

> 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"?

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;

[1]
<https://msdn.microsoft.com/en-us/library/windows/desktop/aa365152(v=vs.85).aspx>
[2]
<https://github.com/php/php-src/blob/php-5.6.10/ext/standard/proc_open.c#L414>

-- 
Christoph M. Becker