Fixed #74699 - Broken ArrayIterator unserializing#2559
Fixed #74699 - Broken ArrayIterator unserializing#2559andrewnester wants to merge 3 commits intophp:PHP-7.0from
Conversation
andrewnester
force-pushed
the
74669-iterator-unserialize
branch
3 times, most recently
from
777d06d to
0bad408
Compare
|
I think the root of the problem is incorrect handling of USE_OTHER during unserialization. getIterator() returns an AI with USE_OTHER, but we don't restore it as such during unserialization. |
andrewnester
force-pushed
the
74669-iterator-unserialize
branch
from
0bad408 to
b8cb98d
Compare
|
@nikic thanks a lot! it's absolutely true. just reworked my PR to handle |
|
@nikic any feedback on this? |
|
@andrewnester Not checked it closely yet, but one problem I could foresee is that this might be maliciously used to create an ArrayObject with USE_OTHER, where the inner "array" is not also an ArrayObject/ArrayIterator. We should check if something bad can happen in that case and/or prevent it. |
|
@nikic I improved test to include check what happens when we create an |
|
@andrewnester What I had in mind here is a test like this: $payload = 'x:i:33554432;O:8:"stdClass":0:{};m:a:0:{}';
$str = 'C:11:"ArrayObject":' . strlen($payload) . ':{' . $payload . '}';
$ao = unserialize($str);
var_dump($ao["foo"]);And indeed, this causes a segmentation fault. I think the proper way to fix this is to not encode the USE_OTHER flag (as it was previously), but instead use spl_array_set_array, which will correctly detect whether the flag needs to be set or not. |
|
Apart from fixing that segfault, doing it that way should also support ArrayObject serializations prior to the fix transparently. |
andrewnester
force-pushed
the
74669-iterator-unserialize
branch
from
802f802 to
823d637
Compare
|
@nikic thanks! I reworked my PR to use |
ext/spl/spl_array.c
Outdated
| } else { | ||
| ZVAL_COPY(&intern->array, &array); | ||
| } | ||
| var_push_dtor(&var_hash, &intern->array); |
There was a problem hiding this comment.
This should be pushing &array now. Alternatively and I think preferably you can declare zval *array and write array = var_tmp_var(&var_hash) and drop this push_dtor (the same as flags and members is handled).
There was a problem hiding this comment.
Doing it that way will also avoid the leaks you're currently seeing.
| spl_array_set_array(object, intern, &array, 0L, 1); | ||
| } else { | ||
| ZVAL_COPY(&intern->array, &array); | ||
| } |
There was a problem hiding this comment.
Two things I'm unsure about here: 1. Whether this still handles the IS_SELF case correctly (I think that might require an extra check) and 2. whether this shouldn't be using spl_array_set_array for normal objects as well. In particular that would include the check for std object properties, something that we currently don't seem to check.
There was a problem hiding this comment.
@nikic thanks
- I've added additional check and fix for this.
- yeah, it's good idea to use
spl_array_set_arrayfor normal objects as well. Just implemented it
|
Merged as afc2282 into 7.0+ with some additional tweaks to handle the IS_SELF case. |
|
I'm still having problems with this in 7.1.10. |
5 participants
Fix for https://bugs.php.net/bug.php?id=74669
thanks @nikic for note: the reason why it stopped work is wrong handling of USE_OTHER flag.