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

Handle PInvoke analysis similar to other methods #2091

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Handle PInvoke analysis similar to other methods #2091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
78a0646
PInvoke fix
LakshanF Jun 14, 2021
04c584a
FB
LakshanF Jun 15, 2021
a7bc36f
FB
LakshanF Jun 16, 2021
af01f1d
FB2
LakshanF Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions src/linker/Linker.Dataflow/MethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
}
Expand All @@ -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.
Expand Down
84 changes: 78 additions & 6 deletions src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
LakshanF marked this conversation as resolved.
Show resolved Hide resolved
context.Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (methodDefinition);
context.Annotations.HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (methodDefinition)
|| methodDefinition.IsPInvokeImpl;
}

public static bool RequiresReflectionMethodBodyScannerForMethodBody (FlowAnnotations flowAnnotations, MethodDefinition methodDefinition)
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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++) {
Expand All @@ -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;
Expand All @@ -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);
}
}
Expand All @@ -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<GenericParameter> genericParameters)
{
bool hasRequirements = false;
Expand Down
82 changes: 0 additions & 82 deletions src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3036,28 +3036,13 @@ void ProcessInteropMethod (MethodDefinition method)

TypeDefinition returnTypeDefinition = _context.TryResolve (method.ReturnType);

bool didWarnAboutCom = false;

const bool includeStaticFields = false;
if (returnTypeDefinition != null) {
if (!returnTypeDefinition.IsImport) {
// What we keep here is correct most of the time, but not every time. Fine for now.
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) {
Expand All @@ -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)
Expand Down
Loading