Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86996
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> <059101d0b38f$38e855b0$aab90110$@belski.net> <55947875.1040009@gmx.de>
In-Reply-To: <55947875.1040009@gmx.de>
Date: Thu, 2 Jul 2015 12:46:57 +0200
Message-ID: <078001d0b4b4$6db1dbb0$49159310$@belski.net>
MIME-Version: 1.0
Content-Type: text/plain;
	charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQHdK+UcnwgdqR38ERSGNmq98XKlbwKZxrYHAh3vSRgCWmRtT512a4fw
Content-Language: en-us
Subject: RE: [PHP-DEV] Re: Fixes for #69900 and #69963
From: anatol.php@belski.net ("Anatol Belski")

Hi Christoph,

> -----Original Message-----
> From: Christoph Becker [mailto:cmbecker69@gmx.de]
> Sent: Thursday, July 2, 2015 1:32 AM
> To: Anatol Belski; 'PHP Internals'
> Cc: 'Pierre Joye'; 'Kalle Sommer Nielsen'; 'Dmitry Stogov'; 'Xinchen =
Hui'; 'Nikita
> Popov'
> Subject: Re: [PHP-DEV] Re: Fixes for #69900 and #69963
>=20
> Hi Anatol!
>=20
> Anatol Belski wrote:
>=20
> >> -----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
> >>
> >> 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 :)
>=20
> I did only a quick check on a single machine, and that worked, but of =
course that
> doesn't mean much.  If it's generally a problem, it seems to be best =
to stick with
> the default, and to document the limited buffer size.
>=20
Yeah, this is interesting anyway. As mentioned, I haven't seen very =
reliable results with it. Also reading once more the MSDN docs, they say =
" The size is only a suggestion; the system uses the value to calculate =
an appropriate buffering mechanism. If this parameter is zero, the =
system uses the default buffer size." ... which probably has the point =
:( For instance, if you set the buffer size to 1 byte (say want no =
buffering), that most likely won't work. It looks more like the =
buffering in the C runtime where we cannot do much.

But where you've got the point is that - on systems where it works, it =
could be one more small detail improving behavior (however would be hard =
to debug and reproduce these parts on different systems). Though bugs =
like #64438 were to recheck again. But where we directly use CreatePipe =
is proc_open. Anyway, it's needs more investigation.

Another thing for proc_open which could be useful I thought about were =
another win only config to pass the timeout from user land. Then it'd =
combine both safe pipe operations and pseudo blocking read behavior. =
However this way the timeout will need to be passed to the streams, and =
php_stdio_stream_data struct will have be extended with at least one =
more 4 byte member.

In general, IMHO, we probably need some refactoring there, instead of =
doing these hotfixes and adding evermore #ifdefs and all that :) =
Particularly with pipes, blocking, sockets, file descriptors, syscalls, =
etc. ... a stronger abstraction by platform would give more flexibility =
to do fixes (like not waiting for the next major), but what is more =
important - it would allow usage of platform specific APIs which often =
bring better results. The world gets evermore asynchronous, anyway :)

> >> 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.
>=20
> When I run the test without the patch it works, except for the =
timings.
>  So it seems they are necessary for the test to be useful. Even though =
such
> timings are generally fragile, I don't have a better idea.
>=20
> However, the "0%sms" test[1] fails here.  It seems that %s doesn't =
match an
> empty string.  I've attached a patch that works for me (I hope it gets =
through to
> the list).  Please consider to use it.
Yeah, makes sense. I've applied this, but still not sure the timings =
should be kept. Whereby no other choice - it's the only way to check the =
correct behavior, but it'll fail with valgrind.

>=20
> >> 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.
>=20
> Ah, yes, I see.  On 32bit architectures it seems to increase the =
struct size from 64
> to 68 bytes (checked with MSVC), though.  I consider this an extremely =
minor
> issue, but still it bugs me a bit to have three bitfields of size 1 =
and another
> boolean declared as unsigned.  Maybe it's cleaner to declare all of
> is_process_pipe, is_pipe, cached_fstat and is_pipe_blocking as =
zend_bool?
Applied your first suggestion, thanks!

Kind regards

Anatol