Merge pull request #16647 from michaelnebel/csharp/idempotentsummarygeneration

C#: Make summary generation idempotent.

Author: michaelnebel

csharp/ql/lib/semmle/code/csharp/dataflow/internal/DataFlowDispatch.qll

@@ -10,8 +10,8 @@ private import semmle.code.csharp.frameworks.system.Collections
 private import semmle.code.csharp.frameworks.system.collections.Generic
 
 /**
- * Gets a source declaration of callable `c` that has a body or has
- * a flow summary.
+ * Gets a source declaration of callable `c` that has a body and is
+ * defined in source.
  */
 Callable getCallableForDataFlow(Callable c) {
   result = c.getUnboundDeclaration() and
@@ -269,13 +269,19 @@ class NonDelegateDataFlowCall extends DataFlowCall, TNonDelegateCall {
   override DataFlowCallable getARuntimeTarget() {
     result.asCallable() = getCallableForDataFlow(dc.getADynamicTarget())
     or
-    exists(Callable c, boolean static |
-      result.asSummarizedCallable() = c and
-      c = this.getATarget(static)
+    // Only use summarized callables with generated summaries in case
+    // we are not able to dispatch to a source declaration.
+    exists(FlowSummary::SummarizedCallable sc, boolean static |
+      result.asSummarizedCallable() = sc and
+      sc = this.getATarget(static) and
+      not (
+        sc.applyGeneratedModel() and
+        dc.getADynamicTarget().getUnboundDeclaration().getFile().fromSource()
+      )
     |
       static = false
       or
-      static = true and not c instanceof RuntimeCallable
+      static = true and not sc instanceof RuntimeCallable
     )
   }
 

csharp/ql/lib/semmle/code/csharp/dataflow/internal/ExternalFlow.qll

@@ -556,9 +556,9 @@ private predicate interpretNeutral(UnboundCallable c, string kind, string proven
 private class SummarizedCallableAdapter extends SummarizedCallable {
   SummarizedCallableAdapter() {
     exists(Provenance provenance | interpretSummary(this, _, _, _, provenance, _) |
-      not this.hasBody()
+      not this.fromSource()
       or
-      this.hasBody() and provenance.isManual()
+      this.fromSource() and provenance.isManual()
     )
   }
 

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs

@@ -200,31 +200,25 @@ void M2()
         void M3()
         {
             var o1 = new object();
-            Sink(MixedFlowArgs(o1, null));
+            Sink(Library.MixedFlowArgs(o1, null));
 
             var o2 = new object();
-            Sink(MixedFlowArgs(null, o2));
+            Sink(Library.MixedFlowArgs(null, o2));
         }
 
         void M4()
         {
             var o1 = new object();
-            Sink(GeneratedFlowWithGeneratedNeutral(o1));
+            Sink(Library.GeneratedFlowWithGeneratedNeutral(o1));
 
             var o2 = new object();
-            Sink(GeneratedFlowWithManualNeutral(o2)); // no flow because the modelled method has a manual neutral summary model
+            Sink(Library.GeneratedFlowWithManualNeutral(o2)); // no flow because the modelled method has a manual neutral summary model
         }
 
         object GeneratedFlow(object o) => throw null;
 
         object GeneratedFlowArgs(object o1, object o2) => throw null;
 
-        object MixedFlowArgs(object o1, object o2) => throw null;
-
-        object GeneratedFlowWithGeneratedNeutral(object o) => throw null;
-
-        object GeneratedFlowWithManualNeutral(object o) => throw null;
-
         static void Sink(object o) { }
     }
 
@@ -268,4 +262,33 @@ void M1(MyInlineArray a)
 
         static void Sink(object o) { }
     }
+
+    public class J
+    {
+        public virtual object Prop1 { get; }
+
+        public virtual void SetProp1(object o) => throw null;
+
+        public virtual object Prop2 { get; }
+
+        public virtual void SetProp2(object o) => throw null;
+
+        void M1()
+        {
+            var j = new object();
+            SetProp1(j);
+            // flow as there is a manual summary.
+            Sink(this.Prop1);
+        }
+
+        void M2()
+        {
+            var j = new object();
+            SetProp2(j);
+            // no flow as there is only a generated summary and source code is available.
+            Sink(this.Prop2);
+        }
+
+        static void Sink(object o) { }
+    }
 }

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.cs_

@@ -0,0 +1,17 @@
+using System;
+
+namespace My.Qltest
+{
+    public class Library
+    {
+        public static object MixedFlowArgs(object o1, object o2) => throw null;
+
+        public static object GeneratedFlowWithGeneratedNeutral(object o) => throw null;
+
+        public static object GeneratedFlowWithManualNeutral(object o) => throw null;
+
+        public static object StepArgReturnGenerated(object x) => throw null;
+
+        public static object StepArgReturnGeneratedIgnored(object x) => throw null;
+    }
+}

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.dll

@@ -27,16 +27,20 @@ extensions:
       - ["My.Qltest", "G", false, "GeneratedFlow", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
       - ["My.Qltest", "G", false, "GeneratedFlowArgs", "(System.Object,System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
       - ["My.Qltest", "G", false, "GeneratedFlowArgs", "(System.Object,System.Object)", "", "Argument[1]", "ReturnValue", "value", "df-generated"]
-      - ["My.Qltest", "G", false, "MixedFlowArgs", "(System.Object,System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
-      - ["My.Qltest", "G", false, "MixedFlowArgs", "(System.Object,System.Object)", "", "Argument[1]", "ReturnValue", "value", "manual"]
-      - ["My.Qltest", "G", false, "GeneratedFlowWithGeneratedNeutral", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
-      - ["My.Qltest", "G", false, "GeneratedFlowWithManualNeutral", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
+      - ["My.Qltest", "Library", false, "MixedFlowArgs", "(System.Object,System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
+      - ["My.Qltest", "Library", false, "MixedFlowArgs", "(System.Object,System.Object)", "", "Argument[1]", "ReturnValue", "value", "manual"]
+      - ["My.Qltest", "Library", false, "GeneratedFlowWithGeneratedNeutral", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
+      - ["My.Qltest", "Library", false, "GeneratedFlowWithManualNeutral", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "df-generated"]
       - ["My.Qltest", "HE", false, "ExtensionMethod", "(My.Qltest.HI)", "", "Argument[0]", "ReturnValue", "value", "manual"]
       - ["My.Qltest", "I", false, "GetFirst", "(My.Qltest.MyInlineArray)", "", "Argument[0].Element", "ReturnValue", "value", "manual"]
+      - ["My.Qltest", "J", false, "get_Prop1", "()", "", "Argument[this]", "ReturnValue", "value", "manual"]
+      - ["My.Qltest", "J", false, "SetProp1", "(System.Object)", "", "Argument[0]", "Argument[this]", "value", "manual"]
+      - ["My.Qltest", "J", false, "get_Prop2", "()", "", "Argument[this]", "ReturnValue", "value", "df-generated"]
+      - ["My.Qltest", "J", false, "SetProp2", "(System.Object)", "", "Argument[0]", "Argument[this]", "value", "manual"]
   - addsTo:
       pack: codeql/csharp-all
       extensible: neutralModel
       # "namespace", "type", "name", "signature", "kind", "provenance"
     data:
-      - ["My.Qltest", "G", "GeneratedFlowWithGeneratedNeutral", "(System.Object)", "summary", "df-generated"]
-      - ["My.Qltest", "G", "GeneratedFlowWithManualNeutral", "(System.Object)", "summary", "manual"]
+      - ["My.Qltest", "Library", "GeneratedFlowWithGeneratedNeutral", "(System.Object)", "summary", "df-generated"]
+      - ["My.Qltest", "Library", "GeneratedFlowWithManualNeutral", "(System.Object)", "summary", "manual"]

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.expected

@@ -20,21 +20,6 @@ module TaintConfig implements DataFlow::ConfigSig {
 
 module Taint = TaintTracking::Global<TaintConfig>;
 
-/**
- * Emulate that methods with summaries do not have a body.
- * This is relevant for dataflow analysis using summaries with a generated like
- * provenance as generated summaries are only applied, if a
- * callable does not have a body.
- */
-private class MethodsWithGeneratedModels extends Method {
-  MethodsWithGeneratedModels() {
-    this.hasFullyQualifiedName("My.Qltest", "G",
-      ["MixedFlowArgs", "GeneratedFlowWithGeneratedNeutral", "GeneratedFlowWithManualNeutral"])
-  }
-
-  override predicate hasBody() { none() }
-}
-
 from Taint::PathNode source, Taint::PathNode sink
 where Taint::flowPath(source, sink)
 select sink, source, sink, "$@", source, source.toString()

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ext.yml

@@ -44,10 +44,10 @@ void Foo()
             new Sub().StepOverride("string");
 
             object arg4 = new object();
-            this.StepArgQualGenerated(arg4);
-            
+            Library.StepArgReturnGenerated(arg4);
+
             object arg5 = new object();
-            this.StepArgQualGeneratedIgnored(arg5);
+            Library.StepArgReturnGeneratedIgnored(arg5);
         }
 
         object StepArgRes(object x) { return null; }
@@ -56,10 +56,6 @@ void StepArgArg(object @in, object @out) { }
 
         void StepArgQual(object x) { }
 
-        void StepArgQualGenerated(object x) { }
-
-        void StepArgQualGeneratedIgnored(object x) { }
-
         object StepQualRes() { return null; }
 
         void StepQualArg(object @out) { }

csharp/ql/test/library-tests/dataflow/external-models/ExternalFlow.ql

@@ -11,12 +11,12 @@ summaryThroughStep
 | Steps.cs:41:29:41:29 | 0 | Steps.cs:41:13:41:30 | call to method StepGeneric | true |
 | Steps.cs:42:30:42:34 | false | Steps.cs:42:13:42:35 | call to method StepGeneric2<Boolean> | true |
 | Steps.cs:44:36:44:43 | "string" | Steps.cs:44:13:44:44 | call to method StepOverride | true |
-| Steps.cs:47:39:47:42 | access to local variable arg4 | Steps.cs:47:13:47:16 | [post] this access | false |
+| Steps.cs:47:44:47:47 | access to local variable arg4 | Steps.cs:47:13:47:48 | call to method StepArgReturnGenerated | false |
 summaryGetterStep
-| Steps.cs:28:13:28:16 | this access | Steps.cs:28:13:28:34 | call to method StepFieldGetter | Steps.cs:67:13:67:17 | field Field |
-| Steps.cs:32:13:32:16 | this access | Steps.cs:32:13:32:37 | call to method StepPropertyGetter | Steps.cs:73:13:73:20 | property Property |
+| Steps.cs:28:13:28:16 | this access | Steps.cs:28:13:28:34 | call to method StepFieldGetter | Steps.cs:63:13:63:17 | field Field |
+| Steps.cs:32:13:32:16 | this access | Steps.cs:32:13:32:37 | call to method StepPropertyGetter | Steps.cs:69:13:69:20 | property Property |
 | Steps.cs:36:13:36:16 | this access | Steps.cs:36:13:36:36 | call to method StepElementGetter | file://:0:0:0:0 | element |
 summarySetterStep
-| Steps.cs:30:34:30:34 | 0 | Steps.cs:30:13:30:16 | [post] this access | Steps.cs:67:13:67:17 | field Field |
-| Steps.cs:34:37:34:37 | 0 | Steps.cs:34:13:34:16 | [post] this access | Steps.cs:73:13:73:20 | property Property |
+| Steps.cs:30:34:30:34 | 0 | Steps.cs:30:13:30:16 | [post] this access | Steps.cs:63:13:63:17 | field Field |
+| Steps.cs:34:37:34:37 | 0 | Steps.cs:34:13:34:16 | [post] this access | Steps.cs:69:13:69:20 | property Property |
 | Steps.cs:38:36:38:36 | 0 | Steps.cs:38:13:38:16 | [post] this access | file://:0:0:0:0 | element |