Fix bug #71334

Always duplicate the array before doing a sort with user-defined
comparison function, to avoid access to the intermediate
inconsistent state.

I've also dropped the "array modification" warning, as protection
against modifications is no longer relevant if we're always working
on a copy anyway.

This also required some changes to how SplArray forwards calls to
sorting functions.

1 files changed, 30 insertions, 15 deletions

diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
index d5cb32606e..60cbac5726 100644
--- a/ext/spl/spl_array.c
+++ b/ext/spl/spl_array.c
@@ -82,18 +82,18 @@ static inline spl_array_object *spl_array_from_obj(zend_object *obj) /* {{{ */ {
 
 #define Z_SPLARRAY_P(zv)  spl_array_from_obj(Z_OBJ_P((zv)))
 
-static inline HashTable *spl_array_get_hash_table(spl_array_object* intern) { /* {{{ */
+static inline HashTable **spl_array_get_hash_table_ptr(spl_array_object* intern) { /* {{{ */
 	//??? TODO: Delay duplication for arrays; only duplicate for write operations
 	if (intern->ar_flags & SPL_ARRAY_IS_SELF) {
 		if (!intern->std.properties) {
 			rebuild_object_properties(&intern->std);
 		}
-		return intern->std.properties;
+		return &intern->std.properties;
 	} else if (intern->ar_flags & SPL_ARRAY_USE_OTHER) {
 		spl_array_object *other = Z_SPLARRAY_P(&intern->array);
-		return spl_array_get_hash_table(other);
+		return spl_array_get_hash_table_ptr(other);
 	} else if (Z_TYPE(intern->array) == IS_ARRAY) {
-		return Z_ARRVAL(intern->array);
+		return &Z_ARRVAL(intern->array);
 	} else {
 		zend_object *obj = Z_OBJ(intern->array);
 		if (!obj->properties) {
@@ -104,9 +104,22 @@ static inline HashTable *spl_array_get_hash_table(spl_array_object* intern) { /*
 			}
 			obj->properties = zend_array_dup(obj->properties);
 		}
-		return obj->properties;
+		return &obj->properties;
 	}
-} /* }}} */
+}
+/* }}} */
+
+static inline HashTable *spl_array_get_hash_table(spl_array_object* intern) { /* {{{ */
+	return *spl_array_get_hash_table_ptr(intern);
+}
+/* }}} */
+
+static inline void spl_array_replace_hash_table(spl_array_object* intern, HashTable *ht) { /* {{{ */
+	HashTable **ht_ptr = spl_array_get_hash_table_ptr(intern);
+	zend_array_destroy(*ht_ptr);
+	*ht_ptr = ht;
+}
+/* }}} */
 
 static inline zend_bool spl_array_is_object(spl_array_object *intern) /* {{{ */
 {
@@ -1432,16 +1445,12 @@ static void spl_array_method(INTERNAL_FUNCTION_PARAMETERS, char *fname, int fnam
 	spl_array_object *intern = Z_SPLARRAY_P(getThis());
 	HashTable *aht = spl_array_get_hash_table(intern);
 	zval function_name, params[2], *arg = NULL;
-	uint32_t old_refcount;
 
 	ZVAL_STRINGL(&function_name, fname, fname_len);
 
-	/* A tricky way to pass "aht" by reference, reset refcount */
-	//??? It may be not safe, if user comparison handler accesses "aht"
-	old_refcount = GC_REFCOUNT(aht);
-	GC_REFCOUNT(aht) = 1;
 	ZVAL_NEW_EMPTY_REF(&params[0]);
 	ZVAL_ARR(Z_REFVAL(params[0]), aht);
+	GC_REFCOUNT(aht)++;
 
 	if (!use_arg) {
 		intern->nApplyCount++;
@@ -1470,10 +1479,16 @@ static void spl_array_method(INTERNAL_FUNCTION_PARAMETERS, char *fname, int fnam
 	}
 
 exit:
-	/* A tricky way to pass "aht" by reference, copy back and cleanup */
-	GC_REFCOUNT(aht) = old_refcount;
-	efree(Z_REF(params[0]));
-	zend_string_free(Z_STR(function_name));
+	{
+		HashTable *new_ht = Z_ARRVAL_P(Z_REFVAL(params[0]));
+		if (aht != new_ht) {
+			spl_array_replace_hash_table(intern, new_ht);
+		} else {
+			GC_REFCOUNT(aht)--;
+		}
+		efree(Z_REF(params[0]));
+		zend_string_free(Z_STR(function_name));
+	}
 } /* }}} */
 
 #define SPL_ARRAY_METHOD(cname, fname, use_arg) \