On Fri, Feb 8, 2013 at 4:11 AM, Laruence <larue...@php.net> wrote:

> On Fri, Feb 8, 2013 at 3:36 AM, Nikita Popov <nikita....@gmail.com> wrote:
> > On Thu, Feb 7, 2013 at 4:44 PM, Xinchen Hui <larue...@php.net> wrote:
> >>
> >> Commit:    290509755ac4a3279b2b31b899aa9f2dd780f5f4
> >> Author:    Xinchen Hui <larue...@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;).


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.

Thanks,
Nikita

Reply via email to