Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #74699 - Broken ArrayIterator unserializing #2559

Closed
wants to merge 3 commits into
base: PHP-7.0
from

Conversation

5 participants
@andrewnester
Copy link
Contributor

andrewnester commented Jun 6, 2017

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.

@andrewnester andrewnester force-pushed the andrewnester:74669-iterator-unserialize branch 3 times, most recently from 777d06d to 0bad408 Jun 6, 2017

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 7, 2017

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.

@nikic nikic added the Bugfix label Jun 7, 2017

@andrewnester andrewnester force-pushed the andrewnester:74669-iterator-unserialize branch from 0bad408 to b8cb98d Jun 8, 2017

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

andrewnester commented Jun 8, 2017

@nikic thanks a lot! it's absolutely true. just reworked my PR to handle USE_OTHER flags in serialisation as well.

@laruence laruence removed the Bugfix label Jun 8, 2017

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

andrewnester commented Jun 12, 2017

@nikic any feedback on this?

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 12, 2017

@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.

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

andrewnester commented Jun 13, 2017

@nikic I improved test to include check what happens when we create an ArrayObject with USE_OTHER, where the inner "array" is simple array

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 23, 2017

@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.

@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jun 23, 2017

Apart from fixing that segfault, doing it that way should also support ArrayObject serializations prior to the fix transparently.

@andrewnester andrewnester force-pushed the andrewnester:74669-iterator-unserialize branch from 802f802 to 823d637 Jun 30, 2017

@andrewnester

This comment has been minimized.

Copy link
Contributor Author

andrewnester commented Jun 30, 2017

@nikic thanks! I reworked my PR to use spl_array_set_array() call.

spl_array_set_array(object, intern, &array, 0L, 1);
} else {
ZVAL_COPY(&intern->array, &array);
}
var_push_dtor(&var_hash, &intern->array);

This comment has been minimized.

@nikic

nikic Jun 30, 2017

Member

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).

This comment has been minimized.

@nikic

nikic Jun 30, 2017

Member

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);
}

This comment has been minimized.

@nikic

nikic Jun 30, 2017

Member

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.

This comment has been minimized.

@andrewnester

andrewnester Jun 30, 2017

Author Contributor

@nikic thanks

  1. I've added additional check and fix for this.
  2. yeah, it's good idea to use spl_array_set_array for normal objects as well. Just implemented it
@nikic

This comment has been minimized.

Copy link
Member

nikic commented Jul 18, 2017

Merged as afc2282 into 7.0+ with some additional tweaks to handle the IS_SELF case.

@nikic nikic closed this Jul 18, 2017

@radiat-r

This comment has been minimized.

Copy link

radiat-r commented Sep 29, 2017

I'm still having problems with this in 7.1.10.
Unserializing in another run fails with "Notice: unserialize(): Error at offset 45 of 193 bytes ..."
See: https://bugs.php.net/bug.php?id=75289

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.