Hi!
Working on unserialize edge case patches backporting, I've discovered
that object_properties_load() function crashes if the properties array
contains non-string keys (which can happen on unserialize). Now, I can
fix the crash, but I can fix it in two ways:
- Ignore such keys (i.e. such properties will be banned, BC break
against 5.x) - Treat such keys as in 5.x i.e. insert them in the hash.
So, I wanted to know if dropping non-string keys there was intentional
or not?
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
I don't see the crash. Integer keys are supported in exactly the same way
as it was in PHP-5.
$ sapi/cli/php -r '$a = [1=>5]; $o = (object)$a; var_dump($o); $s =
serialize($o); var_dump($s);var_dump(unserialize($s));'
object(stdClass)#1 (1) {
[1]=>
int(5)
}
string(27) "O:8:"stdClass":1:{i:1;i:5;}"
object(stdClass)#2 (1) {
["1"]=>
int(5)
}
if you talk about manually crafted serialize string with for example
"float" keys, I think, we should just report a error.
Thanks. Dmitry.
On Wed, Sep 2, 2015 at 9:35 AM, Stanislav Malyshev smalyshev@gmail.com
wrote:
Hi!
Working on unserialize edge case patches backporting, I've discovered
that object_properties_load() function crashes if the properties array
contains non-string keys (which can happen on unserialize). Now, I can
fix the crash, but I can fix it in two ways:
- Ignore such keys (i.e. such properties will be banned, BC break
against 5.x)- Treat such keys as in 5.x i.e. insert them in the hash.
So, I wanted to know if dropping non-string keys there was intentional
or not?Stas Malyshev
smalyshev@gmail.com
Hi!
I don't see the crash. Integer keys are supported in exactly the same
way as it was in PHP-5.
I see the crash on this test:
https://github.com/smalyshev/php-src/blob/master/ext/spl/tests/bug70155.phpt
I'm not sure how integer keys are supported the same way if I see this
in the code:
https://github.com/php/php-src/blob/master/Zend/zend_API.c#L1248
ZEND_HASH_FOREACH_STR_KEY_VAL(properties, key, prop) {
property_info = zend_get_property_info(object->ce, key, 1);
The loop doesn't even consider the possibility of key being NULL (which
happens with integer key) and only considers string keys, and, if the
key is integer, zend_get_property_info gets null as key argument and
segfaults.
--
Stas Malyshev
smalyshev@gmail.com
Hey Stas,
Stanislav Malyshev wrote:
Working on unserialize edge case patches backporting, I've discovered
that object_properties_load() function crashes if the properties array
contains non-string keys (which can happen on unserialize). Now, I can
fix the crash, but I can fix it in two ways:
- Ignore such keys (i.e. such properties will be banned, BC break
against 5.x)- Treat such keys as in 5.x i.e. insert them in the hash.
So, I wanted to know if dropping non-string keys there was intentional
or not?
Sadly I can't tell you if it was intentional. If I were to hazard a
guess, though, it sounds like an oversight on the part of the function
author.
Both cases are rather undesirable. The optimum would be to throw an
error of some sort if there's a non-string key, but that might break things.
Perhaps take option (2), inserting the keys into the hash (it seems to
be the consistent thing to me), but show an E_NOTICE warning you this is
happening?
I hope that's helpful.
--
Andrea Faulds
http://ajf.me/