Fix uses of zval_add_ref and add comment on usage

zval_add_ref should be used as a copy ctor, after the value was
already copied.

In particular when used with hash insertions, it should be applied
to the return value of the insert function.

diff --git a/Zend/zend_variables.c b/Zend/zend_variables.c
index 91d68c398512c81c88f32abc0a5db69608d6623e..70f816166b9513b0db616bce9da74b8fbed1fa13 100644 (file)
--- a/Zend/zend_variables.c
+++ b/Zend/zend_variables.c
@@ -201,13 +201,15 @@ ZEND_API void _zval_internal_dtor_for_ptr(zval *zvalue ZEND_FILE_LINE_DC)
        }
 }
 
+/* This function should only be used as a copy constructor, i.e. it
+ * should only be called AFTER a zval has been copied to another
+ * location using ZVAL_COPY_VALUE. Do not call it before copying,
+ * otherwise a reference may be leaked. */
 ZEND_API void zval_add_ref(zval *p)
 {
        if (Z_REFCOUNTED_P(p)) {
                if (Z_ISREF_P(p) && Z_REFCOUNT_P(p) == 1) {
-                       zend_reference *ref = Z_REF_P(p);
                        ZVAL_COPY(p, Z_REFVAL_P(p));
-                       efree_size(ref, sizeof(zend_reference));
                } else {
                        Z_ADDREF_P(p);
                }

diff --git a/ext/intl/collator/collator_convert.c b/ext/intl/collator/collator_convert.c
index 23fd00001da9a0125a53a912cf8e9b2cd4b45908..fb8530c9f9aaa22ee241d9d2c42471135ffdbf73 100644 (file)
--- a/ext/intl/collator/collator_convert.c
+++ b/ext/intl/collator/collator_convert.c
@@ -35,7 +35,7 @@
 #endif
 
 #define COLLATOR_CONVERT_RETURN_FAILED(retval) { \
-                       zval_add_ref( retval );              \
+                       Z_TRY_ADDREF_P(retval);              \
                        return retval;                       \
        }
 

diff --git a/ext/intl/collator/collator_sort.c b/ext/intl/collator/collator_sort.c
index 8914736deec619d8b56481ebfa290164fda9a607..16f68d06ac8657968d137f9f1fbcad069675d559 100644 (file)
--- a/ext/intl/collator/collator_sort.c
+++ b/ext/intl/collator/collator_sort.c
@@ -103,11 +103,11 @@ static int collator_regular_compare_function(zval *result, zval *op1, zval *op2)
                        else
                        {
                                /* str1 is numeric strings => passthru to PHP-compare. */
-                               zval_add_ref( num1_p );
+                               Z_TRY_ADDREF_P(num1_p);
                                norm1_p = num1_p;
 
                                /* str2 is numeric strings => passthru to PHP-compare. */
-                               zval_add_ref( num2_p );
+                               Z_TRY_ADDREF_P(num2_p);
                                norm2_p = num2_p;
                        }
                }

diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
index 69d59dc1188295638561cf22691924def55e778b..7a7ce2c22cd37029d6732673dd007360c6a3df28 100644 (file)
--- a/ext/spl/spl_array.c
+++ b/ext/spl/spl_array.c
@@ -869,7 +869,7 @@ static HashTable* spl_array_get_debug_info(zval *obj, int *is_temp) /* {{{ */
                        zend_hash_copy(intern->debug_info, intern->std.properties, (copy_ctor_func_t) zval_add_ref);
 
                        storage = &intern->array;
-                       zval_add_ref(storage);
+                       Z_TRY_ADDREF_P(storage);
 
                        base = (Z_OBJ_HT_P(obj) == &spl_handler_ArrayIterator) ? spl_ce_ArrayIterator : spl_ce_ArrayObject;
                        zname = spl_gen_private_prop_name(base, "storage", sizeof("storage")-1);

diff --git a/ext/standard/array.c b/ext/standard/array.c
index cf35823db74091dcdf2092826d083bd2a69a2c69..86707f4842e3d193a9562e7e0dee92e9baaa1ec1 100644 (file)
--- a/ext/standard/array.c
+++ b/ext/standard/array.c
@@ -1628,11 +1628,11 @@ PHP_FUNCTION(array_fill)
 
        num--;
        zend_hash_index_update(Z_ARRVAL_P(return_value), start_key, val);
