Re: [PR 614] ArrayObject and isset() / empty() behaviour

Hi, On Sun, Apr 6, 2014 at 8:55 PM, Jakub Zelenka <bukka@php.net> wrote: > Hi Tjerk, > > I was playing with ZF2 and getting segfaults for Mvc tests (for example > phpunit ZendTest/Mvc/ApplicationTest.php). It's only on 5.6 and master > branch. Think that it might be because of this patch: > > 0 zend_call_method zend_interfaces.c 35 0xae17fd > 1 spl_array_read_dimension_ex spl_array.c 389 0x87179b > 2 spl_array_has_dimension_ex spl_array.c 655 0x87273d > 3 spl_array_has_dimension spl_array.c 675 0x8727d1 > 4 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV > zend_vm_execute.h 40983 0xb817a8 > 5 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER zend_vm_execute.h > 41039 0xb819c1 > 6 execute_ex zend_vm_execute.h 363 0xb097cb > 7 zend_execute zend_vm_execute.h 388 0xb098b1 > 8 zend_call_function zend_execute_API.c 952 0xaa6faa > 9 zend_call_method zend_interfaces.c 97 0xae1bea > 10 spl_array_read_dimension_ex spl_array.c 389 0x87179b > 11 spl_array_has_dimension_ex spl_array.c 655 0x87273d > 12 spl_array_has_dimension spl_array.c 675 0x8727d1 > 13 zend_isset_isempty_dim_prop_obj_handler_SPEC_CV_CV > zend_vm_execute.h 40983 0xb817a8 > 14 ZEND_ISSET_ISEMPTY_DIM_OBJ_SPEC_CV_CV_HANDLER > zend_vm_execute.h 41039 0xb819c1 > 15 execute_ex zend_vm_execute.h 363 0xb097cb > 16 zend_execute zend_vm_execute.h 388 0xb098b1 > 17 zend_call_function zend_execute_API.c 952 0xaa6faa > 18 zend_call_method zend_interfaces.c 97 0xae1bea > 19 spl_array_read_dimension_ex spl_array.c 389 0x87179b > 20 spl_array_has_dimension_ex spl_array.c 655 0x87273d > ... <More> > Yeah, it causes infinite recursion because of this: public function offsetGet($name) { if (isset($this[$name])) { return parent::offsetGet($name); } return null; } Doing this "detaches" the call chain and enters it again later without any context to track a potential recursive call. I'm not sure whether it can be fixed other than not doing this. I've authored a (bc compatible) ZF2 fix here: https://github.com/zendframework/zf2/pull/6096 > Let me know if you can't recreate it > > Thanks > > Jakub > > > On Mon, Mar 10, 2014 at 4:20 PM, Tjerk Meesters <tjerk.meesters@gmail.com>wrote: > >> On Mon, Mar 10, 2014 at 9:58 PM, Tjerk Meesters <tjerk.meesters@gmail.com >> >wrote: >> >> > Hi Etienne, >> > >> > >> > On Mon, Mar 10, 2014 at 9:26 PM, Etienne Kneuss <colder@php.net> wrote: >> > >> >> >> >> >> >> >> >> On Mon, Mar 10, 2014 at 1:43 PM, Ferenc Kovacs <tyra3l@gmail.com> >> wrote: >> >> >> >>> >> >>> >> >>> >> >>> On Sat, Mar 8, 2014 at 10:56 AM, Tjerk Meesters < >> >>> tjerk.meesters@gmail.com> wrote: >> >>> >> >>>> On Sat, Mar 8, 2014 at 8:27 AM, Tjerk Meesters < >> >>>> tjerk.meesters@gmail.com>wrote: >> >>>> >> >>>> > Hi Etienne, >> >>>> > >> >>>> > Thanks for your input, much appreciated. >> >>>> > >> >>>> > I have moved the decision up the chain as you suggested; the >> results >> >>>> can >> >>>> > be found here: >> >>>> > >> >>>> > >> >> https://github.com/datibbaw/php-src/compare/php:PHP-5.4...bug66834a >> >>>> > >> >>>> >> >>>> > >> >>>> > It's suffering from a few memory leaks which can be fixed, >> >>>> > >> >>>> >> >>>> They are fixed now and a new PR has been opened: >> >>>> >> >>>> https://github.com/php/php-src/pull/616 >> >>>> >> >>>> >> >>> Hi Etienne, >> >>> >> >>> Are you fine with the changes in the PR? >> >>> >> >>> >> >> Two things: >> >> >> >> 1) Where is this planned to be merged? Is it really 5.4 ? As is the PR >> is >> >> no longer a simple bug fix and while I agree it should be changed that >> way, >> >> it might introduce hard-to-track BC breaks. >> >> >> > >> > After a short discussion on IRC I'm moving the branch to target 5.6 >> > instead. >> > >> > >> >> 2) Seems to me that with this PR, part of the hack present in >> ArrayObject >> >> in the has_dimension handler is no longer necessary, as it might be >> doing >> >> to value-check twice now. >> >> >> > >> > I'll start looking into this now. Thanks for the heads up :) >> > >> >> I've updated and cleaned up ext/spl; the most notable change is code >> removal according to a changed definition of zend_object_has_dimension_t: >> >> typedef int (*zend_object_has_dimension_t)(zval *object, zval *member >> TSRMLS_DC); >> >> PR: https://github.com/php/php-src/pull/621 >> >> Let me know if you see any other issues; I'm fine with reverting the >> typedef change if that could be an issue. >> >> >> > >> > >> >> >> >> >> >> Best, >> >> >> >> -- >> >>> Ferenc Kovács >> >>> @Tyr43l - http://tyrael.hu >> >>> >> >> >> >> >> >> >> >> -- >> >> Etienne Kneuss >> >> http://www.colder.ch >> >> >> > >> > >> > >> > -- >> > -- >> > Tjerk >> > >> >> >> >> -- >> -- >> Tjerk >> > > -- -- Tjerk