C#: Dispatch.qll performance tweaks

csharp/extractor/Semmle.Util/FileUtils.cs

@@ -57,7 +57,7 @@ public static void TryDelete(string file)
         /// Finds the path for the program <paramref name="prog"/> based on the
         /// <code>PATH</code> environment variable, and in the case of Windows the
         /// <code>PATHEXT</code> environment variable.
-        /// 
+        ///
         /// Returns <code>null</code> of no path can be found.
         /// </summary>
         public static string FindProgramOnPath(string prog)

csharp/ql/src/Security Features/CWE-134/UncontrolledFormatStringBad.cs

@@ -7,7 +7,7 @@ public class HttpHandler : IHttpHandler
     public void ProcessRequest(HttpContext ctx)
     {
         string format = ctx.Request.QueryString["nameformat"];
-        
+
         // BAD: Uncontrolled format string.
         FormattedName = string.Format(format, Surname, Forenames);
     }

csharp/ql/src/semmle/code/csharp/dispatch/Dispatch.qll

@@ -222,27 +222,55 @@ private module Internal {
     }
   }
 
-  private class FieldOrProperty extends Assignable {
-    FieldOrProperty() {
-      this instanceof Field or
-      this instanceof Property
+  private class DynamicFieldOrProperty extends Assignable {
+    DynamicFieldOrProperty() {
+      (
+        this instanceof Field or
+        this instanceof Property
+      ) and
+      this.getName() = any(DynamicMemberAccess dma).getLateBoundTargetName()
+    }
+
+    predicate isMemberOf(string name, ValueOrRefType t) {
+      name = this.getName() and t.hasMember(this)
+    }
+  }
+
+  private class TypeWithDynamicFieldOrProperty extends ValueOrRefType {
+    DynamicFieldOrProperty fp;
+
+    TypeWithDynamicFieldOrProperty() { fp.isMemberOf(_, this) }
+
+    predicate isImplicitlyConvertibleTo(string name, Type t) {
+      name = fp.getName() and
+      this.isImplicitlyConvertibleTo(t)
     }
   }
 
+  pragma[noinline]
+  private predicate isPossibleDynamicMemberAccessQualifierType(
+    DynamicMemberAccess dma, string name, TypeWithDynamicFieldOrProperty t
+  ) {
+    exists(Type qt, boolean isExact |
+      qt = getAPossibleType(dma.getQualifier(), isExact) and
+      name = dma.getLateBoundTargetName()
+    |
+      isExact = true and t = qt
+      or
+      isExact = false and t.isImplicitlyConvertibleTo(name, qt)
+    )
+  }
+
   /**
    * Gets a possible type for expression `e`. Simple flow is used to track the
    * origin of `e`, and in case `e` is a dynamic member access, only types
    * corresponding to the type of a relevant field or property are included.
    */
   private Type getAPossibleType(Expr e, boolean isExact) {
-    exists(ValueOrRefType qualifierType, FieldOrProperty fp, boolean qualifierTypeIsExact |
-      qualifierType = getAPossibleTypeDynamicMemberAccessQualifier(e, qualifierTypeIsExact, fp)
+    exists(DynamicFieldOrProperty fp, string name, TypeWithDynamicFieldOrProperty t |
+      isPossibleDynamicMemberAccessQualifierType(e, name, t) and
+      fp.isMemberOf(name, t)
     |
-      (
-        if qualifierTypeIsExact = true
-        then qualifierType.hasMember(fp)
-        else fp.getDeclaringType().isImplicitlyConvertibleTo(qualifierType)
-      ) and
       result = fp.getType() and
       isExact = false
     )
@@ -251,13 +279,6 @@ private module Internal {
     result = getASourceType(e, isExact)
   }
 
-  private Type getAPossibleTypeDynamicMemberAccessQualifier(
-    DynamicMemberAccess dma, boolean isExact, FieldOrProperty fp
-  ) {
-    result = getAPossibleType(dma.getQualifier(), isExact) and
-    fp.getName() = dma.getLateBoundTargetName()
-  }
-
   /**
    * Provides the predicate `getASourceType()` for finding all relevant source
    * types for a given expression.
@@ -799,22 +820,14 @@ private module Internal {
     //    conflicting types (for example, `Tuple<int, string>` is considered
     //    compatible with `Tuple<T, T>`).
     override RuntimeCallable getADynamicTarget() {
-      // Condition 1
-      result = getADynamicTargetCandidate() and
-      // Condition 2
-      forall(int i | i in [0 .. getNumberOfArguments() - 1] |
-        result = getADynamicTargetCandidateWithCompatibleArg(i)
-      )
-    }
-
-    private RuntimeCallable getADynamicTargetCandidateWithCompatibleArg(int i) {
-      result = getADynamicTargetCandidateWithCompatibleArg1(i) or
-      result = getADynamicTargetCandidateWithCompatibleArg2(i)
+      result = this.getADynamicTarget(this.getNumberOfArguments() - 1)
     }
 
-    pragma[noinline]
-    private RuntimeCallable getADynamicTargetCandidateWithCompatibleArg1(int i) {
-      result = this.getADynamicTargetCandidate() and
+    private RuntimeCallable getADynamicTarget(int i) {
+      i = -1 and
+      result = this.getADynamicTargetCandidate()
+      or
+      result = this.getADynamicTarget(i - 1) and
       exists(Type parameterType, Type argumentType |
         parameterType = this.getAParameterType(result, i) and
         argumentType = getAPossibleType(this.getArgument(i), _)
@@ -827,6 +840,12 @@ private module Internal {
         or
         reflectionOrDynamicArgEqualsParamModuloTypeParameters(argumentType, parameterType)
       )
+      or
+      result = this.getADynamicTarget(i - 1) and
+      exists(Type parameterType, Type t | parameterType = this.getAParameterType(result, i) |
+        this.argumentConvConstExpr(i, t) and
+        t.isImplicitlyConvertibleTo(parameterType)
+      )
     }
 
     private Type getAParameterType(RuntimeCallable c, int i) {
@@ -840,15 +859,6 @@ private module Internal {
       )
     }
 
-    pragma[noinline]
-    private RuntimeCallable getADynamicTargetCandidateWithCompatibleArg2(int i) {
-      result = this.getADynamicTargetCandidate() and
-      exists(Type parameterType, Type t | parameterType = this.getAParameterType(result, i) |
-        this.argumentConvConstExpr(i, t) and
-        t.isImplicitlyConvertibleTo(parameterType)
-      )
-    }
-
     pragma[noinline]
     private predicate argumentConvConstExpr(int i, Type t) {
       convConstantExpr(this.getArgument(i), t)
@@ -954,7 +964,7 @@ private module Internal {
    */
   private predicate isReflectionOrDynamicCallArgumentWithTypeParameters(Type argType, Type paramType) {
     exists(DispatchReflectionOrDynamicCall call, Parameter p, int i, int j |
-      p = call.getAStaticTarget().getParameter(i) and
+      p = call.getADynamicTargetCandidate().getParameter(i) and
       (
         if p.isParams()
         then (

csharp/ql/src/semmle/code/csharp/exprs/Expr.qll

@@ -654,6 +654,14 @@ class QualifiableExpr extends Expr, @qualifiable_expr {
   predicate isConditional() { conditional_access(this) }
 }
 
+private Expr getAnAssignOrForeachChild() {
+  result = any(AssignExpr e).getLValue()
+  or
+  result = any(ForeachStmt fs).getVariableDeclTuple()
+  or
+  result = getAnAssignOrForeachChild().getAChildExpr()
+}
+
 /**
  * An expression representing a tuple, for example
  * `(1, 2)` on line 2 or `(var x, var y)` on line 5 in
@@ -678,10 +686,7 @@ class TupleExpr extends Expr, @tuple_expr {
   Expr getAnArgument() { result = getArgument(_) }
 
   /** Holds if this tuple is a read access. */
-  predicate isReadAccess() {
-    not exists(AssignExpr e | this = e.getLValue().getAChildExpr*()) and
-    not exists(ForeachStmt fs | this = fs.getVariableDeclTuple().getAChildExpr*())
-  }
+  predicate isReadAccess() { not this = getAnAssignOrForeachChild() }
 }
 
 /**

csharp/ql/test/library-tests/controlflow/splits/SplittingStressTest.cs

@@ -166,4 +166,4 @@ void M(bool b1, bool b2, bool b3, bool b4, bool b5, bool b6, bool b7, bool b8, b
             ;
         ;
     }
-}
+}

csharp/ql/test/query-tests/Concurrency/UnsafeLazyInitialization/UnsafeLazyInitialization.cs

@@ -44,7 +44,7 @@ void Fn()
             {
                 if (obj2 == null)
                 {
-                	obj2 = null;
+                    obj2 = null;
                 }
             }
         }

csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.cs

@@ -55,6 +55,6 @@ public bool IsReusable
     void OnButtonClicked()
     {
         // BAD: Use the Roslyn APIs to dynamically evaluate C#
-    	CSharpScript.EvaluateAsync(box1.Text);
+        CSharpScript.EvaluateAsync(box1.Text);
     }
 }

csharp/ql/test/query-tests/Security Features/CWE-094/CodeInjection.expected

@@ -5,8 +5,8 @@ nodes
 | CodeInjection.cs:25:23:25:45 | access to property QueryString |
 | CodeInjection.cs:31:64:31:67 | access to local variable code |
 | CodeInjection.cs:42:36:42:39 | access to local variable code |
-| CodeInjection.cs:58:33:58:41 | access to property Text |
+| CodeInjection.cs:58:36:58:44 | access to property Text |
 #select
 | CodeInjection.cs:31:64:31:67 | access to local variable code | CodeInjection.cs:25:23:25:45 | access to property QueryString | CodeInjection.cs:31:64:31:67 | access to local variable code | $@ flows to here and is compiled as code. | CodeInjection.cs:25:23:25:45 | access to property QueryString | User-provided value |
 | CodeInjection.cs:42:36:42:39 | access to local variable code | CodeInjection.cs:25:23:25:45 | access to property QueryString | CodeInjection.cs:42:36:42:39 | access to local variable code | $@ flows to here and is compiled as code. | CodeInjection.cs:25:23:25:45 | access to property QueryString | User-provided value |
-| CodeInjection.cs:58:33:58:41 | access to property Text | CodeInjection.cs:58:33:58:41 | access to property Text | CodeInjection.cs:58:33:58:41 | access to property Text | $@ flows to here and is compiled as code. | CodeInjection.cs:58:33:58:41 | access to property Text | User-provided value |
+| CodeInjection.cs:58:36:58:44 | access to property Text | CodeInjection.cs:58:36:58:44 | access to property Text | CodeInjection.cs:58:36:58:44 | access to property Text | $@ flows to here and is compiled as code. | CodeInjection.cs:58:36:58:44 | access to property Text | User-provided value |

csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.cs

@@ -9,16 +9,16 @@ public class TaintedPathHandler : IHttpHandler
     public void ProcessRequest(HttpContext ctx)
     {
         String path = ctx.Request.QueryString["page"];
-        
+
         // BAD: Uncontrolled format string.
         String.Format(path, "Do not do this");
-        
+
         // BAD: Using an IFormatProvider.
         String.Format((IFormatProvider)null, path, "Do not do this");
 
         // GOOD: Not the format string.
         String.Format("Do not do this", path);
-        
+
         // GOOD: Not the format string.
         String.Format((IFormatProvider)null, "Do not do this", path);
     }
@@ -27,7 +27,7 @@ public void ProcessRequest(HttpContext ctx)
 
     void OnButtonClicked()
     {
-    	// BAD: Uncontrolled format string.
-    	String.Format(box1.Text, "Do not do this");
+        // BAD: Uncontrolled format string.
+        String.Format(box1.Text, "Do not do this");
     }
 }

csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatString.expected

@@ -8,11 +8,11 @@ nodes
 | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path |
 | UncontrolledFormatString.cs:20:23:20:38 | "Do not do this" |
 | UncontrolledFormatString.cs:23:46:23:61 | "Do not do this" |
-| UncontrolledFormatString.cs:31:20:31:28 | access to property Text |
+| UncontrolledFormatString.cs:31:23:31:31 | access to property Text |
 | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString |
 | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format |
 #select
 | UncontrolledFormatString.cs:14:23:14:26 | access to local variable path | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | UncontrolledFormatString.cs:14:23:14:26 | access to local variable path | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | access to property QueryString |
 | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | UncontrolledFormatString.cs:17:46:17:49 | access to local variable path | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:11:23:11:45 | access to property QueryString | access to property QueryString |
-| UncontrolledFormatString.cs:31:20:31:28 | access to property Text | UncontrolledFormatString.cs:31:20:31:28 | access to property Text | UncontrolledFormatString.cs:31:20:31:28 | access to property Text | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:31:20:31:28 | access to property Text | access to property Text |
+| UncontrolledFormatString.cs:31:23:31:31 | access to property Text | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | $@ flows to here and is used as a format string. | UncontrolledFormatString.cs:31:23:31:31 | access to property Text | access to property Text |
 | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString | UncontrolledFormatStringBad.cs:12:39:12:44 | access to local variable format | $@ flows to here and is used as a format string. | UncontrolledFormatStringBad.cs:9:25:9:47 | access to property QueryString | access to property QueryString |

csharp/ql/test/query-tests/Security Features/CWE-134/UncontrolledFormatStringBad.cs

@@ -7,7 +7,7 @@ public class HttpHandler : IHttpHandler
     public void ProcessRequest(HttpContext ctx)
     {
         string format = ctx.Request.QueryString["nameformat"];
-        
+
         // BAD: Uncontrolled format string.
         FormattedName = string.Format(format, Surname, Forenames);
     }

csharp/ql/test/resources/stubs/JsonNET.cs

@@ -43,7 +43,7 @@ public class JToken : IEnumerable<JToken>, IEnumerable
 
         public static explicit operator string(JToken t) => null;
 
-        public IEnumerable<JToken> SelectToken(string s) => null; 
+        public IEnumerable<JToken> SelectToken(string s) => null;
     }
 
     public class JObject : JToken

csharp/ql/test/resources/stubs/System.Windows.cs

@@ -31,7 +31,7 @@ class TextBox : TextBoxBase
         public char PasswordChar { get; set; }
         public bool UseSystemPasswordChar { get; set; }
     }
-    
+
     class RichTextBox : TextBoxBase
     {
         public string Rtf => null;