GH-22354: support interfaces added by attribute validators in zend_compile_implements()

Zend/zend_compile.c

@@ -8963,17 +8963,108 @@ 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. */
+	const size_t array_size = zend_safe_address_guarded(list->children, sizeof(zend_string *), 0);
+	ALLOCA_FLAG(use_heap_to_add)
+	ALLOCA_FLAG(use_heap_skipped)
+	zend_string **to_add_names = do_alloca(array_size, use_heap_to_add);
+	zend_string **skipped_names = do_alloca(array_size, use_heap_skipped);
+	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) {
+				/* Yes, we want to add the interface even though it was already
+				 * added by an attribute. We get here if 1) an attribute added
+				 * the interface (already_added), 2) the developer declared
+				 * the interface manually (this function), and 3) this is
+				 * *not* the first manual declaration of the interface (i.e.
+				 * a previous manual declaration was already skipped, the
+				 * already_skipped flag). In that case, we add the interface
+				 * again so that the standard warning about implementing the
+				 * same interface multiple times is shown. */
+				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]);
+	}
+	free_alloca(skipped_names, use_heap_skipped);
+
+	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++;
 	}
+	ZEND_ASSERT(added_count == to_add_count);
 
-	ce->num_interfaces = list->children;
+	ce->num_interfaces = initial_count + added_count;
 	ce->interface_names = interface_names;
+	free_alloca(to_add_names, use_heap_to_add);
 }
 /* }}} */
 

ext/zend_test/test.c

@@ -30,6 +30,7 @@
 #include "zend_attributes.h"
 #include "zend_enum.h"
 #include "zend_interfaces.h"
+#include "zend_inheritance.h"
 #include "zend_weakrefs.h"
 #include "Zend/Optimizer/zend_optimizer.h"
 #include "Zend/zend_alloc.h"
@@ -61,6 +62,7 @@ static zend_class_entry *zend_test_attribute;
 static zend_class_entry *zend_test_repeatable_attribute;
 static zend_class_entry *zend_test_parameter_attribute;
 static zend_class_entry *zend_test_property_attribute;
+static zend_class_entry *zend_test_attribute_add_interface;
 static zend_class_entry *zend_test_attribute_with_arguments;
 static zend_class_entry *zend_test_class_with_method_with_parameter_attribute;
 static zend_class_entry *zend_test_child_class_with_method_with_parameter_attribute;
@@ -913,6 +915,96 @@ void zend_attribute_validate_zendtestattribute(zend_attribute *attr, uint32_t ta
 	}
 }
 