-       zval_add_ref(val);
+       Z_TRY_ADDREF_P(val);
 
        while (num--) {
                if (zend_hash_next_index_insert(Z_ARRVAL_P(return_value), val) != NULL) {
-                       zval_add_ref(val);
+                       Z_TRY_ADDREF_P(val);
                } else {
                        zval_dtor(return_value);
                        php_error_docref(NULL, E_WARNING, "Cannot add element to the array as the next element is already occupied");
@@ -1658,12 +1658,12 @@ PHP_FUNCTION(array_fill_keys)
        ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(keys), entry) {
                ZVAL_DEREF(entry);
                if (Z_TYPE_P(entry) == IS_LONG) {
-                       zval_add_ref(val);
+                       Z_TRY_ADDREF_P(val);
                        zend_hash_index_update(Z_ARRVAL_P(return_value), Z_LVAL_P(entry), val);
                } else {
                        zend_string *key = zval_get_string(entry);
 
-                       zval_add_ref(val);
+                       Z_TRY_ADDREF_P(val);
                        zend_symtable_update(Z_ARRVAL_P(return_value), key, val);
 
                        zend_string_release(key);
@@ -2375,18 +2375,16 @@ PHP_FUNCTION(array_slice)
                        break;
                }
 
-               /* Copy elements from input array to the one that's returned */
-               zval_add_ref(entry);
-
                if (string_key) {
-                       zend_hash_add_new(Z_ARRVAL_P(return_value), string_key, entry);
+                       entry = zend_hash_add_new(Z_ARRVAL_P(return_value), string_key, entry);
                } else {
                        if (preserve_keys) {
-                               zend_hash_index_add_new(Z_ARRVAL_P(return_value), num_key, entry);
+                               entry = zend_hash_index_add_new(Z_ARRVAL_P(return_value), num_key, entry);
                        } else {
-                               zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), entry);
+                               entry = zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), entry);
                        }
                }
+               zval_add_ref(entry);
        } ZEND_HASH_FOREACH_END();
 }
 /* }}} */
@@ -2801,7 +2799,10 @@ PHP_FUNCTION(array_values)
        /* Go through input array and add values to the return array */
        ZEND_HASH_FILL_PACKED(Z_ARRVAL_P(return_value)) {
                ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(input), entry) {
-                       zval_add_ref(entry);
+                       if (Z_ISREF_P(entry) && Z_REFCOUNT_P(entry) == 1) {
+                               entry = Z_REFVAL_P(entry);
+                       }
+                       Z_TRY_ADDREF_P(entry);
                        ZEND_HASH_FILL_ADD(entry);
                } ZEND_HASH_FOREACH_END();
        } ZEND_HASH_FILL_END();
@@ -2961,17 +2962,17 @@ PHP_FUNCTION(array_reverse)
        array_init_size(return_value, zend_hash_num_elements(Z_ARRVAL_P(input)));
 
        ZEND_HASH_REVERSE_FOREACH_KEY_VAL(Z_ARRVAL_P(input), num_key, string_key, entry) {
-               zval_add_ref(entry);
-
                if (string_key) {
-                       zend_hash_add_new(Z_ARRVAL_P(return_value), string_key, entry);
+                       entry = zend_hash_add_new(Z_ARRVAL_P(return_value), string_key, entry);
                } else {
                        if (preserve_keys) {
-                               zend_hash_index_add_new(Z_ARRVAL_P(return_value), num_key, entry);
+                               entry = zend_hash_index_add_new(Z_ARRVAL_P(return_value), num_key, entry);
                        } else {
-                               zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), entry);
+                               entry = zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), entry);
                        }
                }
+
+               zval_add_ref(entry);
        } ZEND_HASH_FOREACH_END();
 }
 /* }}} */
@@ -3092,19 +3093,19 @@ PHP_FUNCTION(array_change_key_case)
        array_init_size(return_value, zend_hash_num_elements(Z_ARRVAL_P(array)));
 
        ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(array), num_key, string_key, entry) {
-               zval_add_ref(entry);
-
                if (!string_key) {
-                       zend_hash_index_update(Z_ARRVAL_P(return_value), num_key, entry);
+                       entry = zend_hash_index_update(Z_ARRVAL_P(return_value), num_key, entry);
                } else {
                        if (change_to_upper) {
                                new_key = php_string_toupper(string_key);
                        } else {
                                new_key = php_string_tolower(string_key);
                        }
-                       zend_hash_update(Z_ARRVAL_P(return_value), new_key, entry);
+                       entry = zend_hash_update(Z_ARRVAL_P(return_value), new_key, entry);
                        zend_string_release(new_key);
                }
+
+               zval_add_ref(entry);
        } ZEND_HASH_FOREACH_END();
 }
 /* }}} */
