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

[DanielEScherzer]

No description provided.

[TimWolla]

Logic-wise this seems correct. But I feel this is a “master-only” bugfix, since the situation for this to appear seems to be quite rare.

[TimWolla]

Should also be safe_emalloc.

[DanielEScherzer]

This line is unchanged from earlier and I'd prefer not to mess with it - I just moved it into a conditional block

[TimWolla]

You are repeating initial_count + to_add_count a lot. I suggest a local variable.

[DanielEScherzer]

This is precisely the only place that that expression occurs? Below I use added_count which also changes each time through the loop so shouldn't be saved

[TimWolla]

Ah, yes. In that case using pointer arithmetic might reduce the amount of copy and paste required.

[DanielEScherzer]

Logic-wise this seems correct. But I feel this is a “master-only” bugfix, since the situation for this to appear seems to be quite rare.

I encountered this in an actual extension, https://github.com/DanielEScherzer/CustomCast - I'm not sure why I didn't notice or report it earlier, but it does indeed affect me

[DanielEScherzer]

This line is unchanged from earlier and I'd prefer not to mess with it - I just moved it into a conditional block

[DanielEScherzer]

This is precisely the only place that that expression occurs? Below I use added_count which also changes each time through the loop so shouldn't be saved

[TimWolla]

Also requested another review from someone more experienced with engine code to make a target-branch call.

[iluuu1994]

I agree with Tim that this should target master, this is borderline unintended. I also don't love this approach, because if mutation of classes is allowed, adding an interface is just one thing the attribute could do. It might be better to add a new attribute hook that triggers after the class is fully compiled, after which the attribute can do it's thing, and in this case handle the duplicate interface logic itself.

[DanielEScherzer]

I agree with Tim that this should target master, this is borderline unintended. I also don't love this approach, because if mutation of classes is allowed, adding an interface is just one thing the attribute could do.

Yeah, but that doesn't mean that the mutation would break existing assumptions in zend_compile.c. From the point that attributes are compiled on a class:

php-src/Zend/zend_compile.c

