PropertySetAnalysis: Handling assignments to flow captures when track…

…ing properties
dotnet · Oct 22, 2019 · 51dd600 · 51dd600
1 parent 5b922ab
commit 51dd600
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 18 deletions.
diff --git a/...yzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs b/...yzers/UnitTests/Security/DoNotUseInsecureDeserializerBinaryFormatterWithoutBinderTests.cs
@@ -1198,7 +1198,7 @@ public BookRecord DeserializeBookRecord(byte[] bytes)
         }
 
         [Fact]
-        public void Deserialize_SharedBinderInstance_Diagnostic()
+        public void Deserialize_SharedBinderInstance_NoDiagnostic()
         {
             VerifyCSharpWithMyBinderDefined(@"
 using System;
@@ -1220,10 +1220,7 @@ private object DoDeserialization(Stream stream)
             return f.Deserialize(stream);
         }
     }
-}",
-                GetCSharpResultAt(18, 20, BinderNotSetRule, "object BinaryFormatter.Deserialize(Stream serializationStream)"));
-
-            // Ideally, we'd be able to tell f.Binder is non-null.
+}");
         }
 
         [Fact]

diff --git a/...s/Analysis/PropertySetAnalysis/PropertySetAnalysis.PropertySetDataFlowOperationVisitor.cs b/...s/Analysis/PropertySetAnalysis/PropertySetAnalysis.PropertySetDataFlowOperationVisitor.cs
@@ -241,19 +241,9 @@ protected override PropertySetAbstractValue VisitAssignmentOperation(IAssignment
                     AnalysisEntity targetAnalysisEntity = null;
                     if (operation.Target.Kind == OperationKind.FlowCaptureReference)
                     {
-                        PointsToAbstractValue lValuePointsToAbstractValue = this.GetPointsToAbstractValue(operation.Target);
-                        if (lValuePointsToAbstractValue.LValueCapturedOperations.Count == 1)
+                        if (this.TryUnwrapFlowCaptureReference(operation.Target, out IOperation lValueOperation, OperationKind.PropertyReference, OperationKind.FieldReference))
                         {
-                            IOperation lValueOperation = lValuePointsToAbstractValue.LValueCapturedOperations.First();
-                            if (lValueOperation.Kind == OperationKind.FieldReference
-                                || lValueOperation.Kind == OperationKind.PropertyReference)
-                            {
-                                this.AnalysisEntityFactory.TryCreate(lValueOperation, out targetAnalysisEntity);
-                            }
-                        }
-                        else
-                        {
-                            Debug.Fail("Can LValues FlowCaptureReferences have more than one operation?");
+                            this.AnalysisEntityFactory.TryCreate(lValueOperation, out targetAnalysisEntity);
                         }
                     }
                     else
@@ -295,7 +285,14 @@ protected override PropertySetAbstractValue VisitAssignmentOperation(IAssignment
                     }
                 }
 
-                if (operation.Target is IPropertyReferenceOperation propertyReferenceOperation
+                IPropertyReferenceOperation propertyReferenceOperation = operation.Target as IPropertyReferenceOperation;
+                if (propertyReferenceOperation == null && operation.Target.Kind == OperationKind.FlowCaptureReference)
+                {
+                    this.TryUnwrapFlowCaptureReference(operation.Target, out IOperation lValue, OperationKind.PropertyReference);
+                    propertyReferenceOperation = lValue as IPropertyReferenceOperation;
+                }
+
+                if (propertyReferenceOperation != null
                     && propertyReferenceOperation.Instance != null
                     && this.TrackedTypeSymbols.Any(s => propertyReferenceOperation.Instance.Type.GetBaseTypesAndThis().Contains(s))
                     && this.DataFlowAnalysisContext.PropertyMappers.TryGetPropertyMapper(
@@ -698,6 +695,37 @@ private void MergeInterproceduralResults(IOperation originalOperation)
                     this._visitedLambdas.Add(lambdaOperation);
                 }
             }
+
+            /// <summary>
+            /// Attempts to find the underlying IOperation that a flow capture reference refers to.
+            /// </summary>
+            /// <param name="flowCaptureReferenceOperation">Operation that may be a flow capture reference to look at.</param>
+            /// <param name="unwrappedOperation">The found underlying operation, if any.</param>
+            /// <param name="kinds">Kinds of operations to look for.</param>
+            /// <returns>True if found, false otherwise.</returns>
+            private bool TryUnwrapFlowCaptureReference(IOperation flowCaptureReferenceOperation, out IOperation unwrappedOperation, params OperationKind[] kinds)
+            {
+                unwrappedOperation = null;
+                if (flowCaptureReferenceOperation != null && flowCaptureReferenceOperation.Kind == OperationKind.FlowCaptureReference)
+                {
+                    PointsToAbstractValue lValuePointsToAbstractValue = this.GetPointsToAbstractValue(flowCaptureReferenceOperation);
+                    if (lValuePointsToAbstractValue.LValueCapturedOperations.Count == 1)
+                    {
+                        IOperation lValueOperation = lValuePointsToAbstractValue.LValueCapturedOperations.First();
+                        if (kinds == null || kinds.Contains(lValueOperation.Kind))
+                        {
+                            unwrappedOperation = lValueOperation;
+                            return true;
+                        }
+                    }
+                    else
+                    {
+                        Debug.Fail("Can LValues FlowCaptureReferences have more than one operation?");
+                    }
+                }
+
+                return false;
+            }
         }
     }
 }