Address review comments

Author: TysonAndre

ext/spl/spl_observer.c

@@ -44,10 +44,9 @@ PHPAPI zend_class_entry     *spl_ce_MultipleIterator;
 
 PHPAPI zend_object_handlers spl_handler_SplObjectStorage;
 
-typedef enum {
-	SOS_OVERRIDDEN_READ_DIMENSION = 1,
-	SOS_OVERRIDDEN_WRITE_DIMENSION = 2,
-} SplObjectStorageBitFlags;
+/* Bit flags for marking internal functionality overridden by SplObjectStorage subclasses. */
+#define SOS_OVERRIDDEN_READ_DIMENSION  1
+#define SOS_OVERRIDDEN_WRITE_DIMENSION 2
 
 typedef struct _spl_SplObjectStorage { /* {{{ */
 	HashTable         storage;
@@ -149,16 +148,16 @@ static spl_SplObjectStorageElement *spl_object_storage_create_element(zend_objec
 static spl_SplObjectStorageElement *spl_object_storage_attach_handle(spl_SplObjectStorage *intern, zend_object *obj, zval *inf) /* {{{ */
 {
 	uint32_t handle = obj->handle;
-	zval *entry_zv;
+	zval *entry_zv = zend_hash_index_lookup(&intern->storage, handle);
 	spl_SplObjectStorageElement *pelement;
-	entry_zv = zend_hash_index_lookup(&intern->storage, handle);
 	ZEND_ASSERT(!(intern->flags & SOS_OVERRIDDEN_WRITE_DIMENSION));
 
 	if (Z_TYPE_P(entry_zv) != IS_NULL) {
 		zval zv_inf;
 		ZEND_ASSERT(Z_TYPE_P(entry_zv) == IS_PTR);
 		pelement = Z_PTR_P(entry_zv);
 		ZVAL_COPY_VALUE(&zv_inf, &pelement->inf);
+		ZEND_ASSERT(Z_TYPE(zv_inf) != IS_REFERENCE);
 		if (inf) {
 			ZVAL_COPY(&pelement->inf, inf);
 		} else {
@@ -169,7 +168,6 @@ static spl_SplObjectStorageElement *spl_object_storage_attach_handle(spl_SplObje
 		return pelement;
 	}
 
-	ZEND_ASSERT((GC_FLAGS(&intern->storage) & IS_ARRAY_PERSISTENT) == 0);
 	pelement = spl_object_storage_create_element(obj, inf);
 	ZVAL_PTR(entry_zv, pelement);
 	return pelement;
@@ -193,6 +191,7 @@ spl_SplObjectStorageElement *spl_object_storage_attach(spl_SplObjectStorage *int
 	if (pelement) {
 		zval zv_inf;
 		ZVAL_COPY_VALUE(&zv_inf, &pelement->inf);
+		ZEND_ASSERT(Z_TYPE(zv_inf) != IS_REFERENCE);
 		if (inf) {
 			ZVAL_COPY(&pelement->inf, inf);
 		} else {
@@ -247,8 +246,8 @@ void spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjectStorag
 	intern->index = 0;
 } /* }}} */
 
-#define SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, method) \
-	(((zend_function *)zend_hash_str_find_ptr(&(class_type)->function_table, (method), sizeof((method)) - 1))->common.scope != spl_ce_SplObjectStorage)
+#define SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zstr_method) \
+	(((zend_function *)zend_hash_find_ptr(&(class_type)->function_table, ZSTR_KNOWN(zstr_method)))->common.scope != spl_ce_SplObjectStorage)
 
 static zend_object *spl_object_storage_new_ex(zend_class_entry *class_type, zend_object *orig) /* {{{ */
 {
@@ -275,14 +274,14 @@ static zend_object *spl_object_storage_new_ex(zend_class_entry *class_type, zend
 				if (get_hash->common.scope != spl_ce_SplObjectStorage) {
 					intern->fptr_get_hash = get_hash;
 				}
-				if (get_hash != NULL ||
-					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, "offsetget") ||
-					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, "offsetexists")) {
+				if (intern->fptr_get_hash != NULL ||
+					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, ZEND_STR_OFFSETGET) ||
+					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, ZEND_STR_OFFSETEXISTS)) {
 					intern->flags |= SOS_OVERRIDDEN_READ_DIMENSION;
 				}
 
-				if (get_hash != NULL ||
-					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, "offsetset")) {
+				if (intern->fptr_get_hash != NULL ||
+					SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, ZEND_STR_OFFSETSET)) {
 					intern->flags |= SOS_OVERRIDDEN_WRITE_DIMENSION;
 				}
 			}
@@ -401,20 +400,19 @@ static zend_object *spl_SplObjectStorage_new(zend_class_entry *class_type)
 }
 /* }}} */
 
-/* Returns 1 if the SplObjectStorage contains an entry for getHash(obj), even if the corresponding value is null. */
-int spl_object_storage_contains(spl_SplObjectStorage *intern, zend_object *obj) /* {{{ */
+/* Returns true if the SplObjectStorage contains an entry for getHash(obj), even if the corresponding value is null. */
+bool spl_object_storage_contains(spl_SplObjectStorage *intern, zend_object *obj) /* {{{ */
 {
 	if (EXPECTED(!intern->fptr_get_hash)) {
 		return zend_hash_index_find(&intern->storage, obj->handle) != NULL;
 	}
-	int found;
 	zend_hash_key key;
 	if (spl_object_storage_get_hash(&key, intern, obj) == FAILURE) {
-		return 0;
+		return true;
 	}
 
 	ZEND_ASSERT(key.key);
-	found = zend_hash_exists(&intern->storage, key.key);
+	bool found = zend_hash_exists(&intern->storage, key.key);
 	zend_string_release_ex(key.key, 0);
 
 	return found;
@@ -463,17 +461,18 @@ static zval *spl_object_storage_read_dimension(zend_object *object, zval *offset
 		return zend_std_read_dimension(object, offset, type, rv);
 	}
 	spl_SplObjectStorageElement *element = zend_hash_index_find_ptr(&intern->storage, Z_OBJ_HANDLE_P(offset));
-	if (type == BP_VAR_IS) {
-		if (element == NULL) {
-			return &EG(uninitialized_zval);
-		}
-	}
 
 	if (!element) {
+		if (type == BP_VAR_IS) {
+			return &EG(uninitialized_zval);
+		}
 		zend_throw_exception_ex(spl_ce_UnexpectedValueException, 0, "Object not found");
 		return NULL;
 	} else {
-		ZVAL_COPY_DEREF(rv, &element->inf);
+		/* This deliberately returns a non-reference, even for BP_VAR_W and BP_VAR_RW, to behave the same way as SplObjectStorage did when using the default zend_std_read_dimension behavior.
+		 * i.e. This prevents taking a reference to an entry of SplObjectStorage because offsetGet would return a non-reference. */
+		ZEND_ASSERT(Z_TYPE(element->inf) != IS_REFERENCE);
+		ZVAL_COPY(rv, &element->inf);
 		return rv;
 	}
 }
@@ -538,7 +537,7 @@ PHP_METHOD(SplObjectStorage, offsetGet)
 	if (!element) {
 		zend_throw_exception_ex(spl_ce_UnexpectedValueException, 0, "Object not found");
 	} else {
-		RETURN_COPY_DEREF(&element->inf);
+		RETURN_COPY(&element->inf);
 	}
 } /* }}} */