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

ivanpovazan · 2023-11-10T12:55:43Z

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 · 2023-11-10T12:56:03Z

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 · 2023-11-10T12:59:07Z

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

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

}

lambdageek

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 · 2023-11-13T11:14:21Z

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 · 2023-11-13T13:10:35Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-11-13T13:10:58Z

Azure Pipelines successfully started running 1 pipeline(s).

ivanpovazan · 2023-11-13T22:29:09Z

Failures are not related.

ivanpovazan · 2023-11-14T15:27:08Z

/backport to release/6.0-staging

github-actions · 2023-11-14T15:27:23Z

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

ivanpovazan · 2023-11-14T15:27:46Z

/backport to release/7.0-staging

github-actions · 2023-11-14T15:27:59Z

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

ivanpovazan · 2023-11-14T15:28:11Z

/backport to release/8.0-staging

github-actions · 2023-11-14T15:28:24Z

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

janani66 · 2023-12-13T21:37:01Z

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

ivanpovazan requested review from buyaa-n, lambdageek and tmds November 10, 2023 12:55

ivanpovazan requested a review from marek-safar as a code owner November 10, 2023 12:55

ghost added the area-System.Reflection label Nov 10, 2023

ghost assigned ivanpovazan Nov 10, 2023

ivanpovazan added area-VM-reflection-mono Reflection issues specific to MonoVM and removed area-System.Reflection labels Nov 10, 2023

ivanpovazan commented Nov 10, 2023

View reviewed changes

build-analysis bot mentioned this pull request Nov 10, 2023

[wasm] Runtime tests' build gets terminated with exited with code 137 #94077

Closed

lambdageek approved these changes Nov 10, 2023

View reviewed changes

vargaz approved these changes Nov 12, 2023

View reviewed changes

ivanpovazan mentioned this pull request Nov 13, 2023

[mono] Share more of the CustomAttribute logic between CoreCLR and Mono #94659

Open

This was referenced Nov 13, 2023

[6.0][Android][mono] XHarness exit code 78 PACKAGE_INSTALLATION_FAILURE #84174

Closed

[mono] Tests failing with Failed to find compatible device: arm64-v8a #87267

Closed

[mono] Fix GetCustomAttributes API with a custom attribute provider

fe178fa

ivanpovazan force-pushed the fix-refl-custom-param-info branch from ad04942 to fe178fa Compare November 13, 2023 20:10

ivanpovazan merged commit 0cfad56 into dotnet:main Nov 13, 2023

build-analysis bot mentioned this pull request Nov 13, 2023

[wasm] JSImportExportTest.JsImportTaskTypes - System.ObjectDisposedException #93347

Closed

github-actions bot locked and limited conversation to collaborators Jan 13, 2024

	// 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!;

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

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

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

Uh oh!

Conversation

ivanpovazan commented Nov 10, 2023

Description

Changes

Uh oh!

ghost commented Nov 10, 2023

Description

Changes

Uh oh!

ivanpovazan Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

lambdageek left a comment

Choose a reason for hiding this comment

Uh oh!

ivanpovazan commented Nov 13, 2023

Uh oh!

ivanpovazan commented Nov 13, 2023

Uh oh!

azure-pipelines bot commented Nov 13, 2023

Uh oh!

ivanpovazan commented Nov 13, 2023

Uh oh!

ivanpovazan commented Nov 14, 2023

Uh oh!

github-actions bot commented Nov 14, 2023

Uh oh!

ivanpovazan commented Nov 14, 2023

Uh oh!

github-actions bot commented Nov 14, 2023

Uh oh!

ivanpovazan commented Nov 14, 2023

Uh oh!

github-actions bot commented Nov 14, 2023

Uh oh!

janani66 commented Dec 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

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