Skip to content
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

Jump to bottom

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

Merged
merged 5 commits into from
Jun 22, 2023
Merged

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

merged 5 commits into from
Jun 22, 2023

Conversation

ivanpovazan
Copy link
Member

@ivanpovazan ivanpovazan commented Jun 12, 2023 •
edited
Loading

This PR:

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

Fixes #60650

@ivanpovazan ivanpovazan added the area-VM-reflection-mono Reflection issues specific to MonoVM label Jun 12, 2023
@ivanpovazan ivanpovazan self-assigned this Jun 12, 2023
This was referenced Jun 12, 2023
Closed
Closed
@ivanpovazan
Copy link
Member Author

@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
Copy link
Member

@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
Copy link
Member

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.

ivanpovazan
ivanpovazan commented Jun 21, 2023
View reviewed changes
src/mono/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeModuleBuilder.Mono.cs
Comment on lines -857 to +860
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);
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]);

@ivanpovazan 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 ivanpovazan marked this pull request as ready for review June 21, 2023 10:01
lambdageek
lambdageek approved these changes Jun 21, 2023
View reviewed changes
Copy link
Member

@lambdageek lambdageek left a comment

Choose a reason for hiding this comment

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

thanks Ivan!

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Jun 21, 2023
Open
Closed
Closed
Closed
@ivanpovazan
Copy link
Member Author

Failures seem unrelated.

@ivanpovazan ivanpovazan merged commit 4586587 into dotnet:main Jun 22, 2023
@ivanpovazan ivanpovazan mentioned this pull request Jun 22, 2023
Closed
1 task
@ivanpovazan ivanpovazan deleted the sre-fix-internal-cattr branch July 3, 2023 10:30
@ivanpovazan ivanpovazan mentioned this pull request Jul 3, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lambdageek lambdageek lambdageek approved these changes

@vargaz vargaz Awaiting requested review from vargaz

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees

@ivanpovazan ivanpovazan

Labels
area-VM-reflection-mono Reflection issues specific to MonoVM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mono ignores non-public custom attributes in dynamic assemblies
3 participants
@ivanpovazan @lambdageek @steveharter