Skip to content

[mono] Do not ignore non-public custom attributes in dynamic assemblies #87406

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jun 22, 2023
Merged

[mono] Do not ignore non-public custom attributes in dynamic assemblies #87406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a3e59e
wip: Properly determine custom attribute visibility when applying thr…
ivanpovazan Jun 12, 2023
90f6dad
Properly acknowledge custom attribute visibility
ivanpovazan Jun 21, 2023
266d895
Adjust the added unit tests to match expected behaviour
ivanpovazan Jun 20, 2023
7fe0084
Share GetCustomAttribute between RuntimeModuleBuilder and RuntimeAss…
ivanpovazan Jun 21, 2023
03cf8c9
Cleaning up outdated comments
ivanpovazan Jun 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 70 additions & 0 deletions src/libraries/System.Reflection.Emit/tests/AssemblyBuilderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,76 @@ public void SetCustomAttribute_CustomAttributeBuilder_NullAttributeBuilder_Throw
AssertExtensions.Throws<ArgumentNullException>("customBuilder", () => assembly.SetCustomAttribute(null));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributesData_NonPublicVisibility_DefinedInternally()
{
AssemblyBuilder assembly = Helpers.DynamicAssembly();
ModuleBuilder module = assembly.DefineDynamicModule("DynamicModule");

TypeBuilder internalAttributeType = module.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

assembly.SetCustomAttribute(customAttribute);
Assert.Single(assembly.GetCustomAttributesData());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributes_NonPublicVisibility_DefinedInternally()
{
AssemblyBuilder assembly = Helpers.DynamicAssembly();
ModuleBuilder module = assembly.DefineDynamicModule("DynamicModule");

TypeBuilder internalAttributeType = module.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

assembly.SetCustomAttribute(customAttribute);
Assert.Single(assembly.GetCustomAttributes(false));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributesData_NonPublicVisibility_DefinedExternally()
{
AssemblyBuilder assembly1 = Helpers.DynamicAssembly();
ModuleBuilder module1 = assembly1.DefineDynamicModule("DynamicModule");
AssemblyBuilder assembly2 = Helpers.DynamicAssembly("AnotherTestAssembly");

TypeBuilder internalAttributeType = module1.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

assembly2.SetCustomAttribute(customAttribute);
Assert.Single(assembly2.GetCustomAttributesData());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributes_NonPublicVisibility_DefinedExternally()
{
AssemblyBuilder assembly1 = Helpers.DynamicAssembly();
ModuleBuilder module1 = assembly1.DefineDynamicModule("DynamicModule");
AssemblyBuilder assembly2 = Helpers.DynamicAssembly("AnotherTestAssembly");

TypeBuilder internalAttributeType = module1.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

assembly2.SetCustomAttribute(customAttribute);
Assert.Empty(assembly2.GetCustomAttributes(false));
}

public static IEnumerable<object[]> Equals_TestData()
{
AssemblyBuilder assembly = Helpers.DynamicAssembly(name: "Name1");
Expand Down
66 changes: 66 additions & 0 deletions src/libraries/System.Reflection.Emit/tests/ModuleBuilder/ModuleBuilderSetCustomAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,5 +56,71 @@ public void SetCustomAttribute_CustomAttributeBuilder_NullBuilder_ThrowsArgument
ModuleBuilder module = Helpers.DynamicModule();
AssertExtensions.Throws<ArgumentNullException>("customBuilder", () => module.SetCustomAttribute(null));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributesData_NonPublicVisibility_DefinedInternally()
{
ModuleBuilder module = Helpers.DynamicModule();

TypeBuilder internalAttributeType = module.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

module.SetCustomAttribute(customAttribute);
Assert.Single(module.GetCustomAttributesData());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributes_NonPublicVisibility_DefinedInternally()
{
ModuleBuilder module = Helpers.DynamicModule();

TypeBuilder internalAttributeType = module.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

module.SetCustomAttribute(customAttribute);
Assert.Single(module.GetCustomAttributes(false));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributesData_NonPublicVisibility_DefinedExternally()
{
ModuleBuilder module1 = Helpers.DynamicModule();
ModuleBuilder module2 = Helpers.DynamicModule("AnotherTestAssembly", "AnotherTestModule");

TypeBuilder internalAttributeType = module1.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

module2.SetCustomAttribute(customAttribute);
Assert.Single(module2.GetCustomAttributesData());
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsReflectionEmitSupported))]
public void SetCustomAttribute_GetCustomAttributes_NonPublicVisibility_DefinedExternally()
{
ModuleBuilder module1 = Helpers.DynamicModule();
ModuleBuilder module2 = Helpers.DynamicModule("AnotherTestAssembly", "AnotherTestModule");

TypeBuilder internalAttributeType = module1.DefineType("DynamicInternalAttribute", TypeAttributes.NotPublic, typeof(Attribute));
ConstructorBuilder internalAttributeCtor = internalAttributeType.DefineConstructor(MethodAttributes.Public, CallingConventions.Standard, new Type[0]);
internalAttributeCtor.GetILGenerator().Emit(OpCodes.Ret);

ConstructorInfo internalAttributeCtorInfo = internalAttributeType.CreateTypeInfo().GetConstructor(Array.Empty<Type>());
CustomAttributeBuilder customAttribute = new CustomAttributeBuilder(internalAttributeCtorInfo, Array.Empty<object>());

module2.SetCustomAttribute(customAttribute);
Assert.Empty(module2.GetCustomAttributes(false));
}
}
}
29 changes: 3 additions & 26 deletions src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeModuleBuilder.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,33 +854,10 @@ public override bool IsDefined(Type attributeType, bool inherit)
return base.IsDefined(attributeType, inherit);
}

public override object[] GetCustomAttributes(bool inherit)
{
return GetCustomAttributes(null!, inherit); // FIXME: coreclr doesn't allow null attributeType
}

public override object[] GetCustomAttributes(Type attributeType, bool inherit)
{
if (cattrs == null || cattrs.Length == 0)
return Array.Empty<object>();

if (attributeType is TypeBuilder)
throw new InvalidOperationException(SR.InvalidOperation_CannotHaveFirstArgumentAsTypeBuilder);
public override object[] GetCustomAttributes(bool inherit) => CustomAttribute.GetCustomAttributes(this, inherit);

List<object> results = new List<object>();
for (int i = 0; i < cattrs.Length; i++)
{
Type t = cattrs[i].Ctor.GetType();

if (t is TypeBuilder)
throw new InvalidOperationException(SR.InvalidOperation_CannotConstructCustomAttributeForTypeBuilderType);

if (attributeType == null || attributeType.IsAssignableFrom(t))
results.Add(cattrs[i].Invoke());
}

return results.ToArray();
}
public override object[] GetCustomAttributes(Type attributeType, bool inherit) =>
CustomAttribute.GetCustomAttributes(this, attributeType, inherit);
Comment on lines -857 to +860
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are aligning how GetCustomAttributes is implemented for RuntimeAssemblyBuilder.Mono and RuntimeModuleBuilder.Mono by now sharing the same logic.

This is required as by adding the test cases:

a System.NullReferenceException bug was discovered in the old implementation.

The problem was coming from the fact that when invoking SetCustomAttributes on the RuntimeModuleBuilder.Mono:

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeModuleBuilder.Mono.cs

Line 429 in 1600677

protected override void SetCustomAttributeCore(ConstructorInfo con, ReadOnlySpan<byte> binaryAttribute)

The code inits the custom attribute builders without initializing their namedFields and namedProperties fields, this later fails when calling GetCustomAttributes on a module builder and invoking custom attribute builder via: https://github.com/dotnet/runtime/pull/87406/files#diff-ccf091eb73b1e9e47985dd5763e288a55cdb682be2ae8a1f394c004d37e7bd17L879 as the Invoke expects that those fields are adequately set:

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/Emit/CustomAttributeBuilder.Mono.cs

Lines 71 to 75 in 1600677

for (int i = 0; i < namedFields.Length; i++)
namedFields[i].SetValue(result, fieldValues[i]);
for (int i = 0; i < namedProperties.Length; i++)
namedProperties[i].SetValue(result, propertyValues[i]);


public override IList<CustomAttributeData> GetCustomAttributesData()
{
Expand Down
2 changes: 1 addition & 1 deletion src/mono/mono/metadata/custom-attrs-internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ typedef struct _MonoDecodeCustomAttr {
} MonoDecodeCustomAttr;

MonoCustomAttrInfo*
mono_custom_attrs_from_builders (MonoImage *alloc_img, MonoImage *image, MonoArray *cattrs);
mono_custom_attrs_from_builders (MonoImage *alloc_img, MonoImage *image, MonoArray *cattrs, gboolean respect_cattr_visibility);

typedef gboolean (*MonoAssemblyMetadataCustomAttrIterFunc) (MonoImage *image, guint32 typeref_scope_token, const gchar* nspace, const gchar* name, guint32 method_token, guint32 *cols, gpointer user_data);

Expand Down
Loading