Fix bug #70172

[nikic]

No description provided.

[chtg]

I'm not sure this memory leak can be triggered in 7.x series, since the patch for 5.x series still hasn't been merged in 7.x series.

[nikic]

This patch should cover both the unmerged code in 5.6 (https://github.com/php/php-src/blob/PHP-5.6/ext/standard/var.c#L974-L983) and the leak.

The leak (or at least a leak, it's not really the same) is reproducible on 7.x with the test-case in bug70172_2.phpt with __wakeup commented out (with the __wakeup it causes memory errors and an assertion failure in debug builds).

[smalyshev]

I'm not sure how this would work - if some code inside refers to the current variable, wouldn't the reference keep referring to it and not return_value even after the tmp var was destroyed?

[nikic]

@smalyshev I'm not totally sure what you mean. But yes, for the case of real references (R), the tmp var and the place using R will point to the same zend_reference, while the unserialize() return value will (effectively) be the deref'd value (which is the best we can do, because unserialize() doesn't return by reference -- there is no way to preserve the reference in the return value in that case). For object references (r) the storage location doesn't matter anyway, we only need the value.

[smalyshev]

The problem as it seems to me could be that if some data refers to this variable using R, this reference could be retained in var_hash and then when it comes to cleaning up var_hash it'd try to destroy the variable allocated on stack in a frame that already was destroyed. Am I wrong?

[nikic]

@smalyshev That's exactly the problem we have right now. return_value is a value on the VM stack. We var_push_dtor it, which prevents that the value itself from being destroyed, but the pointer to return_value that is stored in var_entries will still be invalidated. It is likely to point to something else if the VM stack frame has been reused in the meantime.

This patch changes unserialize() to unserialize into a var_dtor_entries slot instead (var_tmp_var() reserves such a slot). var_dtor_entries stays alive until the unserialize context is destroyed, so this pointer cannot be invalidated for the duration of the unserialize.

[smalyshev]

Ah, ok, looks like I was confused and misunderstood what is going on there. OK then, please add test for it and merge.

[nikic]

Okay, I'll do that tomorrow. As @chtg pointed out, this issue is also tracked as bug #72978 (effectively the PHP 7 variant of bug #70172).

[nikic]

Forgot about this, now merged as b2af4e8.