GH-22354: support interfaces added by attribute validators

In `zend_compile_implements()`, when there were no interfaces already added,
keep the existing code of just adding the newly declared interfaces. But, when
some interfaces are already present, don't overwrite them. Instead, add to the
existing list.

In case the developer tries to add an interface that was already added by an
attribute, skip the manual interface addition to avoid errors about trying to
apply the same interface multiple times. But, only skip the *first* manual
interface addition of the same interface, if there are multiple such additions
then there really was an attempt to apply the same interface multiple times.

Fixes #22354

Author: DanielEScherzer

NEWS

@@ -2,6 +2,10 @@ PHP                                                                        NEWS
 |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 ?? ??? ????, PHP 8.4.24
 
+- Core:
+  . Fixed bug GH-22354 (zend_compile_implements() assumes that the class entry
+    has no interfaces already). (DanielEScherzer)
+
 - Reflection:
   . Fixed bug GH-22324 (Ignore leading namespace separator in
     ReflectionParameter::__construct()). (jorgsowa)

Zend/zend_compile.c

@@ -8963,17 +8963,97 @@ static void zend_compile_implements(zend_ast *ast) /* {{{ */
 	zend_class_name *interface_names;
 	uint32_t i;
 
-	interface_names = emalloc(sizeof(zend_class_name) * list->children);
+	// Attribute validators run before `implements` parts are compiled, an
+	// internal attribute from an extension might have added an interface
+	// already so we cannot assume that the list of interfaces is empty.
+	// See GH-22354. The attribute validator might have also added an interface
+	// that the user is trying to manually implement, skip those since otherwise
+	// there are errors about trying to implement an interface that was already
+	// implemented.
+	if (EXPECTED(ce->num_interfaces == 0)) {
+		interface_names = emalloc(sizeof(zend_class_name) * list->children);
+		for (i = 0; i < list->children; ++i) {
+			zend_ast *class_ast = list->child[i];
+			interface_names[i].name =
+				zend_resolve_const_class_name_reference(class_ast, "interface name");
+			interface_names[i].lc_name = zend_string_tolower(interface_names[i].name);
+		}
+
+		ce->num_interfaces = list->children;
+		ce->interface_names = interface_names;
+		return;
+	}
 
+	// Figure out which interfaces in the list should be skipped; first, resolve
+	// the names
+	// BUT, only skip the *first* usage of an interface in the manual `implements`
+	// part, if an interface is added by an attribute but also manually declared
+	// twice it should still be warned about
+	zend_string **to_add_names = safe_emalloc(list->children, sizeof(*to_add_names), 0);
+	zend_string **skipped_names = safe_emalloc(list->children, sizeof(*skipped_names), 0);
+	uint32_t to_add_count = 0;
+	uint32_t skipped_count = 0;
 	for (i = 0; i < list->children; ++i) {
 		zend_ast *class_ast = list->child[i];
-		interface_names[i].name =
-			zend_resolve_const_class_name_reference(class_ast, "interface name");
-		interface_names[i].lc_name = zend_string_tolower(interface_names[i].name);
+		zend_string *name = zend_resolve_const_class_name_reference(class_ast, "interface name");
+		bool already_added = false;
+		for (uint32_t idx = 0; idx < ce->num_interfaces; ++idx) {
+			if (zend_string_equals_ci(name, ce->interface_names[idx].name)) {
+				already_added = true;
+				break;
+			}
+		}
+		if (already_added) {
+			// Did we already skip this interface name once?
+			bool already_skipped = false;
+			for (uint32_t idx = 0; idx < skipped_count; ++idx) {
+				if (zend_string_equals_ci(name, skipped_names[idx])) {
+					already_skipped = true;
+					break;
+				}
+			}
+			if (already_skipped) {
+				to_add_names[i] = name;
+				to_add_count++;
+			} else {
+				to_add_names[i] = NULL;
+				skipped_names[i] = name;
+				skipped_count++;
+			}
+		} else {
+			to_add_names[i] = name;
+			to_add_count++;
+		}
+	}
+	ZEND_ASSERT(skipped_count + to_add_count == list->children);
+
+	// Free the skipped names
+	for (uint32_t idx = 0; idx < skipped_count; ++idx) {
+		zend_string_release(skipped_names[idx]);
+	}
+	efree(skipped_names);
+
+	const uint32_t initial_count = ce->num_interfaces;
+	interface_names = safe_erealloc(ce->interface_names, (initial_count + to_add_count), sizeof(*interface_names), 0);
+
+	uint32_t added_count = 0;
+	for (i = 0; i < list->children; ++i) {
+		zend_string *name = to_add_names[i];
+		if (name == NULL) {
+			// This was one of the names that was already added by a validator
+			continue;
+		}
+		interface_names[initial_count + added_count].name = name;
+		interface_names[initial_count + added_count].lc_name = zend_string_tolower(name);
+		// To make it clear that the to_add_names no longer owns the reference
+		to_add_names[i] = NULL;
+		added_count += 1;
 	}
+	ZEND_ASSERT(added_count == to_add_count);
 
-	ce->num_interfaces = list->children;
+	ce->num_interfaces = initial_count + added_count;
 	ce->interface_names = interface_names;
+	efree(to_add_names);
 }
 /* }}} */
 

ext/zend_test/tests/attribute-adds-interface-02.phpt

@@ -1,7 +1,5 @@
 --TEST--
 Verify that #[ZendTestAttributeAddsInterface] adding an interface doesn't leak (manually implement same interface)
---XFAIL--
-Currently leaks
 --EXTENSIONS--
 zend_test
 --FILE--

ext/zend_test/tests/attribute-adds-interface-03.phpt

@@ -1,7 +1,5 @@
 --TEST--
 Verify that #[ZendTestAttributeAddsInterface] adding an interface doesn't leak (manually implement different interface)
---XFAIL--
-Currently leaks and overwrites the interface added by the attribute
 --EXTENSIONS--
 zend_test
 --FILE--

ext/zend_test/tests/attribute-adds-interface-04.phpt

@@ -0,0 +1,15 @@
+--TEST--
+Verify that #[ZendTestAttributeAddsInterface] and manually implementing same interface repeatedly errors
+--EXTENSIONS--
+zend_test
+--FILE--
+<?php
+
+#[ZendTestAttributeAddsInterface]
+class Demo implements _ZendTestInterface, _ZendTestInterface {}
+
+var_dump(class_implements(Demo::class));
+
+?>
+--EXPECTF--
+Fatal error: Class Demo cannot implement previously implemented interface _ZendTestInterface in %s on line %d

ext/zend_test/tests/attribute-adds-interface-05.phpt

@@ -0,0 +1,17 @@
+--TEST--
+Verify that #[ZendTestAttributeAddsInterface] and manually implementing a different interface repeatedly errors
+--EXTENSIONS--
+zend_test
+--FILE--
+<?php
+
+interface MyInterface {}
+
+#[ZendTestAttributeAddsInterface]
+class Demo implements MyInterface, MyInterface {}
+
+var_dump(class_implements(Demo::class));
+
+?>
+--EXPECTF--
+Fatal error: Class Demo cannot implement previously implemented interface MyInterface in %s on line %d