@@ -4090,12 +4091,11 @@ PHP_FUNCTION(array_diff)
                str = zval_get_string(value);
                if (!zend_hash_exists(&exclude, str)) {
                        if (key) {
-                               zval_add_ref(value);
-                               zend_hash_add_new(Z_ARRVAL_P(return_value), key, value);
+                               value = zend_hash_add_new(Z_ARRVAL_P(return_value), key, value);
                        } else {
-                               zval_add_ref(value);
-                               zend_hash_index_add_new(Z_ARRVAL_P(return_value), idx, value);
+                               value = zend_hash_index_add_new(Z_ARRVAL_P(return_value), idx, value);
                        }
+                       zval_add_ref(value);
                }
                zend_string_release(str);
        } ZEND_HASH_FOREACH_END();
@@ -4628,12 +4628,12 @@ PHP_FUNCTION(array_filter)
                        continue;
                }
 
-               zval_add_ref(operand);
                if (string_key) {
-                       zend_hash_update(Z_ARRVAL_P(return_value), string_key, operand);
+                       operand = zend_hash_update(Z_ARRVAL_P(return_value), string_key, operand);
                } else {
-                       zend_hash_index_update(Z_ARRVAL_P(return_value), num_key, operand);
+                       operand = zend_hash_index_update(Z_ARRVAL_P(return_value), num_key, operand);
                }
+               zval_add_ref(operand);
        } ZEND_HASH_FOREACH_END();
 }
 /* }}} */
@@ -4884,17 +4884,16 @@ PHP_FUNCTION(array_chunk)
                }
 
                /* Add entry to the chunk, preserving keys if necessary. */
-               zval_add_ref(entry);
-
                if (preserve_keys) {
                        if (str_key) {
-                               zend_hash_update(Z_ARRVAL(chunk), str_key, entry);
+                               entry = zend_hash_update(Z_ARRVAL(chunk), str_key, entry);
                        } else {
-                               add_index_zval(&chunk, num_key, entry);
+                               entry = zend_hash_index_update(Z_ARRVAL(chunk), num_key, entry);
                        }
                } else {
-                       add_next_index_zval(&chunk, entry);
+                       entry = zend_hash_next_index_insert(Z_ARRVAL(chunk), entry);
                }
+               zval_add_ref(entry);
 
                /* If reached the chunk size, add it to the result array, and reset the
                 * pointer. */
@@ -4945,15 +4944,15 @@ PHP_FUNCTION(array_combine)
                        } else if (Z_TYPE(Z_ARRVAL_P(values)->arData[pos_values].val) != IS_UNDEF) {
                                entry_values = &Z_ARRVAL_P(values)->arData[pos_values].val;
                                if (Z_TYPE_P(entry_keys) == IS_LONG) {
-                                       zval_add_ref(entry_values);
-                                       add_index_zval(return_value, Z_LVAL_P(entry_keys), entry_values);
+                                       entry_values = zend_hash_index_update(Z_ARRVAL_P(return_value),
+                                               Z_LVAL_P(entry_keys), entry_values);
                                } else {
                                        zend_string *key = zval_get_string(entry_keys);
-
-                                       zval_add_ref(entry_values);
-                                       zend_symtable_update(Z_ARRVAL_P(return_value), key, entry_values);
+                                       entry_values = zend_symtable_update(Z_ARRVAL_P(return_value),
+                                               key, entry_values);
                                        zend_string_release(key);
                                }
+                               zval_add_ref(entry_values);
                                pos_values++;
                                break;
                        }

diff --git a/ext/standard/assert.c b/ext/standard/assert.c
index a0a8569adbfafae4ce59dbdd7ae709d640afdb41..032b937f17c35581c91af83e14ccdbad89878ce1 100644 (file)
--- a/ext/standard/assert.c
+++ b/ext/standard/assert.c
@@ -325,11 +325,9 @@ PHP_FUNCTION(assert_options)
                }
                if (ac == 2) {
                        zval_ptr_dtor(&ASSERTG(callback));
-                       ASSERTG(callback) = *value;
-                       zval_add_ref(value);
+                       ZVAL_COPY(&ASSERTG(callback), value);
                }
                return;
-               break;
 
        default:
                php_error_docref(NULL, E_WARNING, "Unknown value %pd", what);