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] Fix GetCustomAttributes System.Reflection API with a custom attribute provider #94602

Merged
merged 1 commit into from
Nov 13, 2023
Merged

[mono] Fix GetCustomAttributes System.Reflection API with a custom attribute provider #94602

merged 1 commit into from
Nov 13, 2023

Conversation

ivanpovazan
Copy link
Member

Description

This PR fixes issues when using GetCustomAttributes API with a custom attribute provider with Mono.

The problem comes from the fact that entry points of the GetCustomAttributes API in the Mono implementation, do not check early if the custom attribute provider is derived from the base type, but rather call back to the custom override after some problematic "workarounds" are done like:

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

Lines 159 to 165 in 952ac21

// FIXME: GetCustomAttributesBase doesn't like being passed a null attributeType
if (attributeType == typeof(CustomAttribute))
attributeType = null!;
if (attributeType == typeof(Attribute))
attributeType = null!;
if (attributeType == typeof(object))
attributeType = null!;

causing ArgumentNullException

Changes

This has been fixed by lifting up the check for the user-defined custom attribute provider and the call to the adequate overrides.

It seems that such usage is not covered with our unit tests, however it is used in ASP.NET test suite (reported here: #93770 and here: #94437 (comment))

To verify the fix and to increase our test coverage I have added a unit test in this PR inspired by: https://github.com/dotnet/aspnetcore/blob/402e7fc10aa304775f50d318c8a4dafbf46dbfe0/src/Shared/PropertyAsParameterInfo.cs#L15

NOTE: It is possible I did not place the unit test in the right location.

Fixes: #94488

@ghost ghost assigned ivanpovazan Nov 10, 2023
@ghost
Copy link

ghost commented Nov 10, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

This PR fixes issues when using GetCustomAttributes API with a custom attribute provider with Mono.

The problem comes from the fact that entry points of the GetCustomAttributes API in the Mono implementation, do not check early if the custom attribute provider is derived from the base type, but rather call back to the custom override after some problematic "workarounds" are done like:

runtime/src/mono/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

Lines 159 to 165 in 952ac21

// FIXME: GetCustomAttributesBase doesn't like being passed a null attributeType
if (attributeType == typeof(CustomAttribute))
attributeType = null!;
if (attributeType == typeof(Attribute))
attributeType = null!;
if (attributeType == typeof(object))
attributeType = null!;

causing ArgumentNullException

Changes

This has been fixed by lifting up the check for the user-defined custom attribute provider and the call to the adequate overrides.

It seems that such usage is not covered with our unit tests, however it is used in ASP.NET test suite (reported here: #93770 and here: #94437 (comment))

To verify the fix and to increase our test coverage I have added a unit test in this PR inspired by: https://github.com/dotnet/aspnetcore/blob/402e7fc10aa304775f50d318c8a4dafbf46dbfe0/src/Shared/PropertyAsParameterInfo.cs#L15

NOTE: It is possible I did not place the unit test in the right location.

Fixes: #94488

Author: ivanpovazan
Assignees: -
Labels:

area-System.Reflection
Milestone: -

@ivanpovazan ivanpovazan added area-VM-reflection-mono Reflection issues specific to MonoVM and removed area-System.Reflection labels Nov 10, 2023
ivanpovazan
ivanpovazan commented Nov 10, 2023
View reviewed changes
src/mono/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs
@@ -306,6 +303,9 @@ internal static object[] GetCustomAttributes(ICustomAttributeProvider obj, bool
{
ArgumentNullException.ThrowIfNull(obj);

if (IsUserCattrProvider(obj))
return obj.GetCustomAttributes(typeof(Attribute), 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.

I am passing in typeof(Attribute) here to match CoreCLR implementation:

runtime/src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs

Lines 555 to 567 in c07cb44

public static Attribute[] GetCustomAttributes(ParameterInfo element, bool inherit)
{
ArgumentNullException.ThrowIfNull(element);
if (element.Member == null)
throw new ArgumentException(SR.Argument_InvalidParameterInfo, nameof(element));
MemberInfo member = element.Member;
if (member.MemberType == MemberTypes.Method && inherit)
return InternalParamGetCustomAttributes(element, null, inherit);
return (Attribute[])element.GetCustomAttributes(typeof(Attribute), inherit);
}

@build-analysis build-analysis bot mentioned this pull request Nov 10, 2023
Closed
lambdageek
lambdageek approved these changes Nov 10, 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.

It would be nice for net9 to unify more of Mono and CoreCLR's custom attribute logic because the Mono code feels very messy right now

@ivanpovazan ivanpovazan mentioned this pull request Nov 13, 2023
Open
@ivanpovazan
Copy link
Member Author

It would be nice for net9 to unify more of Mono and CoreCLR's custom attribute logic because the Mono code feels very messy right now

I agree. Opened: #94659 for tracking.

@ivanpovazan
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Nov 13, 2023
Open
Closed
@ivanpovazan
Copy link
Member Author

Failures are not related.

@ivanpovazan ivanpovazan merged commit 0cfad56 into dotnet:main Nov 13, 2023
132 of 135 checks passed
@build-analysis build-analysis bot mentioned this pull request Nov 13, 2023
Closed
@ivanpovazan
Copy link
Member Author

/backport to release/6.0-staging

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/6.0-staging: https://github.com/dotnet/runtime/actions/runs/6865787900

@ivanpovazan
Copy link
Member Author

/backport to release/7.0-staging

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/6865796221

@ivanpovazan
Copy link
Member Author

/backport to release/8.0-staging

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6865801202

This was referenced Nov 14, 2023
Closed
Closed
Closed
@janani66
Copy link

It will be good to have this back ported at least for .NET 8

@github-actions github-actions bot locked and limited conversation to collaborators Jan 13, 2024
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 vargaz approved these changes

@tmds tmds Awaiting requested review from tmds

@buyaa-n buyaa-n Awaiting requested review from buyaa-n

@marek-safar marek-safar Awaiting requested review from marek-safar

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][reflection] ArgumentNullException calling GetCustomAttributes on user-defined subtype of ParameterInfo
4 participants
@ivanpovazan @janani66 @vargaz @lambdageek