Re: com php-src: Fixed bug #64135 (Exceptions from set_error_handler are not always propagated): NEWS Zend/tests/bug61767.phpt Zend/zend_vm_def.h
Zend/zend_vm_execute.h
| From: | Laruence | Date: | Sat, 16 Feb 2013 14:34:20 +0000 |
| Subject: | Re: com php-src: Fixed bug #64135 (Exceptions from set_error_handler are not always propagated): NEWS Zend/tests/bug61767.phpt Zend/zend_vm_def.h Zend/zend_vm_execute.h |
||
| References: | 1 2 3 4 5 | Groups: | php.cvs |
| Request: | Send a blank email to php-cvs+get-71242@lists.php.net to get a copy of this message | ||
Hey:
I fixed these issues, and extra one clone issue, committed here:
https://github.com/php/php-src/commit/75742d57eb34c2c1c4d23d55f9b9ee44939b32c2
and if you can find more, please tell me, it will be appreciated.
thanks
On Sat, Feb 16, 2013 at 10:25 PM, Laruence <laruence@php.net> wrote:
> On Fri, Feb 8, 2013 at 8:36 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:
>> On Fri, Feb 8, 2013 at 4:11 AM, Laruence <laruence@php.net> wrote:
>>>
>>> On Fri, Feb 8, 2013 at 3:36 AM, Nikita Popov <nikita.ppv@gmail.com> wrote:
>>> > On Thu, Feb 7, 2013 at 4:44 PM, Xinchen Hui <laruence@php.net> wrote:
>>> >>
>>> >> Commit: 290509755ac4a3279b2b31b899aa9f2dd780f5f4
>>> >> Author: Xinchen Hui <laruence@php.net> Thu, 7 Feb 2013
>>> >> 23:44:46
>>> >> +0800
>>> >> Parents: 0547a36e95ec36025a30e93e971d26b6b1ecf0e9
>>> >> Branches: PHP-5.5
>>> >>
>>> >> Link:
>>> >>
>>> >>
>>> >> http://git.php.net/?p=php-src.git;a=commitdiff;h=290509755ac4a3279b2b31b899aa9f2dd780f5f4
>>> >>
>>> >> Log:
>>> >> Fixed bug #64135 (Exceptions from set_error_handler are not always
>>> >> propagated)
>>> >
>>> >
>>> > Is there any particular reason why the exception check was added only
>>> > the
>>> > that error branch? Can't it be done right after the GET_OP1_OBJ_ZVAL_PTR
>>> > in
>>> >
>>> > http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#2442? This way
>>> > it
>>> > would
>>> > cover the other error conditions as well.
>>> Hey:
>>>
>>> could you please example one, which will trigger a error(warning,
>>> notice) out of that branch?
>>
>>
>> Looking at the code a bit more, you are right, those branches can't be
>> reached. The only warning that the GET_ZVAL_PTR can trigger is an undefined
>> variable in which case the return value will be an uninited zval, which will
>> always use the last error branch.
>>
>> What I did notice though is that this commit just fixes one out of very many
>> cases where something like this or something similar could occur. E.g. just
>> a few lines above it the same problem exists:
>> http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#2432
>>
>> So if you change the test code from $undefinedObject->$definedMethod() to
>> $definedObject->$undefinedMethod() or $undefinedObject->$undefinedMethod()
>> it will still fail.
>>
>> A few lines higher FETCH_CLASS has the same problem
>> (http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#2398). Going down a
>> bit INIT_STATIC_METHOD_CALL has this too:
>> http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#2548.
>> INIT_FCALL_BY_NAME also has this:
>> http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#2753
>> So you can trigger
>> it with just $undefined() too. Again the same thing in THROW:
>> http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_vm_def.h#2963
>> (throw
>> $undefined;).
>
> thanks for the notice. you are right , there are some similar issues
> need to be fixed.
>
>>
>>
>> I could continue this list, but I think you get the point ^^ This issue
>> exists pretty much everywhere where a ZVAL_PTR fetch is followed by some
>> error condition that can happen when the zval is NULL. And we have a *lot*
>> of those. So I'm not sure that it really makes sense to fix one of those
>> cases and leave 50 others intact. Either this should be fixed generically
>> (e.g. on the side of GET_ZVAL_PTR, though that might incur a performance
>> penalty) or should be just left alone.
>
> or , we fix one when we found one.
>
> thanks
>>
>> Thanks,
>> Nikita
>
>
>
> --
> Laruence Xinchen Hui
> http://www.laruence.com/
--
Laruence Xinchen Hui
http://www.laruence.com/