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 29b2ed79b8e2ad256aa39aa1cdf712e8af33e3ad..0cdb3aa0857e59496158b81a70ac709e10c10035 100644 (file)
--- 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 bb8073e1d50dc845720087727afe18c84528f020..dbe2df23deedcb95db14cceb159e7d5799c9f098 100644 (file)
--- 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)