Prevent operands from being released during comparison

Fixes phpGH-19305
Closes phpGH-19309

Author: arnaud-lb

NEWS

@@ -9,6 +9,8 @@ PHP                                                                        NEWS
     unpacking). (ilutov)
   . Fixed OSS-Fuzz #434346548 (Failed assertion with throwing __toString in
     binary const expr). (ilutov)
+  . Fixed bug GH-19305 (Operands may be being released during comparison).
+    (Arnaud)
 
 - FTP:
   . Fix theoretical issues with hrtime() not being available. (nielsdos)

Zend/tests/gh19305-001.phpt

@@ -0,0 +1,27 @@
+--TEST--
+GH-19305 001: Operands may be released during comparison
+--FILE--
+<?php
+
+$a = (object)[
+    'foo' => 'test',
+    'bar' => 2,
+];
+$b = (object)[
+    'foo' => new class {
+        public function __toString() {
+            global $a, $b;
+            $a = $b = null;
+            return '';
+        }
+    },
+    'bar' => 2,
+];
+
+// Comparison of $a->foo and $b->foo calls __toString(), which releases
+// both $a and $b.
+var_dump($a > $b);
+
+?>
+--EXPECT--
+bool(true)

Zend/tests/gh19305-002.phpt

@@ -0,0 +1,27 @@
+--TEST--
+GH-19305 002: Operands may be released during comparison
+--FILE--
+<?php
+
+$a = [
+    'foo' => 'test',
+    'bar' => 2,
+];
+$b = [
+    'foo' => new class {
+        public function __toString() {
+            global $a, $b;
+            $a = $b = null;
+            return '';
+        }
+    },
+    'bar' => 2,
+];
+
+// Comparison of $a['foo'] and $b['foo'] calls __toString(), which releases
+// both $a and $b.
+var_dump($a > $b);
+
+?>
+--EXPECT--
+bool(true)

Zend/tests/gh19305-003.phpt

@@ -0,0 +1,28 @@
+--TEST--
+GH-19305 003: Operands may be released during comparison
+--SKIPIF--
+<?php
+if (!method_exists('ReflectionClass', 'newLazyGhost')) {
+    die('skip No lazy objects');
+}
+?>
+--FILE--
+<?php
+
+class C
+{
+    public $s;
+}
+$r = new ReflectionClass(C::class);
+$o = $r->newLazyProxy(function () { return new C; });
+
+// Comparison calls initializers, which releases $o
+var_dump($o >
+$r->newLazyGhost(function () {
+    global $o;
+    $o = null;
+}));
+
+?>
+--EXPECT--
+bool(false)

Zend/zend_object_handlers.c

@@ -1792,6 +1792,10 @@ ZEND_API int zend_std_compare_objects(zval *o1, zval *o2) /* {{{ */
 		}
 		Z_PROTECT_RECURSION_P(o1);
 
+		GC_ADDREF(zobj1);
+		GC_ADDREF(zobj2);
+		int ret;
+
 		for (i = 0; i < zobj1->ce->default_properties_count; i++) {
 			zval *p1, *p2;
 
@@ -1806,35 +1810,50 @@ ZEND_API int zend_std_compare_objects(zval *o1, zval *o2) /* {{{ */
 
 			if (Z_TYPE_P(p1) != IS_UNDEF) {
 				if (Z_TYPE_P(p2) != IS_UNDEF) {
-					int ret;
-
 					ret = zend_compare(p1, p2);
 					if (ret != 0) {
 						Z_UNPROTECT_RECURSION_P(o1);
-						return ret;
+						goto done;
 					}
 				} else {
 					Z_UNPROTECT_RECURSION_P(o1);
-					return 1;
+					ret = 1;
+					goto done;
 				}
 			} else {
 				if (Z_TYPE_P(p2) != IS_UNDEF) {
 					Z_UNPROTECT_RECURSION_P(o1);
-					return 1;
+					ret = 1;
+					goto done;
 				}
 			}
 		}
 
 		Z_UNPROTECT_RECURSION_P(o1);
-		return 0;
+		ret = 0;
+
+done:
+		OBJ_RELEASE(zobj1);
+		OBJ_RELEASE(zobj2);
+
+		return ret;
 	} else {
+		GC_ADDREF(zobj1);
+		GC_ADDREF(zobj2);
+
 		if (!zobj1->properties) {
 			rebuild_object_properties(zobj1);
 		}
 		if (!zobj2->properties) {
 			rebuild_object_properties(zobj2);
 		}
-		return zend_compare_symbol_tables(zobj1->properties, zobj2->properties);
+
+		int ret = zend_compare_symbol_tables(zobj1->properties, zobj2->properties);
+
+		OBJ_RELEASE(zobj1);
+		OBJ_RELEASE(zobj2);
+
+		return ret;
 	}
 }
 /* }}} */

Zend/zend_operators.c

@@ -3384,7 +3384,19 @@ static int hash_zval_compare_function(zval *z1, zval *z2) /* {{{ */
 
 ZEND_API int ZEND_FASTCALL zend_compare_symbol_tables(HashTable *ht1, HashTable *ht2) /* {{{ */
 {
-	return ht1 == ht2 ? 0 : zend_hash_compare(ht1, ht2, (compare_func_t) hash_zval_compare_function, 0);
+	if (ht1 == ht2) {
+		return 0;
+	}
+
+	GC_TRY_ADDREF(ht1);
+	GC_TRY_ADDREF(ht2);
+
+	int ret = zend_hash_compare(ht1, ht2, (compare_func_t) hash_zval_compare_function, 0);
+
+	GC_TRY_DTOR_NO_REF(ht1);
+	GC_TRY_DTOR_NO_REF(ht2);
+
+	return ret;
 }
 /* }}} */
 

Zend/zend_types.h

@@ -731,6 +731,18 @@ static zend_always_inline uint8_t zval_get_type(const zval* pz) {
 		} \
 	} while (0)
 
+#define GC_TRY_DTOR_NO_REF(p) \
+	do { \
+		zend_refcounted_h *_p = &(p)->gc; \
+		if (!(_p->u.type_info & GC_IMMUTABLE)) { \
+			if (zend_gc_delref(_p) == 0) { \
+				rc_dtor_func((zend_refcounted *)_p); \
+			} else { \
+				gc_check_possible_root_no_ref((zend_refcounted *)_p); \
+			} \
+		} \
+	} while (0)
+
 #define GC_TYPE_MASK				0x0000000f
 #define GC_FLAGS_MASK				0x000003f0
 #define GC_INFO_MASK				0xfffffc00