Lines 9641 to 9767 in 470b424

	if (decl->child[3]) {
	zend_compile_attributes(&ce->attributes, decl->child[3], 0, ZEND_ATTRIBUTE_TARGET_CLASS, 0);
	}
	if (implements_ast) {
	zend_compile_implements(implements_ast);
	}
	if (ce->ce_flags & ZEND_ACC_ENUM) {
	if (enum_backing_type_ast != NULL) {
	zend_compile_enum_backing_type(ce, enum_backing_type_ast);
	}
	zend_enum_add_interfaces(ce);
	zend_enum_register_props(ce);
	}
	zend_compile_stmt(stmt_ast);
	/* Reset lineno for final opcodes and errors */
	CG(zend_lineno) = ast->lineno;
	if ((ce->ce_flags & (ZEND_ACC_IMPLICIT_ABSTRACT_CLASS|ZEND_ACC_INTERFACE|ZEND_ACC_TRAIT|ZEND_ACC_EXPLICIT_ABSTRACT_CLASS)) == ZEND_ACC_IMPLICIT_ABSTRACT_CLASS) {
	zend_verify_abstract_class(ce);
	}
	CG(active_class_entry) = original_ce;
	if (toplevel) {
	ce->ce_flags |= ZEND_ACC_TOP_LEVEL;
	}
	/* We currently don't early-bind classes that implement interfaces or use traits */
	if (!ce->num_interfaces && !ce->num_traits && !ce->num_hooked_prop_variance_checks
	#ifdef ZEND_OPCACHE_SHM_REATTACHMENT
	/* See zend_link_hooked_object_iter(). */
	&& !ce->num_hooked_props
	#endif
	&& !(CG(compiler_options) & ZEND_COMPILE_WITHOUT_EXECUTION)) {
	if (toplevel) {
	if (extends_ast) {
	zend_class_entry *parent_ce = zend_lookup_class_ex(
	ce->parent_name, NULL, ZEND_FETCH_CLASS_NO_AUTOLOAD);
	if (parent_ce
	&& !zend_compile_ignore_class(parent_ce, ce->info.user.filename)) {
	if (zend_try_early_bind(ce, parent_ce, lcname, NULL)) {
	zend_string_release(lcname);
	return;
	}
	}
	} else if (EXPECTED(zend_hash_add_ptr(CG(class_table), lcname, ce) != NULL)) {
	zend_string_release(lcname);
	zend_build_properties_info_table(ce);
	zend_inheritance_check_override(ce);
	ce->ce_flags |= ZEND_ACC_LINKED;
	zend_observer_class_linked_notify(ce, lcname);
	return;
	} else {
	goto link_unbound;
	}
	} else if (!extends_ast) {
	link_unbound:
	/* Link unbound simple class */
	zend_build_properties_info_table(ce);
	zend_inheritance_check_override(ce);
	ce->ce_flags |= ZEND_ACC_LINKED;
	}
	}
	opline = get_next_op();
	if (ce->parent_name) {
	/* Lowercased parent name */
	zend_string *lc_parent_name = zend_string_tolower(ce->parent_name);
	opline->op2_type = IS_CONST;
	LITERAL_STR(opline->op2, lc_parent_name);
	}
	opline->op1_type = IS_CONST;
	/* It's possible that `lcname` is not an interned string because it was not yet in the interned string table.
	* However, by this point another thread may have caused `lcname` to be added in the interned string table.
	* This will cause `lcname` to get freed once it is found in the interned string table. If we were to use
	* LITERAL_STR() here we would not change the `lcname` pointer to the new value, and it would point to the
	* now-freed string. This will cause issues when we use `lcname` in the code below. We solve this by using
	* zend_add_literal_string() which gives us the new value. */
	opline->op1.constant = zend_add_literal_string(&lcname);
	if (decl->flags & ZEND_ACC_ANON_CLASS) {
	opline->opcode = ZEND_DECLARE_ANON_CLASS;
	opline->extended_value = zend_alloc_cache_slot();
	zend_make_var_result(result, opline);
	if (!zend_hash_add_ptr(CG(class_table), lcname, ce)) {
	/* We checked above that the class name is not used. This really shouldn't happen. */
	zend_error_noreturn(E_ERROR,
	"Runtime definition key collision for %s. This is a bug", ZSTR_VAL(name));
	}
	} else {
	/* Generate RTD keys until we find one that isn't in use yet. */
	zend_string *key = NULL;
	do {
	zend_tmp_string_release(key);
	key = zend_build_runtime_definition_key(lcname, decl->start_lineno);
	} while (!zend_hash_add_ptr(CG(class_table), key, ce));
	/* RTD key is placed after lcname literal in op1 */
	zend_add_literal_string(&key);
	opline->opcode = ZEND_DECLARE_CLASS;
	if (toplevel
	&& (CG(compiler_options) & ZEND_COMPILE_DELAYED_BINDING)
	/* We currently don't early-bind classes that implement interfaces or use traits */
	&& !ce->num_interfaces && !ce->num_traits && !ce->num_hooked_prop_variance_checks
	) {
	if (!extends_ast) {
	/* Use empty string for classes without parents to avoid new handler, and special
	* handling of zend_early_binding. */
	opline->op2_type = IS_CONST;
	LITERAL_STR(opline->op2, ZSTR_EMPTY_ALLOC());
	}
	CG(active_op_array)->fn_flags |= ZEND_ACC_EARLY_BINDING;
	opline->opcode = ZEND_DECLARE_CLASS_DELAYED;
	opline->extended_value = zend_alloc_cache_slot();
	opline->result_type = IS_UNUSED;
	opline->result.opline_num = -1;
	}
	}
	}

• zend_compile_enum_backing_type() sets ce->enum_backing_type but that is a uint32_t so no memory concerns
• zend_enum_add_interfaces() reallocates and handles existing interfaces, zend_enum_register_props() just declares new properties but zend_declare_typed_property() adds to the existing properties
• zend_compile_stmt() is going to reach zend_compile_stmt_list() list the class_statement_list rule in the language parser creates lists, and whatever inner statements are included are all going to already handle anything added before them since there is a list (e.g. when we compile a class function we don't erase the existing ones)
• zend_verify_abstract_class() doesn't modify the class other than potentially removing the ZEND_ACC_IMPLICIT_ABSTRACT_CLASS flag
• subsequent logic for early-binding is something that is always going to be the purview of the compiler and extensions shouldn't be messing with that stuff
• the rest just handles emitting the op codes

It is only compiling the interfaces that is problematic, because it is reasonable for extensions to want to add interfaces

It might be better to add a new attribute hook that triggers after the class is fully compiled, after which the attribute can do it's thing, and in this case handle the duplicate interface logic itself.

But the motivation in the custom cast extension was that the attribute would add the interface, and then the normal interface logic would ensure that the interface is properly implemented, https://github.com/DanielEScherzer/CustomCast/blob/d8fe4ab28d4621a0a7fb4e8ce22af035282519a6/custom_cast.c#L339

[iluuu1994]

I haven't deeply thought about internal attributes effectively doing metaprogramming, but it seems more useful for the attribute to trigger at the end of the class compilation so that it has a full view of the class. For example, who says the attribute might not want to inspect or even modify compiled methods?

However we solve this particular case, I don't think this should qualify as a bug fix. This is adding support for something new, rather than fixing something that is broken.

[iluuu1994]

But what if that's exactly intended for the attribute? E.g. enums don't allow manual implementation for UnitEnum and BackedEnum. A delayed validator could make this distinction explicitly.