# 5dee3c11 break - Thread: https://externals.io/message/73950 - Original archive: https://news-web.php.net/php.internals/73950 - Started by Dmitry Stogov on 2014-05-06T08:11:20+00:00 - Messages: 17 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #73950 — Dmitry Stogov — 2014-05-06T08:11:20+00:00 URL: https://externals.io/message/73950#73950 Hi Tjerk, your commit broke the code that worked fine before (still works in 5.5 but broken in 5.6 and above). It leads into infinity recursion until stack overflow. It must be fixed or reverted. Thanks. Dmitry. Hi Tjerk, > > your commit broke the code that worked fine before (still works in 5.5 but > broken in 5.6 and above). > It leads into infinity recursion until stack overflow. > > It must be fixed or reverted. > This has been mentioned by Jakub before and a fix to ZF2 has already been merged: https://github.com/zendframework/zf2/pull/6096 The previous code and my patch basically cannot coexist; it used to work in 5.6 before, but only by the "virtue" of an unfortunate implementation. I believe this is not the only 5.6 issue that ZF2 is dealing with, but if you feel that this breaks too many things for a 5.x release I suppose we can revert it in PHP-5.6 and keep it for PHP-6? Let me know. > Thanks. Dmitry. > > class Parameters extends ArrayObject { > public function __construct(array $values = null) { > if (null === $values) { > $values = array(); > } > parent::__construct($values, ArrayObject::ARRAY_AS_PROPS); > } > public function offsetGet($name) { > if (isset($this[$name])) { > return parent::offsetGet($name); > } > return null; > } > } > $x = new Parameters(); > var_dump($x['foo']); > $x['foo'] = 'bar'; > var_dump($x['foo']); > ?> > -- -- Tjerk --- ## #73967 — Dmitry Stogov — 2014-05-06T11:25:22+00:00 URL: https://externals.io/message/73950#73967 · In reply to: #73962 I didn't review you patch careful, but why do you need to call offsetGet() when offsetExists() must be enough? is it for empty()? Then you probably should implement it a way similar to zend_std_has_dimension() in Zend/zend_object_handlers.c. call offsetExists() and then offsetGet() if necessary. Thanks. Dmitry. On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote: > Hi Dmitry, > > > > > […] > > This has been mentioned by Jakub before and a fix to ZF2 has already been > merged: > > https://github.com/zendframework/zf2/pull/6096 > > The previous code and my patch basically cannot coexist; it used to work > in 5.6 before, but only by the "virtue" of an unfortunate implementation. > > I believe this is not the only 5.6 issue that ZF2 is dealing with, but if > you feel that this breaks too many things for a 5.x release I suppose we > can revert it in PHP-5.6 and keep it for PHP-6? > > Let me know. > > > > […] > > > > -- > -- > Tjerk --- ## #73968 — Tjerk Anne Meesters — 2014-05-06T11:29:57+00:00 URL: https://externals.io/message/73950#73968 · In reply to: #73967 Hi, > I didn't review you patch careful, but why do you need to call offsetGet() > when offsetExists() must be enough? > is it for empty()? > Mostly for empty(), yes. It fixes the expected empty() behaviour for `$a['foo'] == 0` when using ArrayObject. This is further explained in this bug report: https://bugs.php.net/bug.php?id=66834 > Then you probably should implement it a way similar to > zend_std_has_dimension() in Zend/zend_object_handlers.c. > call offsetExists() and then offsetGet() if necessary. > That's already the case :) http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719 > > Thanks. Dmitry. > > > On Tue, May 6, 2014 at 2:31 PM, Tjerk Anne Meesters wrote: > > > […] > > -- -- Tjerk --- ## #73969 — Dmitry Stogov — 2014-05-06T11:50:13+00:00 URL: https://externals.io/message/73950#73969 · In reply to: #73968 Ah, I didn't get what did you mean by (check_empty == 2), but they probably should be changed into (check_empty != 1) It fixes the problem. Could you please verify and commit into all relevant branches. Thanks. Dmitry. --- a/ext/spl/spl_array.c +++ b/ext/spl/spl_array.c @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int check_inherited, zval *object, zval *o if (rv && zend_is_true(rv TSRMLS_CC)) { zval_ptr_dtor(&rv); - if (check_empty == 2) { + if (check_empty != 1) { return 1; } else if (intern->fptr_offset_get) { value = spl_array_read_dimension_ex(1, object, offset, BP_VAR_R TSRMLS_CC); @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int check_inherited, zval *object, zval *o switch(Z_TYPE_P(offset)) { case IS_STRING: if (zend_symtable_find(ht, Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) { - if (check_empty == 2) { + if (check_empty != 1) { return 1; } } else { @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int check_inherited, zval *object, zval *o index = Z_LVAL_P(offset); } if (zend_hash_index_find(ht, index, (void **)&tmp) != FAILURE) { - if (check_empty == 2) { + if (check_empty != 1) { return 1; } } else { On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote: > Hi, > > > > > > […] > > Mostly for empty(), yes. It fixes the expected empty() behaviour for > `$a['foo'] == 0` when using ArrayObject. This is further explained in this > bug report: https://bugs.php.net/bug.php?id=66834 > > > > […] > > That's already the case :) > > http://lxr.php.net/xref/PHP_TRUNK/Zend/zend_object_handlers.c#719 > > > > […] > > > -- > -- > Tjerk --- ## #73973 — Tjerk Anne Meesters — 2014-05-06T12:36:33+00:00 URL: https://externals.io/message/73950#73973 · In reply to: #73969 > Ah, I didn't get what did you mean by (check_empty == 2), but they > probably should be changed into (check_empty != 1) > When I said mostly, perhaps I should have mentioned that the isset() behaviour was deliberately updated as well; in other words, the following works as expected: $x['foo'] = null; isset($x['foo']); // false After taking a closer look at zend_std_has_dimension() it seems that both isset() and empty() behave as if empty() was always used; of course, if this behaviour was translated into spl_array.c, the infinite recursion would still be an issue. So, at this point I'm unsure what the proper action point should be. > > > It fixes the problem. > Could you please verify and commit into all relevant branches. > > Thanks. Dmitry. > > --- a/ext/spl/spl_array.c > +++ b/ext/spl/spl_array.c > @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int > check_inherited, zval *object, zval *o > > if (rv && zend_is_true(rv TSRMLS_CC)) { > zval_ptr_dtor(&rv); > - if (check_empty == 2) { > + if (check_empty != 1) { > return 1; > } else if (intern->fptr_offset_get) { > value = spl_array_read_dimension_ex(1, > object, offset, BP_VAR_R TSRMLS_CC); > @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int > check_inherited, zval *object, zval *o > switch(Z_TYPE_P(offset)) { > case IS_STRING: > if (zend_symtable_find(ht, > Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) { > - if (check_empty == 2) { > + if (check_empty != 1) { > return 1; > } > } else { > @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int > check_inherited, zval *object, zval *o > index = Z_LVAL_P(offset); > } > if (zend_hash_index_find(ht, index, (void > **)&tmp) != FAILURE) { > - if (check_empty == 2) { > + if (check_empty != 1) { > return 1; > } > } else { > > > > On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote: > > > […] > > -- -- Tjerk --- ## #73974 — Dmitry Stogov — 2014-05-06T12:43:00+00:00 URL: https://externals.io/message/73950#73974 · In reply to: #73973 zend_std_has_dimension() doesn't know what (check_empty == 2) means. check_empty == 0 - ISSET => we don't need to call offsetGet() and relay on offsetExists() return value. check_empty == 1 - ISEMPTY => we should call offsetGet() after offsetExists(). `NULL` values should be handled by offsetExists(). Thanks. Dmitry. On Tue, May 6, 2014 at 4:36 PM, Tjerk Anne Meesters wrote: > > > > > > […] > > When I said mostly, perhaps I should have mentioned that the isset() > behaviour was deliberately updated as well; in other words, the following > works as expected: > > $x['foo'] = null; > isset($x['foo']); // false > > After taking a closer look at zend_std_has_dimension() it seems that both > isset() and empty() behave as if empty() was always used; of course, if > this behaviour was translated into spl_array.c, the infinite recursion > would still be an issue. So, at this point I'm unsure what the proper > action point should be. > > > > […] > > > -- > -- > Tjerk --- ## #73976 — Levi Morrison — 2014-05-06T13:11:06+00:00 URL: https://externals.io/message/73950#73976 · In reply to: #73974 > zend_std_has_dimension() doesn't know what (check_empty == 2) means. > > check_empty == 0 - ISSET => we don't need to call offsetGet() and relay on > offsetExists() return value. > check_empty == 1 - ISEMPTY => we should call offsetGet() after > offsetExists(). > > `NULL` values should be handled by offsetExists(). > I am a bit curious, `isset` checks that the variable exists and is not null; `empty` checks that the variable exists and is not empty. Why does one call `offsetGet` and not the other? Both look at the value. Sorry if I missed that bit of conversation. --- ## #73977 — Dmitry Stogov — 2014-05-06T14:46:42+00:00 URL: https://externals.io/message/73950#73977 · In reply to: #73976 > > > > […] > > I am a bit curious, `isset` checks that the variable exists and is not > null; `empty` checks that the variable exists and is not empty. Why does > one call `offsetGet` and not the other? Both look at the value. > > Sorry if I missed that bit of conversation. > yeah, no problem :) I also, don't talk that my opinion is completely right. I think that we make us and users more and more troubles with this messy behavior. Actually, you introduced new behavior (NULL check) and broke at least ZF2 and probably many based on it applications. It leads to crash and it'll make bad php experience. Then someone will report it as a security problem and we all will be blamed :( We shouldn't make existing applications crash! Thanks. Dmitry. --- ## #73978 — Dmitry Stogov — 2014-05-06T14:48:41+00:00 URL: https://externals.io/message/73950#73978 · In reply to: #73977 If you think the current behavior is right, we need protection from recursion (similar to __get()). Thanks. Dmitry. > > > > > > > […] > > yeah, no problem :) > I also, don't talk that my opinion is completely right. > > I think that we make us and users more and more troubles with this messy > behavior. > > Actually, you introduced new behavior (NULL check) and broke at least ZF2 > and probably many based on it applications. > It leads to crash and it'll make bad php experience. > Then someone will report it as a security problem and we all will be > blamed :( > We shouldn't make existing applications crash! > > Thanks. Dmitry. --- ## #73979 — Levi Morrison — 2014-05-06T14:50:51+00:00 URL: https://externals.io/message/73950#73979 · In reply to: #73977 > > > > > […] > > yeah, no problem :) > I also, don't talk that my opinion is completely right. > > I think that we make us and users more and more troubles with this messy > behavior. > > Actually, you introduced new behavior (NULL check) and broke at least ZF2 > and probably many based on it applications. > That isn't any new behavior, at least it shouldn't be. `isset` does not ONLY check existence, it checks that the value is not null; see http://php.net/isset It leads to crash and it'll make bad php experience. > Then someone will report it as a security problem and we all will be > blamed :( > We shouldn't make existing applications crash! > Agreed, but at least from what I understand the current behavior doesn't do what it is supposed to anyway. Sometimes bugs become features but I really don't see this as one of those. We should fix this. That's my $0.02. --- ## #73980 — Dmitry Stogov — 2014-05-06T14:57:38+00:00 URL: https://externals.io/message/73950#73980 · In reply to: #73979 On Tue, May 6, 2014 at 6:50 PM, Levi Morrison wrote: > > > > […] > > That isn't any new behavior, at least it shouldn't be. `isset` does not > ONLY check existence, it checks that the value is not null; see > http://php.net/isset > I know :), but we never did it for magic handlers. In case offsetExists() ot __isset() returned "flase" we didn't try to check the value. > > It leads to crash and it'll make bad php experience. > > […] > > Agreed, but at least from what I understand the current behavior doesn't > do what it is supposed to anyway. Sometimes bugs become features but I > really don't see this as one of those. We should fix this. That's my $0.02. > I would be very glad to have it fixed. Thanks. Dmitry. --- ## #73986 — Tjerk Anne Meesters — 2014-05-06T23:33:26+00:00 URL: https://externals.io/message/73950#73986 · In reply to: #73974 Hi, > zend_std_has_dimension() doesn't know what (check_empty == 2) means. > > check_empty == 0 - ISSET => we don't need to call offsetGet() and relay on > offsetExists() return value. > check_empty == 1 - ISEMPTY => we should call offsetGet() after > offsetExists(). > > `NULL` values should be handled by offsetExists(). > Okay, I suppose it's fair to make its behaviour the same as for zend_std :) I'll make the necessary changes by tonight for 5.6 and master. > > Thanks. Dmitry. > > > On Tue, May 6, 2014 at 4:36 PM, Tjerk Anne Meesters wrote: > > > […] > > -- -- Tjerk --- ## #73992 — Tjerk Anne Meesters — 2014-05-07T04:31:48+00:00 URL: https://externals.io/message/73950#73992 · In reply to: #73969 Hi, > Ah, I didn't get what did you mean by (check_empty == 2), but they > probably should be changed into (check_empty != 1) > > It fixes the problem. > Could you please verify and commit into all relevant branches. > > Thanks. Dmitry. > > --- a/ext/spl/spl_array.c > +++ b/ext/spl/spl_array.c > @@ -603,7 +603,7 @@ static int spl_array_has_dimension_ex(int > check_inherited, zval *object, zval *o > > if (rv && zend_is_true(rv TSRMLS_CC)) { > zval_ptr_dtor(&rv); > - if (check_empty == 2) { > + if (check_empty != 1) { > return 1; > } else if (intern->fptr_offset_get) { > value = spl_array_read_dimension_ex(1, > object, offset, BP_VAR_R TSRMLS_CC); > @@ -622,7 +622,7 @@ static int spl_array_has_dimension_ex(int > check_inherited, zval *object, zval *o > switch(Z_TYPE_P(offset)) { > case IS_STRING: > if (zend_symtable_find(ht, > Z_STRVAL_P(offset), Z_STRLEN_P(offset)+1, (void **) &tmp) != FAILURE) { > - if (check_empty == 2) { > + if (check_empty != 1) { > return 1; > } > } else { > @@ -639,7 +639,7 @@ static int spl_array_has_dimension_ex(int > check_inherited, zval *object, zval *o > index = Z_LVAL_P(offset); > } > if (zend_hash_index_find(ht, index, (void > **)&tmp) != FAILURE) { > - if (check_empty == 2) { > + if (check_empty != 1) { > return 1; > } > } else { > This patch broke an earlier test case, so I've refactored it into the following compromise (until PHP.next): https://gist.github.com/datibbaw/55f0dc8e95f45b91e514 Does that look okay to you? > > > > On Tue, May 6, 2014 at 3:29 PM, Tjerk Anne Meesters wrote: > > > […] > > -- -- Tjerk --- ## #73993 — Levi Morrison — 2014-05-07T05:30:02+00:00 URL: https://externals.io/message/73950#73993 · In reply to: #73986 I think perhaps I haven't been clear in the past posts; I apologize. The basic premise is that `isset` and `empty` should work the same on arrays as they do on any array object. The problem comes from the fact that `offsetExists` does not say that you should return false for an existing offset with a null value. At the end of this email is is a gauntlet of tests that *should work*, the only exception is ExtendedArrayAccess which returns true for an existing key with a null value (uses array_key_exists). I'd argue that ExtendedArrayAccess uses the correct semantics and that all others should be changed to not do the null check (only does it exist) but that would be a big BC break. Hopefully this all makes sense. data = $array; } function offsetExists($offset) { return array_key_exists($offset, $this->data); } function offsetGet($offset) { return $this->data[$offset]; } function offsetSet($offset, $value) { $this->data[$offset] = $value; } function offsetUnset($offset) { unset($this->data[$offset]); } } function createInputs() { return array( array(), new ArrayObject(), new ExtendedArrayObject(), new ExtendedArrayAccess(), ); } class Tests { function isset_existing_key_with_not_empty_value($array) { $array['foo'] = 1; $this->expect(isset($array['foo']) === true, $array); } function empty_existing_key_with_not_empty_value($array) { $array['foo'] = 1; $this->expect(empty($array['foo']) === false, $array); } function isset_existing_key_with_empty_value($array) { $array['foo'] = 0; $this->expect(isset($array['foo']) === true, $array); } function empty_existing_key_with_empty_value($array) { $array['foo'] = 0; $this->expect(empty($array['foo']) === true, $array); } function isset_non_existent_key($array) { $this->expect(isset($array['foo']) === false, $array); } function empty_non_existent_key($array) { $this->expect(empty($array['foo']) === true, $array); } function isset_existing_key_with_null_value($array) { $array['foo'] = null; $this->expect(isset($array['foo']) === false, $array); } function empty_existing_key_with_null_value($array) { $array['foo'] = null; $this->expect(empty($array['foo']) === true, $array); } private function getType($v) { return is_array($v) ? 'array' : get_class($v); } private function expect($condition, $array) { if (!$condition) { echo "\t", $this->getType($array), ' failed ', PHP_EOL; } } } $tests = new Tests; foreach (get_class_methods($tests) as $test) { echo "$test:", PHP_EOL; foreach (createInputs() as $structure) { $tests->$test($structure); } echo PHP_EOL; } --- ## #73994 — Levi Morrison — 2014-05-07T05:31:12+00:00 URL: https://externals.io/message/73950#73994 · In reply to: #73993 On Tue, May 6, 2014 at 11:30 PM, Levi Morrison wrote: > I think perhaps I haven't been clear in the past posts; I apologize. The > basic premise is that `isset` and `empty` should work the same on arrays as > they do on any array object. The problem comes from the fact that > `offsetExists` does not say that you should return false for an existing > offset with a null value. > > At the end of this email is is a gauntlet of tests that *should work*, the > only exception is ExtendedArrayAccess which returns true for an existing > key with a null value (uses array_key_exists). I'd argue that > ExtendedArrayAccess uses the correct semantics and that all others should > be changed to not do the null check (only does it exist) but that would be > a big BC break. Hopefully this all makes sense. > > > class ExtendedArrayObject extends ArrayObject { > function offsetExists($offset) { > //echo "\t", __METHOD__, PHP_EOL; > return parent::offsetExists($offset); > } > function offsetGet($offset) { > //echo "\t", __METHOD__, PHP_EOL; > return parent::offsetGet($offset); > } > } > > class ExtendedArrayAccess implements ArrayAccess { > private $data = array(); > function __construct(array $array = array()) { > $this->data = $array; > } > function offsetExists($offset) { > return array_key_exists($offset, $this->data); > } > function offsetGet($offset) { > return $this->data[$offset]; > } > function offsetSet($offset, $value) { > $this->data[$offset] = $value; > } > function offsetUnset($offset) { > unset($this->data[$offset]); > } > } > > function createInputs() { > return array( > array(), > new ArrayObject(), > new ExtendedArrayObject(), > new ExtendedArrayAccess(), > ); > } > > class Tests { > function isset_existing_key_with_not_empty_value($array) { > $array['foo'] = 1; > $this->expect(isset($array['foo']) === true, $array); > } > > function empty_existing_key_with_not_empty_value($array) { > $array['foo'] = 1; > $this->expect(empty($array['foo']) === false, $array); > } > > function isset_existing_key_with_empty_value($array) { > $array['foo'] = 0; > $this->expect(isset($array['foo']) === true, $array); > } > > function empty_existing_key_with_empty_value($array) { > $array['foo'] = 0; > $this->expect(empty($array['foo']) === true, $array); > } > > function isset_non_existent_key($array) { > $this->expect(isset($array['foo']) === false, $array); > } > > function empty_non_existent_key($array) { > $this->expect(empty($array['foo']) === true, $array); > } > > function isset_existing_key_with_null_value($array) { > $array['foo'] = null; > $this->expect(isset($array['foo']) === false, $array); > } > > function empty_existing_key_with_null_value($array) { > $array['foo'] = null; > $this->expect(empty($array['foo']) === true, $array); > } > > private function getType($v) { > return is_array($v) ? 'array' : get_class($v); > } > > private function expect($condition, $array) { > if (!$condition) { > echo "\t", $this->getType($array), ' failed ', PHP_EOL; > } > } > } > > $tests = new Tests; > foreach (get_class_methods($tests) as $test) { > echo "$test:", PHP_EOL; > foreach (createInputs() as $structure) { > $tests->$test($structure); > } > echo PHP_EOL; > } > > Here it is on 3v4l: http://3v4l.org/SoFnK --- ## #74265 — Dmitry Stogov — 2014-05-16T16:32:36+00:00 URL: https://externals.io/message/73950#74265 · In reply to: #73992 hi Tjerk, sorry, for long delay. The patch looks fine to me. I think you may commit it. Thanks for fixing this. Dmitry. On Wed, May 7, 2014 at 8:31 AM, Tjerk Anne Meesters wrote: > Hi, > > > > > > […] > > This patch broke an earlier test case, so I've refactored it into the > following compromise (until PHP.next): > > https://gist.github.com/datibbaw/55f0dc8e95f45b91e514 > > Does that look okay to you? > > > > […] > > > -- > -- > Tjerk