Merge pull request #15179 from michaelnebel/modelgenrespectmanual

C#/Java: Increase precision of model generation.

Author: michaelnebel

csharp/ql/src/utils/modelgenerator/CaptureDiscardedSummaryModels.ql

@@ -11,7 +11,5 @@ import internal.CaptureModels
 import internal.CaptureSummaryFlowQuery
 
 from DataFlowTargetApi api, string noflow
-where
-  noflow = captureNoFlow(api) and
-  not api.(FlowSummaryImpl::Public::SummarizedCallable).applyManualModel()
+where noflow = captureNoFlow(api)
 select noflow order by noflow

csharp/ql/src/utils/modelgenerator/CaptureNeutralModels.ql

@@ -11,7 +11,5 @@ import internal.CaptureModels
 import internal.CaptureSummaryFlowQuery
 
 from DataFlowTargetApi api, string flow
-where
-  flow = captureFlow(api) and
-  not api.(FlowSummaryImpl::Public::SummarizedCallable).applyManualModel()
+where flow = captureFlow(api)
 select flow order by flow

csharp/ql/src/utils/modelgenerator/CaptureSummaryModels.ql

@@ -7,6 +7,7 @@ private import dotnet
 private import semmle.code.csharp.commons.Util as Util
 private import semmle.code.csharp.commons.Collections as Collections
 private import semmle.code.csharp.dataflow.internal.DataFlowDispatch
+private import semmle.code.csharp.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 private import semmle.code.csharp.frameworks.system.linq.Expressions
 import semmle.code.csharp.dataflow.internal.ExternalFlow as ExternalFlow
 import semmle.code.csharp.dataflow.internal.DataFlowImplCommon as DataFlowImplCommon
@@ -37,7 +38,10 @@ private predicate isRelevantForModels(CS::Callable api) {
   not api instanceof Util::MainMethod and
   not api instanceof CS::Destructor and
   not api instanceof CS::AnonymousFunctionExpr and
-  not api.(CS::Constructor).isParameterless()
+  not api.(CS::Constructor).isParameterless() and
+  // Disregard all APIs that have a manual model.
+  not api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()) and
+  not api = any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel())
 }
 
 /**

csharp/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll

@@ -0,0 +1,6 @@
+extensions:
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: neutralModel
+    data:
+      - [ "NoSummaries", "ManuallyModelled", "HasNeutralSummaryNoFlow", "(System.Object)", "summary", "manual"]

csharp/ql/test/utils/modelgenerator/dataflow/CaptureNeutralModels.ext.yml

@@ -0,0 +1,12 @@
+extensions:
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: summaryModel
+    data:
+      - [ "NoSummaries", "ManuallyModelled", False, "HasSummary", "(System.Object)", "", "Argument[0]", "ReturnValue", "value", "manual"]
+
+  - addsTo:
+      pack: codeql/csharp-all
+      extensible: neutralModel
+    data:
+      - [ "NoSummaries", "ManuallyModelled", "HasNeutralSummary", "(System.Object)", "summary", "manual"]

csharp/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.ext.yml

@@ -143,3 +143,22 @@ public ParameterlessConstructor()
         IsInitialized = true;
     }
 }
+
+// No models should be created, if there exist either a manual summary or neutral summary.
+public class ManuallyModelled
+{
+    public object HasSummary(object o)
+    {
+        return o;
+    }
+
+    public object HasNeutralSummary(object o)
+    {
+        return o;
+    }
+
+    public object HasNeutralSummaryNoFlow(object o)
+    {
+        return null;
+    }
+}

csharp/ql/test/utils/modelgenerator/dataflow/NoSummaries.cs

@@ -124,7 +124,6 @@
 | Summaries;TypeBasedCollection<T>;false;Add;(T);;Argument[0];Argument[this].Element;value;tb-generated |
 | Summaries;TypeBasedCollection<T>;false;AddMany;(System.Collections.Generic.IEnumerable<T>);;Argument[0].Element;Argument[this].Element;value;tb-generated |
 | Summaries;TypeBasedCollection<T>;false;First;();;Argument[this].Element;ReturnValue;value;tb-generated |
-| Summaries;TypeBasedCollection<T>;false;GetEnumerator;();;Argument[this].Element;ReturnValue.SyntheticField[ArgType0];value;tb-generated |
 | Summaries;TypeBasedCollection<T>;false;GetMany;();;Argument[this].Element;ReturnValue.Element;value;tb-generated |
 | Summaries;TypeBasedComplex<T>;false;AddMany;(System.Collections.Generic.IEnumerable<T>);;Argument[0].Element;Argument[this].SyntheticField[ArgType0];value;tb-generated |
 | Summaries;TypeBasedComplex<T>;false;Apply;(System.Func<T,System.Int32>);;Argument[this].SyntheticField[ArgType0];Argument[0].Parameter[0];value;tb-generated |

csharp/ql/test/utils/modelgenerator/typebasedflow/CaptureTypeBasedSummaryModels.expected

@@ -5,6 +5,7 @@
 private import java as J
 private import semmle.code.java.dataflow.internal.DataFlowPrivate
 private import semmle.code.java.dataflow.internal.ContainerFlow as ContainerFlow
+private import semmle.code.java.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 private import semmle.code.java.dataflow.internal.ModelExclusions
 private import semmle.code.java.dataflow.DataFlow as Df
 private import semmle.code.java.dataflow.SSA as Ssa
@@ -21,6 +22,8 @@ class Type = J::Type;
 
 class Unit = J::Unit;
 
+class Callable = J::Callable;
+
 private J::Method superImpl(J::Method m) {
   result = m.getAnOverride() and
   not exists(result.getAnOverride()) and
@@ -35,15 +38,19 @@ private predicate isInfrequentlyUsed(J::CompilationUnit cu) {
 /**
  * Holds if it is relevant to generate models for `api`.
  */
