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.

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);