Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86981
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain belski.net from 85.214.73.107 cause and error)
To: "'Christoph Becker'" <cmbecker69@gmx.de>,
	"'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> <55931E47.6030209@gmx.de>
In-Reply-To: <55931E47.6030209@gmx.de>
Date: Wed, 1 Jul 2015 01:48:07 +0200
Message-ID: <059101d0b38f$38e855b0$aab90110$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQHdK+UcnwgdqR38ERSGNmq98XKlbwKZxrYHnZfs/rA=
Content-Language: en-us
Subject: RE: [PHP-DEV] Re: Fixes for #69900 and #69963
From: anatol.php@belski.net ("Anatol Belski")

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
>=20
> Hi!
>=20
> Anatol Belski wrote:
>=20
> > regarding the mentioned tickets I've prepared a patch
> > https://github.com/php/php-
> src/compare/master...weltling:pipe_blocking_win .
>=20
> Great.
>=20
> > 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
>=20
> 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 -
> >
> > =3D=3D=3D=3D SNIPPET =3D=3D=3D=3D
> > /* here proc_open() will do a blocking read on the child process*/
> > $process =3D proc_open(PHP_BINARY.' -f ' . $fl, $descriptorspec, =
$pipes,
> > NULL, NULL, array("blocking_pipes" =3D> true)); =3D=3D=3D=3D END =
SNIPPET =3D=3D=3D=3D
> >
> > =3D=3D=3D=3D SNIPPET =3D=3D=3D=3D
> > $in =3D fopen("php://stdin", "rb", false, =
stream_context_create(array("pipe"
> > =3D> array("blocking" =3D> true))));
> > +while(!feof($in)){ /* here feof() will do a blocking read on the =
pipe
> > +*/
> > .........................
> > =3D=3D=3D=3D END SNIPPET =3D=3D=3D=3D
> >
> > 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.
>=20
> 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:
>=20
> -	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