-private predicate isRelevantForModels(J::Callable api) {
+private predicate isRelevantForModels(Callable api) {
   not isUninterestingForModels(api) and
-  not isInfrequentlyUsed(api.getCompilationUnit())
+  not isInfrequentlyUsed(api.getCompilationUnit()) and
+  // Disregard all APIs that have a manual model.
+  not api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()).asCallable() and
+  not api =
+    any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel()).asCallable()
 }
 
 /**
  * Holds if it is relevant to generate models for `api` based on data flow analysis.
  */
-predicate isRelevantForDataFlowModels(J::Callable api) {
+predicate isRelevantForDataFlowModels(Callable api) {
   isRelevantForModels(api) and
   (not api.getDeclaringType() instanceof J::Interface or exists(api.getBody()))
 }
@@ -56,7 +63,7 @@ predicate isRelevantForTypeBasedFlowModels = isRelevantForModels/1;
  * In the Standard library and 3rd party libraries it the Callables that can be called
  * from outside the library itself.
  */
-class TargetApiSpecific extends J::Callable {
+class TargetApiSpecific extends Callable {
   TargetApiSpecific() {
     this.isPublic() and
     this.fromSource() and
@@ -66,6 +73,15 @@ class TargetApiSpecific extends J::Callable {
     ) and
     isRelevantForModels(this)
   }
+
+  /**
+   * Gets the callable that a model will be lifted to, if any.
+   */
+  Callable lift() {
+    exists(Method m | m = superImpl(this) and m.fromSource() | result = m)
+    or
+    not exists(superImpl(this)) and result = this
+  }
 }
 
 private string isExtensible(J::RefType ref) {
@@ -79,9 +95,7 @@ private string typeAsModel(J::RefType type) {
 }
 
 private J::RefType bestTypeForModel(TargetApiSpecific api) {
-  if exists(superImpl(api))
-  then superImpl(api).fromSource() and result = superImpl(api).getDeclaringType()
-  else result = api.getDeclaringType()
+  result = api.lift().getDeclaringType()
 }
 
 /**
@@ -195,7 +209,7 @@ string returnNodeAsOutput(DataFlowImplCommon::ReturnNodeExt node) {
 /**
  * Gets the enclosing callable of `ret`.
  */
-J::Callable returnNodeEnclosingCallable(DataFlowImplCommon::ReturnNodeExt ret) {
+Callable returnNodeEnclosingCallable(DataFlowImplCommon::ReturnNodeExt ret) {
   result = DataFlowImplCommon::getNodeEnclosingCallable(ret).asCallable()
 }
 

java/ql/src/utils/modelgenerator/internal/CaptureModelsSpecific.qll

@@ -73,10 +73,11 @@ string captureFlow(DataFlowTargetApi api) {
 }
 
 /**
- * Gets the neutral summary for `api`, if any.
- * A neutral model is generated, if there does not exist any summary model.
+ * Gets the neutral summary model for `api`, if any.
+ * A neutral summary model is generated, if we are not generating
+ * a summary model that applies to `api`.
  */
 string captureNoFlow(DataFlowTargetApi api) {
-  not exists(captureFlow(api)) and
+  not exists(DataFlowTargetApi api0 | exists(captureFlow(api0)) and api0.lift() = api.lift()) and
   result = ModelPrinting::asNeutralSummaryModel(api)
 }

java/ql/src/utils/modelgenerator/internal/CaptureSummaryFlowQuery.qll

@@ -22,6 +22,7 @@
 | p;Joiner;false;setEmptyValue;(CharSequence);;Argument[0];Argument[this];taint;df-generated |
 | p;Joiner;false;setEmptyValue;(CharSequence);;Argument[this];ReturnValue;value;df-generated |
 | p;Joiner;false;toString;();;Argument[this];ReturnValue;taint;df-generated |
+| p;MultipleImpl2$IInterface;true;m;(Object);;Argument[0];ReturnValue;taint;df-generated |
 | p;MultipleImpls$Strat2;true;getValue;();;Argument[this];ReturnValue;taint;df-generated |
 | p;MultipleImpls$Strategy;true;doSomething;(String);;Argument[0];Argument[this];taint;df-generated |
 | p;MultipleImpls$Strategy;true;doSomething;(String);;Argument[0];ReturnValue;taint;df-generated |

java/ql/test/utils/modelgenerator/dataflow/CaptureSummaryModels.expected

@@ -0,0 +1,23 @@
+package p;
+
+class MultipleImpl2 {
+
+    // Multiple implementations of the same interface.
+    // This is used to test that we only generate a summary model and
+    // not neutral summary model for `IInterface.m`.
+    public interface IInterface {
+        Object m(Object value);
+    }
+
+    public class Impl1 implements IInterface {
+        public Object m(Object value) {
+            return null;
+        }
+    }
+
+    public class Impl2 implements IInterface {
+        public Object m(Object value) {
+            return value;
+        }
+    }
+}