From d2e3ebfc44ac578d3655b0db955407f44cef4f4a Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 7 Jul 2024 21:10:53 +0300 Subject: [PATCH 1/7] Ignore type resolution failures when filtering custom attributes. --- .../Reflection/RuntimeCustomAttributeData.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs index 468f71204421f0..01cb2a4a552f39 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs @@ -1635,15 +1635,24 @@ private static bool FilterCustomAttributeRecord( RuntimeType attributeFilterType, bool mustBeInheritable, ref RuntimeType.ListBuilder derivedAttributes, - out RuntimeType attributeType, + [MaybeNullWhen(false)] out RuntimeType attributeType, out IRuntimeMethodInfo? ctorWithParameters, out bool isVarArg) { ctorWithParameters = null; isVarArg = false; - // Resolve attribute type from ctor parent token found in decorated decoratedModule scope - attributeType = (decoratedModule.ResolveType(scope.GetParentToken(caCtorToken), null, null) as RuntimeType)!; + try + { + // Resolve attribute type from ctor parent token found in decorated decoratedModule scope + attributeType = (decoratedModule.ResolveType(scope.GetParentToken(caCtorToken), null, null) as RuntimeType)!; + } + catch + { + // Skip attributes whose type cannot be resolved. + attributeType = null; + return false; + } // Test attribute type against user provided attribute type filter if (!MatchesTypeFilter(attributeType, attributeFilterType)) From c401f48e1969d1736f5146e0b18ccf73e95d364e Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Sun, 7 Jul 2024 22:41:59 +0300 Subject: [PATCH 2/7] Ignore errors only when looking for sealed attribute types. --- .../src/System/Reflection/RuntimeCustomAttributeData.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs index 01cb2a4a552f39..394dd93c78243f 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs @@ -1647,9 +1647,12 @@ private static bool FilterCustomAttributeRecord( // Resolve attribute type from ctor parent token found in decorated decoratedModule scope attributeType = (decoratedModule.ResolveType(scope.GetParentToken(caCtorToken), null, null) as RuntimeType)!; } - catch + catch when (attributeFilterType.IsSealed) { - // Skip attributes whose type cannot be resolved. + // If the type of one of the attributes cannot be resolved and the filter type is sealed, we can ignore + // the error and return false. We can do this only when looking for sealed attribute types because otherwise, + // that type that failed to be resolved could have been a subclass of the filter type, and by ignoring the + // error we are potentially returning false information. attributeType = null; return false; } From 33f29fba65b1aaa26394bbb2a5255b18517604dc Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Mon, 8 Jul 2024 00:31:13 +0300 Subject: [PATCH 3/7] Try to fix nullability annotations. --- .../src/System/Reflection/RuntimeCustomAttributeData.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs index 394dd93c78243f..b73a27d49c36a7 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs @@ -1516,7 +1516,7 @@ private static void AddCustomAttributes( if (!FilterCustomAttributeRecord(caRecord.tkCtor, in scope, decoratedModule, decoratedMetadataToken, attributeFilterType!, mustBeInheritable, ref derivedAttributes, - out RuntimeType attributeType, out IRuntimeMethodInfo? ctorWithParameters, out bool isVarArg)) + out RuntimeType? attributeType, out IRuntimeMethodInfo? ctorWithParameters, out bool isVarArg)) { continue; } @@ -1635,7 +1635,7 @@ private static bool FilterCustomAttributeRecord( RuntimeType attributeFilterType, bool mustBeInheritable, ref RuntimeType.ListBuilder derivedAttributes, - [MaybeNullWhen(false)] out RuntimeType attributeType, + [NotNullWhen(true)] out RuntimeType? attributeType, out IRuntimeMethodInfo? ctorWithParameters, out bool isVarArg) { From 9dcaa011f0b09d44e9f8d800f58ac9c77c9991a5 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 9 Jul 2024 03:48:50 +0300 Subject: [PATCH 4/7] Revert existing changes. --- .../Reflection/RuntimeCustomAttributeData.cs | 20 ++++--------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs index b73a27d49c36a7..468f71204421f0 100644 --- a/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs +++ b/src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs @@ -1516,7 +1516,7 @@ private static void AddCustomAttributes( if (!FilterCustomAttributeRecord(caRecord.tkCtor, in scope, decoratedModule, decoratedMetadataToken, attributeFilterType!, mustBeInheritable, ref derivedAttributes, - out RuntimeType? attributeType, out IRuntimeMethodInfo? ctorWithParameters, out bool isVarArg)) + out RuntimeType attributeType, out IRuntimeMethodInfo? ctorWithParameters, out bool isVarArg)) { continue; } @@ -1635,27 +1635,15 @@ private static bool FilterCustomAttributeRecord( RuntimeType attributeFilterType, bool mustBeInheritable, ref RuntimeType.ListBuilder derivedAttributes, - [NotNullWhen(true)] out RuntimeType? attributeType, + out RuntimeType attributeType, out IRuntimeMethodInfo? ctorWithParameters, out bool isVarArg) { ctorWithParameters = null; isVarArg = false; - try - { - // Resolve attribute type from ctor parent token found in decorated decoratedModule scope - attributeType = (decoratedModule.ResolveType(scope.GetParentToken(caCtorToken), null, null) as RuntimeType)!; - } - catch when (attributeFilterType.IsSealed) - { - // If the type of one of the attributes cannot be resolved and the filter type is sealed, we can ignore - // the error and return false. We can do this only when looking for sealed attribute types because otherwise, - // that type that failed to be resolved could have been a subclass of the filter type, and by ignoring the - // error we are potentially returning false information. - attributeType = null; - return false; - } + // Resolve attribute type from ctor parent token found in decorated decoratedModule scope + attributeType = (decoratedModule.ResolveType(scope.GetParentToken(caCtorToken), null, null) as RuntimeType)!; // Test attribute type against user provided attribute type filter if (!MatchesTypeFilter(attributeType, attributeFilterType)) From e9c0605083a0ff7988ec156b2d86e03a5d0e0a75 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 9 Jul 2024 03:37:38 +0300 Subject: [PATCH 5/7] Guard the call to `IsDefined(typeof(CompilerGeneratedAttribute))` from throwing. An `IsDefinedSafe` method was added that returns false if checking for the attribute throws, and other usages in the file were updated as well. This also allows us to remove a big try catch block. --- .../src/System/Diagnostics/StackTrace.cs | 46 +++++++++++-------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs index 5c5f8d374cc23b..f4b0ac38b5f6ec 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs @@ -240,7 +240,7 @@ internal void ToString(TraceFormat traceFormat, StringBuilder sb) Type? declaringType = mb.DeclaringType; string methodName = mb.Name; bool methodChanged = false; - if (declaringType != null && declaringType.IsDefined(typeof(CompilerGeneratedAttribute), inherit: false)) + if (declaringType != null && IsDefinedSafe(declaringType, typeof(CompilerGeneratedAttribute), inherit: false)) { isAsync = declaringType.IsAssignableTo(typeof(IAsyncStateMachine)); if (isAsync || declaringType.IsAssignableTo(typeof(IEnumerator))) @@ -384,33 +384,39 @@ private static bool ShowInStackTrace(MethodBase mb) return false; } - try + if (IsDefinedSafe(mb, typeof(StackTraceHiddenAttribute), inherit: false)) { - if (mb.IsDefined(typeof(StackTraceHiddenAttribute), inherit: false)) - { - // Don't show where StackTraceHidden is applied to the method. - return false; - } - - Type? declaringType = mb.DeclaringType; - // Methods don't always have containing types, for example dynamic RefEmit generated methods. - if (declaringType != null && - declaringType.IsDefined(typeof(StackTraceHiddenAttribute), inherit: false)) - { - // Don't show where StackTraceHidden is applied to the containing Type of the method. - return false; - } + // Don't show where StackTraceHidden is applied to the method. + return false; } - catch + + Type? declaringType = mb.DeclaringType; + // Methods don't always have containing types, for example dynamic RefEmit generated methods. + if (declaringType != null && + IsDefinedSafe(declaringType, typeof(StackTraceHiddenAttribute), inherit: false)) { - // Getting the StackTraceHiddenAttribute has failed, behave as if it was not present. - // One of the reasons can be that the method mb or its declaring type use attributes - // defined in an assembly that is missing. + // Don't show where StackTraceHidden is applied to the containing Type of the method. + return false; } return true; } + private static bool IsDefinedSafe(MemberInfo memberInfo, Type attributeType bool inherit) + { + try + { + return memberInfo.IsDefined(attributeType, inherit); + } + catch + { + // Checking for the attribute has failed, behave as if it was not present. One of + // the reasons can be that the member has attributes defined in an assembly that + // is missing. + return false; + } + } + private static bool TryResolveStateMachineMethod(ref MethodBase method, out Type declaringType) { Debug.Assert(method != null); From 2afa59f8be3a3fd9c8eb0051a30fc747c81132d4 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 9 Jul 2024 03:47:05 +0300 Subject: [PATCH 6/7] Guard getting the `StateMachineAttribute`s from throwing. --- .../src/System/Diagnostics/StackTrace.cs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs index f4b0ac38b5f6ec..0c703bfe628cc0 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs @@ -417,6 +417,20 @@ private static bool IsDefinedSafe(MemberInfo memberInfo, Type attributeType bool } } + private static Attribute[] GetCustomAttributesSafe(MemberInfo memberInfo, Type attributeType, bool inherit) + { + try + { + return Attribute.GetCustomAttributes(memberInfo, attributeType, inherit); + } + catch + { + // Getting the attributes has failed, return an empty array. One of the reasons + // can be that the member has attributes defined in an assembly that is missing. + return []; + } + } + private static bool TryResolveStateMachineMethod(ref MethodBase method, out Type declaringType) { Debug.Assert(method != null); @@ -444,7 +458,7 @@ private static bool TryResolveStateMachineMethod(ref MethodBase method, out Type foreach (MethodInfo candidateMethod in methods) { - StateMachineAttribute[]? attributes = (StateMachineAttribute[])Attribute.GetCustomAttributes(candidateMethod, typeof(StateMachineAttribute), inherit: false); + StateMachineAttribute[]? attributes = (StateMachineAttribute[])GetCustomAttributesSafe(candidateMethod, typeof(StateMachineAttribute), inherit: false); if (attributes == null) { continue; From 9dff755a4f1ff880977ac1a80107bf3fe0a97f84 Mon Sep 17 00:00:00 2001 From: Theodore Tsirpanis Date: Tue, 9 Jul 2024 13:29:40 +0300 Subject: [PATCH 7/7] Fix typo. --- .../System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs index 0c703bfe628cc0..8def9abf8a7e0a 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs @@ -402,7 +402,7 @@ private static bool ShowInStackTrace(MethodBase mb) return true; } - private static bool IsDefinedSafe(MemberInfo memberInfo, Type attributeType bool inherit) + private static bool IsDefinedSafe(MemberInfo memberInfo, Type attributeType, bool inherit) { try {