Fix use-after-free serializing an array grown by an element's hook

[iliaal]

The IS_ARRAY case of php_var_serialize_intern() walks the array's HashTable without holding a reference while php_var_serialize_nested_data() recurses into user hooks, so a __serialize() that grows the same array through a by-reference alias reallocs the backing store mid-walk and the iterator reads freed memory. The object case, var_dump, and var_export already hold a ref across the walk; this adds the same GC_ADDREF/GC_DTOR_NO_REF for arrays, so the append separates a copy instead of reallocating in place. __sleep() and Serializable::serialize() reach the same walk, so one test covers all three.

Reproducer (invalid read in php_var_serialize_nested_data under ASan/valgrind; a normal build usually reads the freed memory and passes, so the test is an ASan canary):

<?php
class G {
    public $ref;
    public function __serialize(): array {
        for ($i = 0; $i < 128; $i++) $this->ref[] = 'x' . $i;
        return ['d' => 1];
    }
}
$g = new G();
$inner = [$g, 'tail'];
$g->ref = &$inner;
$top = [&$inner];
serialize($top);

[ndossche]

GC_TRY_ADDREF

[ndossche]

GC_TRY_DTOR_NO_REF

[ndossche]

I would only merge this in master though

[iliaal]

I would only merge this in master though

Any reason for master only? Change seem fairly safe to me and does address UAF.

[ndossche]

I would only merge this in master though

Any reason for master only? Change seem fairly safe to me and does address UAF.

Generally, for maliciously-crafted code we only do these changes on master to avoid breakage.
For me personally, I'm worried about any refcount interactions in the serialization code. Since the serializer relies on refcounts in some places, we had some breakage of such code in the past.
If the code sits in master for a while and does not break anything, we can consider backporting it.

[iliaal]

I would only merge this in master though

Any reason for master only? Change seem fairly safe to me and does address UAF.

Generally, for maliciously-crafted code we only do these changes on master to avoid breakage. For me personally, I'm worried about any refcount interactions in the serialization code. Since the serializer relies on refcounts in some places, we had some breakage of such code in the past. If the code sits in master for a while and does not break anything, we can consider backporting it.

Fair enough I'll update the PR to master