Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:86990
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmx.de designates 212.227.17.21 as permitted sender)
Message-ID: <55947875.1040009@gmx.de>
Date: Thu, 02 Jul 2015 01:32:05 +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> <55931E47.6030209@gmx.de> <059101d0b38f$38e855b0$aab90110$@belski.net>
In-Reply-To: <059101d0b38f$38e855b0$aab90110$@belski.net>
Content-Type: multipart/mixed;
 boundary="------------040605000506010601090503"
Subject: Re: [PHP-DEV] Re: Fixes for #69900 and #69963
From: cmbecker69@gmx.de (Christoph Becker)

--------------040605000506010601090503
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

Hi Anatol!

Anatol Belski wrote:

>> -----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 :)

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.

>> 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.

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.

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.

>> 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.

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?

[1]
<https://github.com/php/php-src/commit/7fbfd81d3e0242d73f7662ecb830c857704a4c5e>

-- 
Christoph M. Becker

--------------040605000506010601090503
Content-Type: text/plain; charset=windows-1252;
 name="proc_open_bug69900.phpt.patch.txt"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="proc_open_bug69900.phpt.patch.txt"

IGV4dC9zdGFuZGFyZC90ZXN0cy9zdHJlYW1zL3Byb2Nfb3Blbl9idWc2OTkwMC5waHB0IHwg
MjQgKysrKysrKysrKystLS0tLS0tLS0tLQogMSBmaWxlIGNoYW5nZWQsIDEyIGluc2VydGlv
bnMoKyksIDEyIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2V4dC9zdGFuZGFyZC90ZXN0
cy9zdHJlYW1zL3Byb2Nfb3Blbl9idWc2OTkwMC5waHB0IGIvZXh0L3N0YW5kYXJkL3Rlc3Rz
L3N0cmVhbXMvcHJvY19vcGVuX2J1ZzY5OTAwLnBocHQKaW5kZXggYjM1MjMyOS4uYzk4ZmQ5
NyAxMDA2NDQKLS0tIGEvZXh0L3N0YW5kYXJkL3Rlc3RzL3N0cmVhbXMvcHJvY19vcGVuX2J1
ZzY5OTAwLnBocHQKKysrIGIvZXh0L3N0YW5kYXJkL3Rlc3RzL3N0cmVhbXMvcHJvY19vcGVu
X2J1ZzY5OTAwLnBocHQKQEAgLTMzLDcgKzMzLDcgQEAgZm9yKCRpID0gMDsgJGkgPCAxMDsg
JGkrKyl7CiAJJHQxID0gbWljcm90aW1lKDEpOwogCiAJZWNobyAkczsJCQotCWVjaG8gImZn
ZXRzKCkgdG9vayAiLCAoKCR0MSAtICR0MCkqMTAwMCksICJtc1xuIjsKKwllY2hvICJmZ2V0
cygpIHRvb2sgIiwgKCgkdDEgLSAkdDApKjEwMDAgPiAxID8gJ21vcmUnIDogJ2xlc3MnKSwg
IiB0aGFuIDEgbXNcbiI7CiB9CiAKIGZjbG9zZSgkcGlwZXNbMF0pOwpAQCAtNDgsMjUgKzQ4
LDI1IEBAIHByb2NfY2xvc2UoJHByb2Nlc3MpOwogJGZsID0gZGlybmFtZShfX0ZJTEVfXykg
LiBESVJFQ1RPUllfU0VQQVJBVE9SIC4gInRlc3Q2OTkwMC5waHAiOwogQHVubGluaygkZmwp
OwogPz4KLS0tRVhQRUNURi0tCistLUVYUEVDVC0tCiBoZWxsbzAKLWZnZXRzKCkgdG9vayAl
ZCVzbXMKK2ZnZXRzKCkgdG9vayBtb3JlIHRoYW4gMSBtcwogaGVsbG8xCi1mZ2V0cygpIHRv
b2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4gMSBtcwogaGVsbG8yCi1mZ2V0cygp
IHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4gMSBtcwogaGVsbG8zCi1mZ2V0
cygpIHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4gMSBtcwogaGVsbG80Ci1m
Z2V0cygpIHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4gMSBtcwogaGVsbG81
Ci1mZ2V0cygpIHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4gMSBtcwogaGVs
bG82Ci1mZ2V0cygpIHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4gMSBtcwog
aGVsbG83Ci1mZ2V0cygpIHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4gMSBt
cwogaGVsbG84Ci1mZ2V0cygpIHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRoYW4g
MSBtcwogaGVsbG85Ci1mZ2V0cygpIHRvb2sgMCVzbXMKK2ZnZXRzKCkgdG9vayBsZXNzIHRo
YW4gMSBtcwogPT09RE9ORT09PQo=
--------------040605000506010601090503--