Skip to content

Comments

Issue 66085 - discussion#550

Closed
ahamid wants to merge 2 commits intophp:PHP-5.6from
ahamid:issue-66085
Closed

Issue 66085 - discussion#550
ahamid wants to merge 2 commits intophp:PHP-5.6from
ahamid:issue-66085

Conversation

@ahamid
Copy link
Contributor

@ahamid ahamid commented Dec 24, 2013

Submitting pull request for discussion only (i.e. please don't merge this ;), see: https://bugs.php.net/bug.php?id=66085

When invoking a custom serializer which itself uses serialize, serialized temporary objects are unreferenced and garbage collected. Their handles and zval memory locations can be reused which results in defeating the reference linking capability of serialize (the collected objects are viewed as having been visited and are candidates for reference linking).

I don't see any great solutions but here are a few:

  1. Just don't support re-entrant serialize with reference linking. Create a new serialization context for every serialize entrance.
  2. It turns out (from what I can tell) zvals do not have unique ids over time. Handles are just slots which are reused and memory locations are also reused exactly, so these conspire against being able to distinguish a previous dead object from a live object occupying the same memory location. A solution could be to introduce a monotonic counter which is set on every zval produced in order to produce completely unique ids for the lifetime of the php process. This presumably would entail modifying zval or other core structure so could affect (albiet innocuously) a large surface area. With truly unique ids, a live zval could never be confused with a previous live or dead zval.
  3. Forcibly prevent collection of zvals implicated in an object graph undergoing serialization. This can be done by incrementing a reference count, and storing a list of visited zvals so their unreference/collection can be deferred until the end of the top level serialize call.

The last option is what this patch implements.

I do not vouch for this implementation - in specific, I found I needed to Z_ADDREF_P when capturing zvals but zval_ptr_dtor for cleanup, otherwise I would be warned about leaked objects. I'm not clear why this would be, or if I'm not just lucky here. Working around the GC in this way is at best a hack and at worst possibly really stupid, so I'm just curious for my own sake whether my understanding of the issues here are correct.

For what it is worth, I also investigated msgpack-php implementation, and it appears their impl shares (or is heavily inspired) by the PHP serialize impl (or vice versa), and I wouldn't be surprised if they run into the same issue.

@ahamid
Copy link
Contributor Author

ahamid commented Dec 25, 2013

Travis build: https://travis-ci.org/php/php-src/builds/15918290

Most failures are expected/known. One stands out (Bug #62672 (Error on serialize of ArrayObject) [ext/spl/tests/bug62672.phpt]), but I cannot reproduce this - it passes locally.

@ghost
Copy link

ghost commented Mar 7, 2015

Can one of the admins verify this patch?

@php-pulls
Copy link

Comment on behalf of krakjoe at php.net:

Since this PR has many merge conflicts, targets a security fix only branch of PHP, and appears abandoned by the author, I'm closing this PR.

Please take this action as encouragement to move forward with a solution to the problem, if indeed it still exists in a supported version of PHP.

@php-pulls php-pulls closed this Jan 1, 2017
@nikic
Copy link
Member

nikic commented Jan 1, 2017

Note that PHP 7 is doing something akin to what is proposed here, using a different implementation. See https://github.com/php/php-src/blob/master/ext/standard/var.c#L629.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants