diff --git a/src/linker/Linker.Dataflow/MethodBodyScanner.cs b/src/linker/Linker.Dataflow/MethodBodyScanner.cs index 1ffce3d5373a..fa8ca8dd19fa 100644 --- a/src/linker/Linker.Dataflow/MethodBodyScanner.cs +++ b/src/linker/Linker.Dataflow/MethodBodyScanner.cs @@ -526,13 +526,13 @@ public void Scan (MethodBody methodBody) } // Pop arguments - PopUnknown (currentStack, signature.Parameters.Count, methodBody, operation.Offset); + if (signature.Parameters.Count > 0) + PopUnknown (currentStack, signature.Parameters.Count, methodBody, operation.Offset); // Pop function pointer PopUnknown (currentStack, 1, methodBody, operation.Offset); - // Push return value - if (signature.ReturnType.MetadataType != MetadataType.Void) + if (GetReturnTypeWithoutModifiers (signature.ReturnType).MetadataType != MetadataType.Void) PushUnknown (currentStack); } break; @@ -567,7 +567,9 @@ public void Scan (MethodBody methodBody) break; case Code.Ret: { - bool hasReturnValue = methodBody.Method.ReturnType.MetadataType != MetadataType.Void; + + bool hasReturnValue = GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void; + if (currentStack.Count != (hasReturnValue ? 1 : 0)) { WarnAboutInvalidILInMethod (methodBody, operation.Offset); } @@ -892,7 +894,7 @@ private void HandleCall ( else methodReturnValue = newObjValue; } else { - if (calledMethod.ReturnType.MetadataType != MetadataType.Void) { + if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) { methodReturnValue = UnknownValue.Instance; } } @@ -908,6 +910,14 @@ private void HandleCall ( } } + protected static TypeReference GetReturnTypeWithoutModifiers (TypeReference returnType) + { + while (returnType is IModifierType) { + returnType = ((IModifierType) returnType).ElementType; + } + return returnType; + } + // Array types that are dynamically accessed should resolve to System.Array instead of its element type - which is what Cecil resolves to. // Any data flow annotations placed on a type parameter which receives an array type apply to the array itself. None of the members in its // element type should be marked. diff --git a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs index 9459ce1d7705..a453702d0bc7 100644 --- a/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs +++ b/src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs @@ -37,10 +37,10 @@ public static bool RequiresReflectionMethodBodyScannerForCallSite (LinkContext c if (methodDefinition == null) return false; - return - GetIntrinsicIdForMethod (methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel || + return GetIntrinsicIdForMethod (methodDefinition) > IntrinsicId.RequiresReflectionBodyScanner_Sentinel || context.Annotations.FlowAnnotations.RequiresDataFlowAnalysis (methodDefinition) || - context.Annotations.HasLinkerAttribute (methodDefinition); + context.Annotations.HasLinkerAttribute (methodDefinition) + || methodDefinition.IsPInvokeImpl; } public static bool RequiresReflectionMethodBodyScannerForMethodBody (FlowAnnotations flowAnnotations, MethodDefinition methodDefinition) @@ -75,7 +75,7 @@ public void ScanAndProcessReturnValue (MethodBody methodBody) { Scan (methodBody); - if (methodBody.Method.ReturnType.MetadataType != MetadataType.Void) { + if (GetReturnTypeWithoutModifiers (methodBody.Method.ReturnType).MetadataType != MetadataType.Void) { var method = methodBody.Method; var requiredMemberTypes = _context.Annotations.FlowAnnotations.GetReturnParameterAnnotation (method); if (requiredMemberTypes != 0) { @@ -1729,6 +1729,20 @@ methodParams[argsParam] is ArrayValue arrayValue && break; default: + + if (calledMethodDefinition.IsPInvokeImpl) { + // Is the PInvoke dangerous? + bool comDangerousMethod = IsComInterop (calledMethodDefinition.MethodReturnType, calledMethodDefinition.ReturnType); + foreach (ParameterDefinition pd in calledMethodDefinition.Parameters) { + comDangerousMethod |= IsComInterop (pd, pd.ParameterType); + } + + if (comDangerousMethod) { + reflectionContext.AnalyzingPattern (); + reflectionContext.RecordUnrecognizedPattern (2050, $"P/invoke method '{calledMethodDefinition.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed."); + } + } + if (requiresDataFlowAnalysis) { reflectionContext.AnalyzingPattern (); for (int parameterIndex = 0; parameterIndex < methodParams.Count; parameterIndex++) { @@ -1755,7 +1769,7 @@ methodParams[argsParam] is ArrayValue arrayValue && // To get good reporting of errors we need to track the origin of the value for all method calls // but except Newobj as those are special. - if (calledMethodDefinition.ReturnType.MetadataType != MetadataType.Void) { + if (GetReturnTypeWithoutModifiers (calledMethodDefinition.ReturnType).MetadataType != MetadataType.Void) { methodReturnValue = CreateMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes); return true; @@ -1771,7 +1785,7 @@ methodParams[argsParam] is ArrayValue arrayValue && // didn't set the return value (and the method has a return value), we will set it to be an // unknown value with the return type of the method. if (methodReturnValue == null) { - if (calledMethod.ReturnType.MetadataType != MetadataType.Void) { + if (GetReturnTypeWithoutModifiers (calledMethod.ReturnType).MetadataType != MetadataType.Void) { methodReturnValue = CreateMethodReturnValue (calledMethodDefinition, returnValueDynamicallyAccessedMemberTypes); } } @@ -1794,6 +1808,64 @@ methodParams[argsParam] is ArrayValue arrayValue && return true; } + bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReference parameterType) + { + // This is best effort. One can likely find ways how to get COM without triggering these alarms. + // AsAny marshalling of a struct with an object-typed field would be one, for example. + + // This logic roughly corresponds to MarshalInfo::MarshalInfo in CoreCLR, + // not trying to handle invalid cases and distinctions that are not interesting wrt + // "is this COM?" question. + + NativeType nativeType = NativeType.None; + if (marshalInfoProvider.HasMarshalInfo) { + nativeType = marshalInfoProvider.MarshalInfo.NativeType; + } + + if (nativeType == NativeType.IUnknown || nativeType == NativeType.IDispatch || nativeType == NativeType.IntF) { + // This is COM by definition + return true; + } + + if (nativeType == NativeType.None) { + // Resolve will look at the element type + var parameterTypeDef = _context.TryResolve (parameterType); + + if (parameterTypeDef != null) { + if (parameterTypeDef.IsTypeOf ("System", "Array")) { + // System.Array marshals as IUnknown by default + return true; + } else if (parameterTypeDef.IsTypeOf ("System", "String") || + parameterTypeDef.IsTypeOf ("System.Text", "StringBuilder")) { + // String and StringBuilder are special cased by interop + return false; + } + + if (parameterTypeDef.IsValueType) { + // Value types don't marshal as COM + return false; + } else if (parameterTypeDef.IsInterface) { + // Interface types marshal as COM by default + return true; + } else if (parameterTypeDef.IsMulticastDelegate ()) { + // Delegates are special cased by interop + return false; + } else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "CriticalHandle")) { + // Subclasses of CriticalHandle are special cased by interop + return false; + } else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "SafeHandle")) { + // Subclasses of SafeHandle are special cased by interop + return false; + } else if (!parameterTypeDef.IsSequentialLayout && !parameterTypeDef.IsExplicitLayout) { + // Rest of classes that don't have layout marshal as COM + return true; + } + } + } + + return false; + } + bool AnalyzeGenericInstatiationTypeArray (ValueNode arrayParam, ref ReflectionPatternContext reflectionContext, MethodReference calledMethod, IList genericParameters) { bool hasRequirements = false; diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index c5b87c3833c0..d6577c367be3 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -3036,8 +3036,6 @@ void ProcessInteropMethod (MethodDefinition method) TypeDefinition returnTypeDefinition = _context.TryResolve (method.ReturnType); - bool didWarnAboutCom = false; - const bool includeStaticFields = false; if (returnTypeDefinition != null) { if (!returnTypeDefinition.IsImport) { @@ -3045,19 +3043,6 @@ void ProcessInteropMethod (MethodDefinition method) MarkDefaultConstructor (returnTypeDefinition, new DependencyInfo (DependencyKind.InteropMethodDependency, method)); MarkFields (returnTypeDefinition, includeStaticFields, new DependencyInfo (DependencyKind.InteropMethodDependency, method)); } - - // Best-effort attempt to find COM marshalling. - // It might seem reasonable to allow out parameters, but once we get a foreign object back (an RCW), it's going to look - // like a regular managed class/interface, but every method invocation that takes a class/interface implies COM - // marshalling. We can't detect that once we have an RCW. - if (method.IsPInvokeImpl) { - if (IsComInterop (method.MethodReturnType, method.ReturnType) && !didWarnAboutCom) { - _context.LogWarning ( - $"P/invoke method '{method.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.", - 2050, method, subcategory: MessageSubCategory.TrimAnalysis); - didWarnAboutCom = true; - } - } } if (method.HasThis && !method.DeclaringType.IsImport) { @@ -3079,75 +3064,8 @@ void ProcessInteropMethod (MethodDefinition method) MarkDefaultConstructor (paramTypeDefinition, new DependencyInfo (DependencyKind.InteropMethodDependency, method)); } } - - // Best-effort attempt to find COM marshalling. - // It might seem reasonable to allow out parameters, but once we get a foreign object back (an RCW), it's going to look - // like a regular managed class/interface, but every method invocation that takes a class/interface implies COM - // marshalling. We can't detect that once we have an RCW. - if (method.IsPInvokeImpl) { - if (IsComInterop (pd, paramTypeReference) && !didWarnAboutCom) { - _context.LogWarning ($"P/invoke method '{method.GetDisplayName ()}' declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.", 2050, method); - didWarnAboutCom = true; - } - } - } - } - } - - bool IsComInterop (IMarshalInfoProvider marshalInfoProvider, TypeReference parameterType) - { - // This is best effort. One can likely find ways how to get COM without triggering these alarms. - // AsAny marshalling of a struct with an object-typed field would be one, for example. - - // This logic roughly corresponds to MarshalInfo::MarshalInfo in CoreCLR, - // not trying to handle invalid cases and distinctions that are not interesting wrt - // "is this COM?" question. - - NativeType nativeType = NativeType.None; - if (marshalInfoProvider.HasMarshalInfo) { - nativeType = marshalInfoProvider.MarshalInfo.NativeType; - } - - if (nativeType == NativeType.IUnknown || nativeType == NativeType.IDispatch || nativeType == NativeType.IntF) { - // This is COM by definition - return true; - } - - if (nativeType == NativeType.None) { - if (parameterType.IsTypeOf ("System", "Array")) { - // System.Array marshals as IUnknown by default - return true; - } else if (parameterType.IsTypeOf ("System", "String") || - parameterType.IsTypeOf ("System.Text", "StringBuilder")) { - // String and StringBuilder are special cased by interop - return false; - } - - var parameterTypeDef = _context.TryResolve (parameterType); - if (parameterTypeDef != null) { - if (parameterTypeDef.IsValueType) { - // Value types don't marshal as COM - return false; - } else if (parameterTypeDef.IsInterface) { - // Interface types marshal as COM by default - return true; - } else if (parameterTypeDef.IsMulticastDelegate ()) { - // Delegates are special cased by interop - return false; - } else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "CriticalHandle")) { - // Subclasses of CriticalHandle are special cased by interop - return false; - } else if (parameterTypeDef.IsSubclassOf ("System.Runtime.InteropServices", "SafeHandle")) { - // Subclasses of SafeHandle are special cased by interop - return false; - } else if (!parameterTypeDef.IsSequentialLayout && !parameterTypeDef.IsExplicitLayout) { - // Rest of classes that don't have layout marshal as COM - return true; - } } } - - return false; } protected virtual bool ShouldParseMethodBody (MethodDefinition method) diff --git a/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs b/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs index 40e661340803..720ad738b6f8 100644 --- a/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs +++ b/test/Mono.Linker.Tests.Cases/Interop/PInvoke/Warnings/ComPInvokeWarning.cs @@ -15,39 +15,198 @@ class ComPInvokeWarning [UnconditionalSuppressMessage ("trim", "IL2026")] static void Main () { - SomeMethodTakingInterface (null); - SomeMethodTakingObject (null); - GetInterface (); - CanSuppressWarningOnParameter (null); - CanSuppressWarningOnReturnType (); - CanSuppressWithRequiresUnreferencedCode (null); + Call_SomeMethodTakingInterface (); + Call_SomeMethodTakingObject (); + Call_SomeMethodTakingArray (); + Call_SomeMethodTakingString (); + Call_SomeMethodTakingStringBuilder (); + Call_SomeMethodTakingCriticalHandle (); + Call_SomeMethodTakingSafeHandle (); + Call_SomeMethodTakingExplicitLayoutClass (); + Call_SomeMethodTakingSequentialLayoutClass (); + Call_SomeMethodTakingAutoLayoutClass (); + Call_GetInterface (); + Call_CanSuppressWarningOnParameter (); + Call_CanSuppressWarningOnReturnType (); + Call_CanSuppressWithRequiresUnreferencedCode (); + Call_CanSuppressPInvokeWithRequiresUnreferencedCode (); + Call_PInvokeWithRequiresUnreferencedCode (); } [ExpectedWarning ("IL2050")] + static void Call_SomeMethodTakingInterface () + { + SomeMethodTakingInterface (null); + } [DllImport ("Foo")] static extern void SomeMethodTakingInterface (IFoo foo); [ExpectedWarning ("IL2050")] + static void Call_SomeMethodTakingObject () + { + SomeMethodTakingObject (null); + } [DllImport ("Foo")] static extern void SomeMethodTakingObject ([MarshalAs (UnmanagedType.IUnknown)] object obj); [ExpectedWarning ("IL2050")] + static void Call_SomeMethodTakingArray () + { + SomeMethodTakingArray (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingArray (Array array); + + static void Call_SomeMethodTakingStringBuilder () + { + SomeMethodTakingStringBuilder (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingStringBuilder (StringBuilder str); + + static void Call_SomeMethodTakingCriticalHandle () + { + SomeMethodTakingCriticalHandle (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingCriticalHandle (MyCriticalHandle handle); + + + static void Call_SomeMethodTakingSafeHandle () + { + SomeMethodTakingSafeHandle (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingSafeHandle (TestSafeHandle handle); + + static void Call_SomeMethodTakingExplicitLayoutClass () + { + SomeMethodTakingExplicitLayout (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingExplicitLayout (ExplicitLayout _class); + + static void Call_SomeMethodTakingSequentialLayoutClass () + { + SomeMethodTakingSequentialLayout (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingSequentialLayout (SequentialLayout _class); + + [ExpectedWarning ("IL2050")] + static void Call_SomeMethodTakingAutoLayoutClass () + { + SomeMethodTakingAutoLayout (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingAutoLayout (AutoLayout _class); + + + static void Call_SomeMethodTakingString () + { + SomeMethodTakingString (null); + } + [DllImport ("Foo")] + static extern void SomeMethodTakingString (String str); + + [ExpectedWarning ("IL2050")] + static void Call_GetInterface () + { + GetInterface (); + } [DllImport ("Foo")] static extern IFoo GetInterface (); [UnconditionalSuppressMessage ("trim", "IL2050")] + static void Call_CanSuppressWarningOnParameter () + { + CanSuppressWarningOnParameter (null); + } [DllImport ("Foo")] static extern void CanSuppressWarningOnParameter ([MarshalAs (UnmanagedType.IUnknown)] object obj); [UnconditionalSuppressMessage ("trim", "IL2050")] + static void Call_CanSuppressWarningOnReturnType () + { + CanSuppressWarningOnReturnType (); + } [DllImport ("Foo")] static extern IFoo CanSuppressWarningOnReturnType (); - [ExpectedWarning ("IL2050")] // Issue https://github.com/mono/linker/issues/1989 [RequiresUnreferencedCode ("test")] + static void Call_CanSuppressWithRequiresUnreferencedCode () + { + CanSuppressWithRequiresUnreferencedCode (null); + } + [DllImport ("Foo")] static extern void CanSuppressWithRequiresUnreferencedCode (IFoo foo); + [RequiresUnreferencedCode ("test")] + static void Call_CanSuppressPInvokeWithRequiresUnreferencedCode () + { + CanSuppressPInvokeWithRequiresUnreferencedCode (null); + } + + [RequiresUnreferencedCode ("test")] + [DllImport ("Foo")] + static extern void CanSuppressPInvokeWithRequiresUnreferencedCode (IFoo foo); + + [ExpectedWarning ("IL2050")] + [ExpectedWarning ("IL2026")] + static void Call_PInvokeWithRequiresUnreferencedCode () + { + PInvokeWithRequiresUnreferencedCode (null); + } + + [RequiresUnreferencedCode ("test")] + [DllImport ("Foo")] + static extern void PInvokeWithRequiresUnreferencedCode (IFoo foo); + interface IFoo { } + + class TestSafeHandle : SafeHandle + { + public TestSafeHandle () + : base (IntPtr.Zero, true) + { } + + public override bool IsInvalid => handle == IntPtr.Zero; + + protected override bool ReleaseHandle () + { + return true; + } + } + + class MyCriticalHandle : CriticalHandle + { + public MyCriticalHandle () : base (new IntPtr (-1)) { } + + public override bool IsInvalid { + get { return false; } + } + + protected override bool ReleaseHandle () + { + return false; + } + } + + [StructLayout (LayoutKind.Explicit)] + public class ExplicitLayout + { + } + + [StructLayout (LayoutKind.Sequential)] + public class SequentialLayout + { + } + + [StructLayout (LayoutKind.Auto)] + public class AutoLayout + { + } + } }