zend_array_try_init() with dtor can cause engine UAF

[YuanchengJiang]

Description

The following code:

<?php
$date_string = '00.00.0000 - 00:00:00';
$fusion = $date_string;
class Box {
}
class Test {
function __destruct() {
global $box;
$box->value = null;
}
}
function test($box) {
var_dump($box->value = new Test);
}
$box = new Box();
test($box);
try {mb_eregi($fusion,$date_string,$box);} catch (Exception $e) { echo($e); }

Resulted in this output:

/home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_hash.c(2672) : ht=0x7ff28984b360 is being destroyed
php: /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_hash.c:74: void _zend_is_inconsistent(const HashTable *, const char *, int): Assertion `0' failed.
Aborted (core dumped)

PHP Version

nightly

Operating System

No response

[nielsdos]

I'm not a huge fan of adding these engine guards because they complicate things and make slowdowns. This one may be okay because it's never really done on a hot code path AFAIK.

diff --git a/Zend/zend_API.h b/Zend/zend_API.h
index f9fe81d50dd..d91da91bf29 100644
--- a/Zend/zend_API.h
+++ b/Zend/zend_API.h
@@ -1485,7 +1485,10 @@ static zend_always_inline zval *zend_try_array_init_size(zval *zv, uint32_t size
 		}
 		zv = &ref->val;
 	}
-	zval_ptr_dtor(zv);
+	zval garbage;
+	ZVAL_COPY_VALUE(&garbage, zv);
+	ZVAL_NULL(zv);
+	zval_ptr_dtor(&garbage);
 	ZVAL_ARR(zv, arr);
 	return zv;
 }

Basically we make the object inaccessible prior to destruction such that it is protected against the overwrite. This is also consistent with the behaviour if we were to comment out the call to mb_eregi.

cc @iluuu1994 Do you have opinions?

[iluuu1994]

I don't mind this fix, but I agree that it's not a great approach long-term. Maybe static analysis could help detecting these cases, and then we can decide whether it's worth applying fixes, ignoring the issue or looking for an alternative solution, e.g. by delaying all destructor calls to some specified point-in-time. We may try to delay them until the zend_interrupt_helper, but obviously this is a behavioral change and may break some use-cases.

[nielsdos]

right-click > save

I don't mind this fix, but I agree that it's not a great approach long-term.

Yeah, okay, I'll open a PR.

e.g. by delaying all destructor calls to some specified point-in-time.

Would fix the ones in the VM too.
I don't know if you remember the implicit move optimization PR I did, but there was some critique from someone from the Wikimedia team because they actually rely in many places on destructor calls happening at certain points in time. So it may be that the delay solution is also a no-go.

[iluuu1994]

but there was some critique from someone from the Wikimedia team because they actually rely in many places on destructor calls happening at certain points in time

I do remember. That said, it's extremely easy for this to break. E.g. accidentally store a non-static closure as a property, and you're relying on the cycle collector. It would be helpful to consider a specific case and see if there's an alternative solution we could offer.

[nielsdos]

I agree. It seems they're doing it for RAII/defer like purposes. I don't expect first-class "defer" support to be a hard thing to implement (famous last words).