Fix GH-12265: Cloning an object breaks serialization recursion #12287
Conversation
ext/standard/var.c
Outdated
| && (Z_OBJ_P(var)->properties == NULL || GC_REFCOUNT(Z_OBJ_P(var)->properties) == 1)) { | ||
| && (Z_OBJ_P(var)->properties == NULL || GC_REFCOUNT(Z_OBJ_P(var)->properties) == 1) | ||
| /* __serialize may arbitrarily increase the refcount */ | ||
| && Z_OBJCE_P(var)->__serialize == NULL) { |
There was a problem hiding this comment.
Isn't this also a possible problem for __sleep()?
There was a problem hiding this comment.
Indeed, added a check for this and a test. Although the check is unfortunately more expensive than the one for __serialize, so we maybe need a different solution...
For example by calling var_add_hash before php_var_serialize_call_sleep when it hasn't been called before (or early-returned before).
There was a problem hiding this comment.
So another follow-up question is, what happens if Serializable is used without __serialize() (other than a deprecation notice)?
We really should think about deprecating __sleep()/__wakeup() as it seems to me the functionality is covered by the new serialize magic methods.
There was a problem hiding this comment.
So another follow-up question is, what happens if Serializable is used without __serialize() (other than a deprecation notice)?
I don't think this is a problem for this bug? But I could be wrong or misunderstanding the question.
There was a problem hiding this comment.
IIRC If Serializable is implemented, serialize() function will call the serialize() method of the object, similar to __sleep()/__serialize()
There was a problem hiding this comment.
That serialize methods is supposed to returns the string that will be embedded in serialize's output. As the bug manifests in the built-in reference tracking when it performs the default serialization, I don't think we'll have that issue in that case?
There was a problem hiding this comment.
Ah so that's how it works, yeah should be fine then
It broke because of the refcount==1 condition added in 6c5942f. Removing that condition would work, but remove an optimization. The test file starts with the outer object at RC1 so it isn't registered in var_hash, but during serialization it gets an additional reference so now we have a problem.