Zend/zend_execute: clear reference before calling dtor

[MaxKellermann]

If we don't do this, then zend_objects_store_del() will call the PHP destructor, which may then recurse into zend_objects_store_del(), leading to a use-after-free / double-free bug.

Fixes failures of Zend/tests/gh10168_3.phpt (added recently by 71ddede) which, depending on the timing, can trigger the recursive zend_objects_store_del() call.

This is similar to commit a057f06

@dstogov @nielsdos

[iluuu1994]

There's an existing PR here to fix the issue. #10546 I'll have a closer look later today.

[MaxKellermann]

There's an existing PR here to fix the issue

Oh! But I guess my fix is both simpler and more reliable; it fixes the recursion where it happens, and it fixes the problem for both callers.

[nielsdos]

From a brief look this makes sense to me, I don't know which approach is best though, I would need more experience with the core.
One small detail: you should move the assert before the ZVAL_UNDEF.

[MaxKellermann]

One small detail: you should move the assert before the ZVAL_UNDEF.

Of course, I changed it, thanks!

[iluuu1994]

This PR doesn't make sense. Decreasing the refcount should definitely not unset the value. This is observable.

class Test
{
	static ?Test $a = null;

	public function __construct() {
		if (self::$a === null) {
			self::$a = &$this;
		} else {
			self::$a = $this;
		}
	}

	function __destruct() {
		var_dump(self::$a);
		self::$a = null;
	}
}
new Test();
new Test();

UNKNOWN:0
object(Test)#2 (0) {
}

#10546 solves one part of the problem (the destructor being called before the new value is assigned). The other problem is that in many places zend_assign_to_variable is called and the result value used. That is unsafe however, since the assignment might release the returned value. This can even happen for write_property, which unfortunately we can't fix in a patch... Anyway, I adjusted as many places as I could in the other PR. I'll still need to make sure it is correct.

[MaxKellermann]

Decreasing the refcount should definitely not unset the value

Oh, I think I see my mistake - I should only unset it when !GC_DELREF, right? Only unset it when the dtor actually gets called.

[iluuu1994]

@MaxKellermann I think the result will be the same, because the unsetting happens before calling the destructor.

[iluuu1994]

If I understand it correctly, this patch avoids the problem by self::$a = null; not releasing the object anymore (because self::$a is UNDEF at that point). This solution also seems to leak, because the destructor might assign some allocated value to the given property.

class Foo {}

class Test
{
	static Test|Foo|null $a = null;

	public function __construct() {
		if (self::$a === null) {
			var_dump(self::$a = &$this);
		} else {
			var_dump(self::$a = $this);
		}
	}

	function __destruct() {
		self::$a = new Foo();
	}
}
new Test();
new Test();

I think the value should be assigned before actually calling the destructor.

[MaxKellermann]

Only unset it when the dtor actually gets called.

That, too was a mistake - but it's not the one you meant, I think.

You are right that my fix clears the reference, so the destructor sees it's already NULL.

[MaxKellermann]

This solution also seems to leak, because the destructor might assign some allocated value to the given property.

Damn, this sounds like it could become an endless loop, because before assigning, zend_execute.c would need to check again and again if the dtor that was implicitly called set another value meanwhile.... this needs a whole new approach.

[nielsdos]

It's all quite hairy. I think ideally all destructors would be executed after the opcode is executed, but that would require a enormous amount of refactoring.

[MaxKellermann]

I think ideally all destructors would be executed after the opcode is executed

Yes, postponing this when it's safe sounds like a good plan - but it has visible side effects, too - the destructor would already see the new value. Would that be okay?

[iluuu1994]

Yeah it's quite the rabbit hole... I think #10546 is close to being correct, apart from write_property (and the JIT failure which I have to check).

[nielsdos]

I think ideally all destructors would be executed after the opcode is executed

Yes, postponing this when it's safe sounds like a good plan - but it has visible side effects, too - the destructor would already see the new value. Would that be okay?

Sounds like BC breakage unfortunately...

[MaxKellermann]

I think #10546 is close to being correct

After glimpsing into the rabbit hole, I agree that f0e1e01 is the better fix - I thought mine was simpler and more elegant, because often the simpler fix is the better one, but there's indeed some unavoidable complexity involved.

When stuff invokes callbacks (dtors, exception handlers etc.), everything can happen, and getting it right is difficult. Postponing handlers is indeed the way to go, but it's strictly a breakage... but unavoidable if you want to fix this ugly bug!

[MaxKellermann]

It would be possible to implement it in a way without BC breakage. To do that, during assignment, one must hold a reference to the object, but call zend_objects_store_del() with a non-zero refcount (which requires removing the ZEND_ASSERT(GC_REFCOUNT(object) == 0)); that would invoke the destructor, but would not efree() the actual object.
That way, the destructor gets called at the right time, during assignment, not after.
I figured that it is already possible to increment the reference counter even after the destructor got called, by letting the destructor assign itself to a global/static variable. That leaves an object that was already destructed; it can still be used, just the destructor will never be called again. Doing that probably breaks each and every class, but it's technically possible.
Therefore, it's also possible to keep holding a reference while calling the destructor.

That adds some complexity and a tiny amount of overhead, but it avoids the BC breakage. If there is interest in such an implementation, I'd be willing to give it a try. @iluuu1994, @nielsdos or anybody else?

[nielsdos]

I find it a very interesting idea, but I'm not sure what kind of rabbit hole that will open. I'm not qualified nor experience enough with the core to say whether it's a good idea or not though.

[iluuu1994]

That way, the destructor gets called at the right time, during assignment, not after.

I'm not sure that is the right point in time to call the destructor. That leads to a weird control flow, especially when value is re-assigned in the destructor (outer assignment calls destructor of old value, destructor overwrites the value, outer assignment happens). In that case, the outer assignment overwrite the value again which is not what I would expect.

This also seems like the exact opposite of what was suggested here:

I think ideally all destructors would be executed after the opcode is executed

[MaxKellermann]

This also seems like the exact opposite of what was suggested here:

I think ideally all destructors would be executed after the opcode is executed

Depends on what your goal is. What you wrote sounds reasonable, but if you want to avoid the BC breakage at all cost, then you can't do that.

[iluuu1994]

@MaxKellermann I think #10546 should solve all issues except for one (#10546 (comment)). Unfortunately, I don't think that one can be solved without a BC break. It doesn't cause memory corruption, but the result is inconsistent... Or is there an issue I'm missing?

[dstogov]

The problem is really huge and it answers the question why Java doesn't have destructors.
In PHP we have much bigger problem because of an ability of indirect modification of sensetive data in the middle of update through user error handlers, destructors, and other magic functions (e.g. __toString).

I would propose to think in a different way. Instead of trying fixing each place where such update is possible, we may try to prohibit dangerous modifications. I mean FETCH_GLOBAL might check some ZVAL flag and prohibit future ZVAL modification (emit fatal error "attempt of indirect modification of sensitive/frozen/dirty data"). This is a quick idea, it's not completely formed...