Skip to content

System.Reflection.CustomAttributeFormatException thrown on a retrieval of derived attribute with overridden property getter #117584

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -1573,14 +1573,39 @@ private static void AddCustomAttributes(
}
}

RuntimePropertyInfo? property = (RuntimePropertyInfo?)(type is null ?
attributeType.GetProperty(name) :
attributeType.GetProperty(name, type, Type.EmptyTypes)) ??
RuntimeMethodInfo? setMethod = null;
RuntimeMethodInfo? getMethod = null;
RuntimePropertyInfo? property = null;
Type? baseAttributeType = attributeType;

while (setMethod is null && baseAttributeType is not null
&& (property is null || getMethod is null || getMethod.IsVirtual))
{
property = (RuntimePropertyInfo?)(type is null ?
baseAttributeType.GetProperty(name) :
baseAttributeType.GetProperty(name, type, Type.EmptyTypes));

if (property is not null)
{
// Public properties may have non-public setter methods
setMethod = property.GetSetMethod(true)!;
getMethod = property.GetGetMethod(true)!;
}
else
{
setMethod = null;
getMethod = null;
}

baseAttributeType = baseAttributeType.BaseType;
}

if (property is null)
{
throw new CustomAttributeFormatException(SR.Format(SR.RFLCT_InvalidPropFail, name));
RuntimeMethodInfo setMethod = property.GetSetMethod(true)!;
}

// Public properties may have non-public setter methods
if (!setMethod.IsPublic)
if (setMethod is null || !setMethod.IsPublic)
{
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public static Attribute Instantiate(this CustomAttributeData cad)
for (; ; )
{
PropertyInfo? propertyInfo = walk.GetProperty(name, BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly);
if (propertyInfo != null)
if (propertyInfo?.SetMethod is not null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this logic equivalent to the others, or just equivalent enough to pass the test? What corner cases are we missing in the test that necessitate the more complex logic elsewhere?

Copy link
Contributor Author

@pedrobsaila pedrobsaila Jul 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was going to be off for a few days so I did rapidely just enough to pass the test and not let the PR hanging, I fixed it now. The missing condition stops the lookup in base classes early if someone used the new keyword to hide the property instead of overriding it.

{
propertyInfo.SetValue(newAttribute, argumentValue);
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -437,4 +437,41 @@ public void MyMethod3(int x, int y, double z) { }
public void mymethod4(string x) { }
}
}
public static class InheritGetterAndSetterMethodInProperty
{
[Fact]
public static void GetterAndSetterAreAvailableOnSubTypeWhenOverridingOneButInheritingBothFromBaseClass()
{
object[] attributes = typeof(ClassWithDerivedAttr).GetCustomAttributes(true);
object attribute = Assert.Single(attributes);
var derivedAttributeWithGetterAttr = Assert.IsType<DerivedAttributeWithGetter>(attribute);
Assert.Equal(2, derivedAttributeWithGetterAttr.P);
}

public class BaseAttributeWithGetterSetter : Attribute
{
protected int _p;

public virtual int P
{
get => _p;
set
{
_p = value;
}
}
}

public class DerivedAttributeWithGetter : BaseAttributeWithGetterSetter
{
public override int P
{
get => _p;
}
}

[DerivedAttributeWithGetter(P = 2)]
public class ClassWithDerivedAttr
{ }
}
}
28 changes: 24 additions & 4 deletions src/mono/mono/metadata/custom-attrs.c
Original file line number Diff line number Diff line change
Expand Up @@ -1245,21 +1245,41 @@ create_custom_attr (MonoImage *image, MonoMethod *method, const guchar *data, gu

mono_field_set_value_internal (MONO_HANDLE_RAW (attr), field, val); // FIXMEcoop
} else if (named_type == CATTR_TYPE_PROPERTY) {
MonoProperty *prop;
prop = mono_class_get_property_from_name_internal (mono_handle_class (attr), name);
MonoProperty *prop = NULL;
MonoMethod *setMethod = NULL;
MonoMethod *getMethod = NULL;
MonoClass *attrBaseClass = mono_handle_class (attr);
while (!setMethod && attrBaseClass && (!prop || !getMethod || m_method_is_virtual(getMethod)))
{
prop = mono_class_get_property_from_name_internal (attrBaseClass, name);

if (prop)
{
setMethod = prop->set;
getMethod = prop->get;
}
else
{
setMethod = NULL;
getMethod = NULL;
}

attrBaseClass = m_class_get_parent(attrBaseClass);
}

if (!prop) {
mono_error_set_generic_error (error, "System.Reflection", "CustomAttributeFormatException", "Could not find a property with name %s", name);
goto fail;
}

if (!prop->set) {
if (!setMethod) {
mono_error_set_generic_error (error, "System.Reflection", "CustomAttributeFormatException", "Could not find the setter for %s", name);
goto fail;
}

/* can we have more that 1 arg in a custom attr named property? */
prop_type = prop->get? mono_method_signature_internal (prop->get)->ret :
mono_method_signature_internal (prop->set)->params [mono_method_signature_internal (prop->set)->param_count - 1];
mono_method_signature_internal (setMethod)->params [mono_method_signature_internal (setMethod)->param_count - 1];

MonoObject *param_obj;
pparams [0] = load_cattr_value (image, prop_type, &param_obj, named, data_end, &named, error);
Expand Down
Loading