+/**
+ * Recursive handler to check if a class implements the test interface, either
+ * directly or via inheritance.
+ *
+ * This is needed because even though attribute validators run before interfaces
+ * are added, another attribute validator might have done something weird like
+ * add *our* interface... better safe than sorry
+ */
+static bool check_class_has_interface(zend_class_entry *scope) {
+	/* Might be *linked* (so already have class entries) but not *resolved*,
+	 * since that waits until the inherited parent class is resolved */
+	if (scope->ce_flags & ZEND_ACC_LINKED) {
+		for (uint32_t iii = 0; iii < scope->num_interfaces; iii++) {
+			if (scope->interfaces[iii] == zend_test_interface) {
+				return true;
+			}
+		}
+	} else {
+		for (uint32_t iii = 0; iii < scope->num_interfaces; iii++) {
+			if (zend_string_equals_literal(
+				scope->interface_names[iii].lc_name,
+				"_zendtestinterface"
+			)) {
+				// Interface was added manually be the developer
+				return true;
+			}
+		}
+	}
+	zend_class_entry *parent = NULL;
+	if (scope->ce_flags & ZEND_ACC_LINKED) {
+		if (scope->parent == NULL) {
+			return false;
+		}
+		parent = scope->parent;
+	} else if (scope->parent_name == NULL) {
+		return false;
+	} else {
+		parent = zend_lookup_class_ex(
+			scope->parent_name,
+			NULL,
+			ZEND_FETCH_CLASS_ALLOW_UNLINKED
+		);
+	}
+	if (parent == NULL) {
+		// Invalid class to extend? Leave that up to normal PHP to deal with
+		return false;
+	}
+	return check_class_has_interface(parent);
+}
+
+void zend_attribute_validate_add_interface(zend_attribute *attr, uint32_t target, zend_class_entry *scope)
+{
+	if (target != ZEND_ATTRIBUTE_TARGET_CLASS) {
+		return;
+	}
+	if (scope->ce_flags & (ZEND_ACC_ENUM|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT)) {
+		zend_error_noreturn(E_ERROR, "Only classes can be marked with #[ZendTestAttributeAddsInterface]");
+	}
+	if (check_class_has_interface(scope)) {
+		return;
+	}
+	if (scope->ce_flags & ZEND_ACC_LINKED) {
+		// There is already a method to add interfaces
+		zend_do_implement_interface(scope, zend_test_interface);
+		return;
+	}
+
+	// Add the interface automatically to the list
+	const uint32_t interfaceIdx = scope->num_interfaces;
+	scope->num_interfaces++;
+
+	zend_class_name *newInterfaceSet = safe_erealloc(
+		scope->interface_names,
+		scope->num_interfaces,
+		sizeof(*newInterfaceSet),
+		0
+	);
+	newInterfaceSet[interfaceIdx].name = zend_string_init(
+		"_ZendTestInterface",
+		strlen("_ZendTestInterface"),
+		0
+	);
+	newInterfaceSet[interfaceIdx].lc_name = zend_string_init(
+		"_zendtestinterface",
+		strlen("_zendtestinterface"),
+		0
+	);
+	scope->interface_names = newInterfaceSet;
+}
+
 static ZEND_METHOD(_ZendTestClass, __toString)
 {
 	ZEND_PARSE_PARAMETERS_NONE();
@@ -1296,6 +1388,12 @@ PHP_MINIT_FUNCTION(zend_test)
 	zend_test_property_attribute = register_class_ZendTestPropertyAttribute();
 	zend_mark_internal_attribute(zend_test_property_attribute);
 
+	zend_test_attribute_add_interface = register_class_ZendTestAttributeAddsInterface();
+	{
+		zend_internal_attribute *attr = zend_mark_internal_attribute(zend_test_attribute_add_interface);
+		attr->validator = zend_attribute_validate_add_interface;
+	}
+
 	zend_test_attribute_with_arguments = register_class_ZendTestAttributeWithArguments();
 	zend_mark_internal_attribute(zend_test_attribute_with_arguments);
 

ext/zend_test/test.stub.php

@@ -154,6 +154,9 @@ final class ZendTestPropertyAttribute {
         public function __construct(string $parameter) {}
     }
 
+    #[Attribute(Attribute::TARGET_CLASS)]
+    final class ZendTestAttributeAddsInterface {}
+
     class ZendTestClassWithMethodWithParameterAttribute {
         final public function no_override(
             #[ZendTestParameterAttribute("value2")]

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

@@ -0,0 +1,18 @@
+--TEST--
+Verify that #[ZendTestAttributeAddsInterface] adding an interface doesn't leak (no manual implements)
+--EXTENSIONS--
+zend_test
+--FILE--
+<?php
+
+#[ZendTestAttributeAddsInterface]
+class Demo {}
+
+var_dump(class_implements(Demo::class));
+
+?>
+--EXPECT--
+array(1) {
+  ["_ZendTestInterface"]=>
+  string(18) "_ZendTestInterface"
+}

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

@@ -0,0 +1,18 @@
+--TEST--
+Verify that #[ZendTestAttributeAddsInterface] adding an interface doesn't leak (manually implement same interface)
+--EXTENSIONS--
+zend_test
+--FILE--
+<?php
+
+#[ZendTestAttributeAddsInterface]
+class Demo implements _ZendTestInterface {}
+
+var_dump(class_implements(Demo::class));
+
+?>
+--EXPECT--
+array(1) {
+  ["_ZendTestInterface"]=>
+  string(18) "_ZendTestInterface"
+}

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

@@ -0,0 +1,22 @@
+--TEST--
+Verify that #[ZendTestAttributeAddsInterface] adding an interface doesn't leak (manually implement different interface)
+--EXTENSIONS--
+zend_test
+--FILE--
+<?php
+
+interface MyInterface {}
+
+#[ZendTestAttributeAddsInterface]
+class Demo implements MyInterface {}
+
+var_dump(class_implements(Demo::class));
+
+?>
+--EXPECT--
+array(2) {
+  ["_ZendTestInterface"]=>
+  string(18) "_ZendTestInterface"
+  ["MyInterface"]=>
+  string(11) "MyInterface"
+}

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