From 1266515e19cb30dab5c63df764d18233a234aba6 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Mon, 5 Jan 2015 17:02:11 +0100 Subject: [PATCH] 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. --- Zend/zend_variables.c | 6 ++- ext/intl/collator/collator_convert.c | 2 +- ext/intl/collator/collator_sort.c | 4 +- ext/spl/spl_array.c | 2 +- ext/standard/array.c | 73 ++++++++++++++-------------- ext/standard/assert.c | 4 +- 6 files changed, 45 insertions(+), 46 deletions(-) diff --git a/Zend/zend_variables.c b/Zend/zend_variables.c index 91d68c3985..70f816166b 100644 --- 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 23fd00001d..fb8530c9f9 100644 --- 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 8914736dee..16f68d06ac 100644 --- 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 69d59dc118..7a7ce2c22c 100644 --- 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 cf35823db7..86707f4842 100644 --- 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 a0a8569adb..032b937f17 100644 --- 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); -- 2.40.0