github · tamasvajk · Mar 16, 2021 · Feb 24, 2021 · Feb 24, 2021 · Feb 24, 2021
@@ -0,0 +1,3 @@
+lgtm,codescanning
+* Data flow for tuples has been improved to track data flowing into and out of
+tuple fields. Tuple extraction was changed to extract non-named tuple elements.
@@ -12,19 +12,22 @@ namespace Semmle.Extraction.CSharp.Entities
     internal class Expression : FreshEntity, IExpressionParentEntity
     {
         private readonly IExpressionInfo info;
-        public AnnotatedTypeSymbol? Type { get; }
+        public AnnotatedTypeSymbol? Type { get; private set; }
         public Extraction.Entities.Location Location { get; }
         public ExprKind Kind { get; }
 
-        internal Expression(IExpressionInfo info)
+        internal Expression(IExpressionInfo info, bool shouldPopulate = true)
             : base(info.Context)
         {
             this.info = info;
             Location = info.Location;
             Kind = info.Kind;
             Type = info.Type;
 
-            TryPopulate();
+            if (shouldPopulate)
+            {
+                TryPopulate();
+            }
         }
 
         protected sealed override void Populate(TextWriter trapFile)
@@ -59,6 +62,14 @@ protected sealed override void Populate(TextWriter trapFile)
 
         public override Location? ReportingLocation => Location.Symbol;
 
+        internal void SetType(ITypeSymbol? type)
+        {
+            if (type is not null)
+            {
+                Type = new AnnotatedTypeSymbol(type, type.NullableAnnotation);
+            }
+        }
+
         bool IExpressionParentEntity.IsTopLevelParent => false;
 
         /// <summary>

@@ -3,7 +3,9 @@
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Semmle.Extraction.CSharp.Populators;
 using Semmle.Extraction.Kinds;
-using Semmle.Extraction.Entities;
+using System.Collections.Generic;
+using System.Linq;
+using System.Collections.Immutable;
 
 namespace Semmle.Extraction.CSharp.Entities.Expressions
 {
@@ -47,69 +49,91 @@ private static VariableDeclaration CreateSingle(Context cx, DeclarationExpressio
         /// Create a tuple expression representing a parenthesized variable declaration.
         /// That is, we consider `var (x, y) = ...` to be equivalent to `(var x, var y) = ...`.
         /// </summary>
-        public static Expression CreateParenthesized(Context cx, DeclarationExpressionSyntax node, ParenthesizedVariableDesignationSyntax designation, IExpressionParentEntity parent, int child)
+        public static Expression CreateParenthesized(Context cx, DeclarationExpressionSyntax node, ParenthesizedVariableDesignationSyntax designation, IExpressionParentEntity parent, int child, INamedTypeSymbol? t)
         {
-            AnnotatedTypeSymbol? type = null; // Should ideally be a corresponding tuple type
+            var type = t is null ? (AnnotatedTypeSymbol?)null : new AnnotatedTypeSymbol(t, t.NullableAnnotation);
             var tuple = new Expression(new ExpressionInfo(cx, type, cx.CreateLocation(node.GetLocation()), ExprKind.TUPLE, parent, child, false, null));
 
             cx.Try(null, null, () =>
             {
-                var child0 = 0;
-                foreach (var variable in designation.Variables)
-                    Create(cx, node, variable, tuple, child0++);
+                for (var child0 = 0; child0 < designation.Variables.Count; child0++)
+                {
+                    Create(cx, node, designation.Variables[child0], tuple, child0, t?.TypeArguments[child0] as INamedTypeSymbol);
+                }
             });
 
             return tuple;
         }
 
         public static Expression CreateParenthesized(Context cx, VarPatternSyntax varPattern, ParenthesizedVariableDesignationSyntax designation, IExpressionParentEntity parent, int child)
         {
-            AnnotatedTypeSymbol? type = null; // Should ideally be a corresponding tuple type
-            var tuple = new Expression(new ExpressionInfo(cx, type, cx.CreateLocation(varPattern.GetLocation()), ExprKind.TUPLE, parent, child, false, null));
+            var tuple = new Expression(
+                new ExpressionInfo(cx, null, cx.CreateLocation(varPattern.GetLocation()), ExprKind.TUPLE, parent, child, false, null),
+                shouldPopulate: false);
 
+            var elementTypes = new List<ITypeSymbol?>();
             cx.Try(null, null, () =>
             {
                 var child0 = 0;
                 foreach (var variable in designation.Variables)
                 {
+                    Expression sub;
                     switch (variable)
                     {
                         case ParenthesizedVariableDesignationSyntax paren:
-                            CreateParenthesized(cx, varPattern, paren, tuple, child0++);
+                            sub = CreateParenthesized(cx, varPattern, paren, tuple, child0++);
                             break;
                         case SingleVariableDesignationSyntax single:
                             if (cx.GetModel(variable).GetDeclaredSymbol(single) is ILocalSymbol local)
                             {
                                 var decl = Create(cx, variable, local.GetAnnotatedType(), tuple, child0++);
                                 var l = LocalVariable.Create(cx, local);
                                 l.PopulateManual(decl, true);
+                                sub = decl;
                             }
                             else
                             {
                                 throw new InternalError(single, "Failed to access local variable");
                             }
                             break;
                         case DiscardDesignationSyntax discard:
-                            new Discard(cx, discard, tuple, child0++);
+                            sub = new Discard(cx, discard, tuple, child0++);
+                            if (!sub.Type.HasValue || sub.Type.Value.Symbol is null)
+                            {
+                                // The type is only updated in memory, it will not be written to the trap file.
+                                sub.SetType(cx.Compilation.GetSpecialType(SpecialType.System_Object));
+                            }
                             break;
                         default:
                             throw new InternalError(variable, "Unhandled designation type");
                     }
+
+                    elementTypes.Add(sub.Type.HasValue && sub.Type.Value.Symbol?.Kind != SymbolKind.ErrorType
+                        ? sub.Type.Value.Symbol
+                        : null);
                 }
             });
 
+            INamedTypeSymbol? tupleType = null;
+            if (!elementTypes.Any(et => et is null))
+            {
+                tupleType = cx.Compilation.CreateTupleTypeSymbol(elementTypes.ToImmutableArray()!);
+            }
+
+            tuple.SetType(tupleType);
+            tuple.TryPopulate();
+
             return tuple;
         }
 
-
-        private static Expression Create(Context cx, DeclarationExpressionSyntax node, VariableDesignationSyntax? designation, IExpressionParentEntity parent, int child)
+        private static Expression Create(Context cx, DeclarationExpressionSyntax node, VariableDesignationSyntax? designation, IExpressionParentEntity parent, int child, INamedTypeSymbol? declarationType)
         {
             switch (designation)
             {
                 case SingleVariableDesignationSyntax single:
                     return CreateSingle(cx, node, single, parent, child);
                 case ParenthesizedVariableDesignationSyntax paren:
-                    return CreateParenthesized(cx, node, paren, parent, child);
+                    return CreateParenthesized(cx, node, paren, parent, child, declarationType);
                 case DiscardDesignationSyntax discard:
                     var type = cx.GetType(discard);
                     return Create(cx, node, type, parent, child);
@@ -120,7 +144,7 @@ private static Expression Create(Context cx, DeclarationExpressionSyntax node, V
         }
 
         public static Expression Create(Context cx, DeclarationExpressionSyntax node, IExpressionParentEntity parent, int child) =>
-            Create(cx, node, node.Designation, parent, child);
+            Create(cx, node, node.Designation, parent, child, cx.GetTypeInfo(node).Type.DisambiguateType() as INamedTypeSymbol);
 
         public static VariableDeclaration Create(Context cx, CSharpSyntaxNode c, AnnotatedTypeSymbol? type, IExpressionParentEntity parent, int child) =>
             new VariableDeclaration(new ExpressionInfo(cx, type, cx.CreateLocation(c.FixedLocation()), ExprKind.LOCAL_VAR_DECL, parent, child, false, null));

@@ -17,7 +17,7 @@ private Field(Context cx, IFieldSymbol init)
             type = new Lazy<Type>(() => Entities.Type.Create(cx, Symbol.Type));
         }
 
-        public static Field Create(Context cx, IFieldSymbol field) => FieldFactory.Instance.CreateEntityFromSymbol(cx, field);
+        public static Field Create(Context cx, IFieldSymbol field) => FieldFactory.Instance.CreateEntityFromSymbol(cx, field.CorrespondingTupleField ?? field);
 
         // Do not populate backing fields.
         // Populate Tuple fields.

@@ -13,7 +13,7 @@ namespace Semmle.Extraction.CSharp.Entities
     /// </summary>
     internal class TupleType : Type<INamedTypeSymbol>
     {
-        public static TupleType Create(Context cx, INamedTypeSymbol type) => TupleTypeFactory.Instance.CreateEntityFromSymbol(cx, type);
+        public static TupleType Create(Context cx, INamedTypeSymbol type) => TupleTypeFactory.Instance.CreateEntityFromSymbol(cx, type.TupleUnderlyingType ?? type);
 
         private class TupleTypeFactory : CachedEntityFactory<INamedTypeSymbol, TupleType>
         {
@@ -41,7 +41,7 @@ public override void Populate(TextWriter trapFile)
             PopulateType(trapFile);
             PopulateGenerics();
 
-            var underlyingType = NamedType.CreateNamedTypeFromTupleType(Context, Symbol.TupleUnderlyingType ?? Symbol);
+            var underlyingType = NamedType.CreateNamedTypeFromTupleType(Context, Symbol);
 
             trapFile.tuple_underlying_type(this, underlyingType);
 

@@ -295,7 +295,7 @@ private static void BuildNamedTypeId(this INamedTypeSymbol named, Context cx, Te
                 trapFile.BuildList(",", named.TupleElements,
                     (f, tb0) =>
                     {
-                        trapFile.Write(f.Name);
+                        trapFile.Write((f.CorrespondingTupleField ?? f).Name);
                         trapFile.Write(":");
                         f.Type.BuildOrWriteId(cx, tb0, symbolBeingDefined, addBaseClass);
                     }

@@ -13,7 +13,7 @@ protected FreshEntity(Context cx) : base(cx)
 
         protected abstract void Populate(TextWriter trapFile);
 
-        protected void TryPopulate()
+        public void TryPopulate()
         {
             Context.Try(null, null, () => Populate(Context.TrapWriter.Writer));
         }

@@ -537,6 +537,9 @@ module AssignableDefinitions {
     /** Gets the underlying assignment. */
     AssignExpr getAssignment() { result = ae }
 
+    /** Gets the leaf expression. */
+    Expr getLeaf() { result = leaf }
+
     /**
      * Gets the evaluation order of this definition among the other definitions
      * in the compound tuple assignment. For example, in `(x, (y, z)) = ...` the

@@ -220,6 +220,16 @@ module LocalFlow {
           e1 = we.getInitializer() and
           e2 = we
         )
+        or
+        scope = any(AssignExpr ae | ae.getLValue().(TupleExpr) = e2 and ae.getRValue() = e1) and
+        isSuccessor = false
+        or
+        isSuccessor = true and
+        exists(ControlFlowElement cfe | cfe = e2.(TupleExpr).(PatternExpr).getPatternMatch() |
+          cfe.(IsExpr).getExpr() = e1 and scope = cfe
+          or
+          exists(Switch sw | sw.getACase() = cfe and sw.getExpr() = e1 and scope = sw)
+        )
       )
     }
 
@@ -483,6 +493,18 @@ private predicate fieldOrPropertyStore(Expr e, Content c, Expr src, Expr q, bool
       src = mi.getRValue() and
       postUpdate = false
     )
+    or
+    // Tuple element, `(..., src, ...)` `f` is `ItemX` of tuple `q`
+    e =
+      any(TupleExpr te |
+        exists(int i |
+          e = q and
+          src = te.getArgument(i) and
+          te.isConstruction() and
+          f = q.getType().(TupleType).getElement(i) and
+          postUpdate = false
+        )
+      )
   )
 }
 
@@ -495,7 +517,7 @@ private predicate overridesOrImplementsSourceDecl(Property p1, Property p2) {
 
 /**
  * Holds if `e2` is an expression that reads field or property `c` from
- * expresion `e1`. This takes overriding into account for properties written
+ * expression `e1`. This takes overriding into account for properties written
  * from library code.
  */
 private predicate fieldOrPropertyRead(Expr e1, Content c, FieldOrPropertyRead e2) {
@@ -792,6 +814,37 @@ private module Cached {
       hasNodePath(x, node1, node2) and
       node2.asExpr().(AwaitExpr).getExpr() = node1.asExpr() and
       c = getResultContent()
+      or
+      // node1 = (..., node2, ...)
+      // node1.ItemX flows to node2
+      exists(TupleExpr te, int i, Expr item |
+        te = node1.asExpr() and
+        not te.isConstruction() and
+        c.(FieldContent).getField() = te.getType().(TupleType).getElement(i).getUnboundDeclaration() and
+        // node1 = (..., item, ...)
+        te.getArgument(i) = item
+      |
+        // item = (..., ..., ...) in node1 = (..., (..., ..., ...), ...)
+        node2.asExpr().(TupleExpr) = item and
+        hasNodePath(x, node1, node2)
+        or
+        // item = variable in node1 = (..., variable, ...)
+        exists(AssignableDefinitions::TupleAssignmentDefinition tad, Ssa::ExplicitDefinition def |
+          node2.(SsaDefinitionNode).getDefinition() = def and
+          def.getADefinition() = tad and
+          tad.getLeaf() = item and
+          hasNodePath(x, node1, node2)
+        )
+        or
+        // item = variable in node1 = (..., variable, ...) in a case/is var (..., ...)
+        te = any(PatternExpr pe).getAChildExpr*() and
+        exists(AssignableDefinitions::LocalVariableDefinition lvd, Ssa::ExplicitDefinition def |
+          node2.(SsaDefinitionNode).getDefinition() = def and
+          def.getADefinition() = lvd and
+          lvd.getDeclaration() = item and
+          hasNodePath(x, node1, node2)
+        )
+      )
     )
     or
     FlowSummaryImpl::Private::readStep(node1, c, node2)
@@ -1731,6 +1784,11 @@ private class ReadStepConfiguration extends ControlFlowReachabilityConfiguration
     e1 = e2.(AwaitExpr).getExpr() and
     scope = e2 and
     isSuccessor = true
+    or
+    exactScope = false and
+    e2 = e1.(TupleExpr).getAnArgument() and
+    scope = e1 and
+    isSuccessor = false
   }
 
   override predicate candidateDef(
@@ -1752,6 +1810,22 @@ private class ReadStepConfiguration extends ControlFlowReachabilityConfiguration
       scope = fs.getVariableDeclExpr() and
       exactScope = false
     )
+    or
+    scope =
+      any(AssignExpr ae |
+        ae = defTo.(AssignableDefinitions::TupleAssignmentDefinition).getAssignment() and
+        e = ae.getLValue().getAChildExpr*().(TupleExpr) and
+        exactScope = false and
+        isSuccessor = true
+      )
+    or
+    scope =
+      any(TupleExpr te |
+        te.getAnArgument() = defTo.(AssignableDefinitions::LocalVariableDefinition).getDeclaration() and
+        e = te and
+        exactScope = false and
+        isSuccessor = false
+      )
   }
 }
 

@@ -62,25 +62,9 @@ private class LocalTaintExprStepConfiguration extends ControlFlowReachabilityCon
       e1 = e2.(BinaryLogicalOperation).getAnOperand() and
       scope = e2
       or
-      // Taint from tuple argument
-      e2 =
-        any(TupleExpr te |
-          e1 = te.getAnArgument() and
-          te.isReadAccess() and
-          scope = e2
-        )
-      or
       e1 = e2.(InterpolatedStringExpr).getAChild() and
       scope = e2
       or
-      // Taint from tuple expression
-      e2 =
-        any(MemberAccess ma |
-          ma.getQualifier().getType() instanceof TupleType and
-          e1 = ma.getQualifier() and
-          scope = e2
-        )
-      or
       e2 =
         any(OperatorCall oc |
           oc.getTarget().(ConversionOperator).fromLibrary() and

@@ -1035,7 +1035,13 @@ class TupleExpr extends Expr, @tuple_expr {
   Expr getAnArgument() { result = getArgument(_) }
 
   /** Holds if this tuple is a read access. */
-  predicate isReadAccess() { not this = getAnAssignOrForeachChild() }
+  deprecated predicate isReadAccess() { not this = getAnAssignOrForeachChild() }
+
+  /** Holds if this expression is a tuple construction. */
+  predicate isConstruction() {
+    not this = getAnAssignOrForeachChild() and
+    not this = any(PatternExpr pe).getAChildExpr*()
+  }
 
   override string getAPrimaryQlClass() { result = "TupleExpr" }
 }