From: Nikita Popov <nikita.ppv@gmail.com>
Date: Tue, 16 Feb 2021 08:55:02 +0000 (+0100)
Subject: Delref only after successful allocation
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=353f7ffb6b3f6e7c3aed8b3ed51182a973e120b8;p=php

Delref only after successful allocation

Otherwise we may have inconsistent refcounts after OOM.

I expect this problem is much more prevalent, but this at least
fixes some string/array separation cases.

Fixes oss-fuzz #30999.
---

diff --git a/Zend/zend_operators.c b/Zend/zend_operators.c
index 29b2ed79b8..0cdb3aa085 100644
--- a/Zend/zend_operators.c
+++ b/Zend/zend_operators.c
@@ -2323,8 +2323,10 @@ static void ZEND_FASTCALL increment_string(zval *str) /* {{{ */
 		Z_STR_P(str) = zend_string_init(Z_STRVAL_P(str), Z_STRLEN_P(str), 0);
 		Z_TYPE_INFO_P(str) = IS_STRING_EX;
 	} else if (Z_REFCOUNT_P(str) > 1) {
-		Z_DELREF_P(str);
+		/* Only release string after allocation succeeded. */
+		zend_string *orig_str = Z_STR_P(str);
 		Z_STR_P(str) = zend_string_init(Z_STRVAL_P(str), Z_STRLEN_P(str), 0);
+		GC_DELREF(orig_str);
 	} else {
 		zend_string_forget_hash_val(Z_STR_P(str));
 	}
diff --git a/Zend/zend_types.h b/Zend/zend_types.h
index bb8073e1d5..dbe2df23de 100644
--- a/Zend/zend_types.h
+++ b/Zend/zend_types.h
@@ -625,6 +625,7 @@ static zend_always_inline zend_uchar zval_get_type(const zval* pz) {
 #define GC_ADDREF_EX(p, rc)			zend_gc_addref_ex(&(p)->gc, rc)
 #define GC_DELREF_EX(p, rc)			zend_gc_delref_ex(&(p)->gc, rc)
 #define GC_TRY_ADDREF(p)			zend_gc_try_addref(&(p)->gc)
+#define GC_TRY_DELREF(p)			zend_gc_try_delref(&(p)->gc)
 
 #define GC_TYPE_MASK				0x0000000f
 #define GC_FLAGS_MASK				0x000003f0
@@ -1180,6 +1181,13 @@ static zend_always_inline void zend_gc_try_addref(zend_refcounted_h *p) {
 	}
 }
 
+static zend_always_inline void zend_gc_try_delref(zend_refcounted_h *p) {
+	if (!(p->u.type_info & GC_IMMUTABLE)) {
+		ZEND_RC_MOD_CHECK(p);
+		--p->refcount;
+	}
+}
+
 static zend_always_inline uint32_t zend_gc_delref(zend_refcounted_h *p) {
 	ZEND_ASSERT(p->refcount > 0);
 	ZEND_RC_MOD_CHECK(p);
@@ -1351,9 +1359,9 @@ static zend_always_inline uint32_t zval_delref_p(zval* pz) {
 			zend_string *_str = Z_STR_P(_zv);			\
 			ZEND_ASSERT(Z_REFCOUNTED_P(_zv));			\
 			ZEND_ASSERT(!ZSTR_IS_INTERNED(_str));		\
-			Z_DELREF_P(_zv);							\
 			ZVAL_NEW_STR(_zv, zend_string_init(			\
 				ZSTR_VAL(_str),	ZSTR_LEN(_str), 0));	\
+			GC_DELREF(_str);							\
 		}												\
 	} while (0)
 
@@ -1361,10 +1369,8 @@ static zend_always_inline uint32_t zval_delref_p(zval* pz) {
 		zval *__zv = (zv);								\
 		zend_array *_arr = Z_ARR_P(__zv);				\
 		if (UNEXPECTED(GC_REFCOUNT(_arr) > 1)) {		\
-			if (Z_REFCOUNTED_P(__zv)) {					\
-				GC_DELREF(_arr);						\
-			}											\
 			ZVAL_ARR(__zv, zend_array_dup(_arr));		\
+			GC_TRY_DELREF(_arr);						\
 		}												\
 	} while (0)