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
