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

ivanpovazan · 2023-06-12T15:53:46Z

This PR:

fixes the way mono determines custom attribute visibility that are applied on dynamic assemblies
adds unit tests to verify the behaviour

…ough SRE

ivanpovazan · 2023-06-12T19:14:35Z

@lambdageek looking at the test failures with coreCLR, it seems like explicitly setting an internal attribute defined in one dynamic assembly on another dynamic assembly: https://github.com/dotnet/runtime/pull/87406/files#diff-fd74227f1b0f0310d5980748bee0599f868713d789a4ddf80c9cc9eb7f86c2b3R313 works - as if custom attribute visibility is not taken into account.
Is this expected behaviour?

lambdageek · 2023-06-13T14:18:34Z

@lambdageek looking at the test failures with coreCLR, it seems like explicitly setting an internal attribute defined in one dynamic assembly on another dynamic assembly: https://github.com/dotnet/runtime/pull/87406/files#diff-fd74227f1b0f0310d5980748bee0599f868713d789a4ddf80c9cc9eb7f86c2b3R313 works - as if custom attribute visibility is not taken into account. Is this expected behaviour?

I'm not sure. @steveharter @buyaa-n Do you know if the CoreCLR behavior here is intentional or a bug: When there are two dynamic assemblies and one of them defines a non-public custom attribute that is then applied in the other assembly, a subsequent call to GetCustomAttributes will see the non-public attribute?

steveharter · 2023-06-16T17:57:57Z

I'm not sure. @steveharter @buyaa-n Do you know if the CoreCLR behavior here is intentional or a bug: When there are two dynamic assemblies and one of them defines a non-public custom attribute that is then applied in the other assembly, a subsequent call to GetCustomAttributes will see the non-public attribute?

From the test failures in Core, GetCustomAttributeData() returns non-empty, but a bit of additional testing locally shows GetCustomAttributes() returns empty. A visibility check is done by calling IsCAVisibleFromDecoratedType.

…mblyBuilder

ivanpovazan · 2023-06-21T09:59:44Z

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

-        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);


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:

https://github.com/dotnet/runtime/pull/87406/files#diff-666a23992bc09b4499c853602019f69bdad7f3dc4cf247a32a5e40a2d76616d1R77

https://github.com/dotnet/runtime/pull/87406/files#diff-666a23992bc09b4499c853602019f69bdad7f3dc4cf247a32a5e40a2d76616d1R110

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]);

src/mono/mono/metadata/custom-attrs.c

lambdageek

thanks Ivan!

ivanpovazan · 2023-06-21T16:15:22Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-06-21T16:15:46Z

Azure Pipelines successfully started running 1 pipeline(s).

ivanpovazan · 2023-06-22T13:43:34Z

Failures seem unrelated.

wip: Properly determine custom attribute visibility when applying thr…

4a3e59e

…ough SRE

ivanpovazan added the area-VM-reflection-mono Reflection issues specific to MonoVM label Jun 12, 2023

ivanpovazan self-assigned this Jun 12, 2023

This was referenced Jun 12, 2023

Tracking issue for CI build timeouts #76454

Closed

WasmTemplateTests.ConsolePublishAndRun fails with exit code 137 #87414

Closed

ivanpovazan added 3 commits June 21, 2023 11:15

Properly acknowledge custom attribute visibility

90f6dad

Adjust the added unit tests to match expected behaviour

266d895

Share GetCustomAttribute between RuntimeModuleBuilder and RuntimeAsse…

7fe0084

…mblyBuilder

ivanpovazan commented Jun 21, 2023

View reviewed changes

ivanpovazan changed the title ~~WIP: [mono] Do not ignore non-public custom attributes in dynamic assemblies~~ [mono] Do not ignore non-public custom attributes in dynamic assemblies Jun 21, 2023

ivanpovazan marked this pull request as ready for review June 21, 2023 10:01

ivanpovazan requested review from vargaz, lambdageek, thaystg and marek-safar as code owners June 21, 2023 10:01

ivanpovazan commented Jun 21, 2023

View reviewed changes

src/mono/mono/metadata/custom-attrs.c Outdated Show resolved Hide resolved

lambdageek approved these changes Jun 21, 2023

View reviewed changes

Cleaning up outdated comments

03cf8c9

ivanpovazan merged commit 4586587 into dotnet:main Jun 22, 2023

ivanpovazan mentioned this pull request Jun 22, 2023

Not able to skip aspnetcore testcases with Mono runtime dotnet/aspnetcore#48688

Closed

1 task

ivanpovazan deleted the sre-fix-internal-cattr branch July 3, 2023 10:30

ivanpovazan mentioned this pull request Jul 3, 2023

[mono] Reenable disabled tests as mono now respects non-public custom attributes in dynamic assemblies #88322

Merged

ghost locked as resolved and limited conversation to collaborators Aug 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ivanpovazan commented Jun 12, 2023 •

edited

Loading

ivanpovazan commented Jun 12, 2023

lambdageek commented Jun 13, 2023

steveharter commented Jun 16, 2023

ivanpovazan Jun 21, 2023

lambdageek left a comment

ivanpovazan commented Jun 21, 2023

azure-pipelines bot commented Jun 21, 2023

ivanpovazan commented Jun 22, 2023

	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]);

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

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

Conversation

ivanpovazan commented Jun 12, 2023 • edited Loading

ivanpovazan commented Jun 12, 2023

lambdageek commented Jun 13, 2023

steveharter commented Jun 16, 2023

ivanpovazan Jun 21, 2023

Choose a reason for hiding this comment

lambdageek left a comment

Choose a reason for hiding this comment

ivanpovazan commented Jun 21, 2023

azure-pipelines bot commented Jun 21, 2023

ivanpovazan commented Jun 22, 2023

ivanpovazan commented Jun 12, 2023 •

edited

Loading