Conversation
|
Travis build: https://travis-ci.org/php/php-src/builds/15918290 Most failures are expected/known. One stands out ( |
|
Can one of the admins verify this patch? |
|
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. |
|
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. |
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:
The last option is what this patch implements.
I do not vouch for this implementation - in specific, I found I needed to
Z_ADDREF_Pwhen capturing zvals butzval_ptr_dtorfor 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.