Merge pull request #16374 from michaelnebel/java/narrowsuperimpl

Java: Improve finding best type for models and lifting.

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

@@ -7,7 +7,7 @@ private import CaptureModelsSpecific
 private import CaptureModelsPrinting
 
 class DataFlowTargetApi extends TargetApiSpecific {
-  DataFlowTargetApi() { isRelevantForDataFlowModels(this) }
+  DataFlowTargetApi() { not isUninterestingForDataFlowModels(this) }
 }
 
 private module Printing implements PrintingSig {

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

@@ -51,16 +51,18 @@ private predicate isRelevantForModels(CS::Callable api) {
 }
 
 /**
- * Holds if it is relevant to generate models for `api` based on data flow analysis.
+ * Holds if it is irrelevant to generate models for `api` based on data flow analysis.
+ *
+ * This serves as an extra filter for the `relevant` predicate.
  */
-predicate isRelevantForDataFlowModels(CS::Callable api) {
-  isRelevantForModels(api) and not isHigherOrder(api)
-}
+predicate isUninterestingForDataFlowModels(CS::Callable api) { isHigherOrder(api) }
 
 /**
- * Holds if it is relevant to generate models for `api` based on its type.
+ * Holds if it is irrelevant to generate models for `api` based on type-based analysis.
+ *
+ * This serves as an extra filter for the `relevant` predicate.
  */
-predicate isRelevantForTypeBasedFlowModels = isRelevantForModels/1;
+predicate isUninterestingForTypeBasedFlowModels(CS::Callable api) { none() }
 
 /**
  * A class of callables that are relevant generating summary, source and sinks models for.
@@ -71,7 +73,8 @@ predicate isRelevantForTypeBasedFlowModels = isRelevantForModels/1;
 class TargetApiSpecific extends CS::Callable {
   TargetApiSpecific() {
     this.fromSource() and
-    this.isUnboundDeclaration()
+    this.isUnboundDeclaration() and
+    isRelevantForModels(this)
   }
 }
 

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

@@ -190,7 +190,7 @@ private module ModelPrinting = PrintingImpl<Printing>;
  * on the Theorems for Free approach.
  */
 class TypeBasedFlowTargetApi extends Specific::TargetApiSpecific {
-  TypeBasedFlowTargetApi() { Specific::isRelevantForTypeBasedFlowModels(this) }
+  TypeBasedFlowTargetApi() { not Specific::isUninterestingForTypeBasedFlowModels(this) }
 
   /**
    * Gets the string representation of all type based summaries for `this`

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

@@ -7,7 +7,7 @@ private import CaptureModelsSpecific
 private import CaptureModelsPrinting
 
 class DataFlowTargetApi extends TargetApiSpecific {
-  DataFlowTargetApi() { isRelevantForDataFlowModels(this) }
+  DataFlowTargetApi() { not isUninterestingForDataFlowModels(this) }
 }
 
 private module Printing implements PrintingSig {

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

@@ -24,38 +24,59 @@ class Unit = J::Unit;
 
 class Callable = J::Callable;
 
-private J::Method superImpl(J::Method m) {
-  result = m.getAnOverride() and
-  not exists(result.getAnOverride()) and
-  not m instanceof J::ToStringMethod
-}
-
 private predicate isInfrequentlyUsed(J::CompilationUnit cu) {
   cu.getPackage().getName().matches("javax.swing%") or
   cu.getPackage().getName().matches("java.awt%")
 }
 
-/**
- * Holds if it is relevant to generate models for `api`.
- */
-private predicate isRelevantForModels(Callable api) {
+private predicate relevant(Callable api) {
+  api.isPublic() and
+  api.getDeclaringType().isPublic() and
+  api.fromSource() and
   not isUninterestingForModels(api) and
-  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()
+  not isInfrequentlyUsed(api.getCompilationUnit())
+}
+
+private J::Method getARelevantOverride(J::Method m) {
+  result = m.getAnOverride() and
+  relevant(result) and
+  // Other exclusions for overrides.
+  not m instanceof J::ToStringMethod
 }
 
 /**
- * Holds if it is relevant to generate models for `api` based on data flow analysis.
+ * Gets the super implementation of `m` if it is relevant.
+ * If such a super implementations does not exist, returns `m` if it is relevant.
  */
-predicate isRelevantForDataFlowModels(Callable api) {
-  isRelevantForModels(api) and
-  (not api.getDeclaringType() instanceof J::Interface or exists(api.getBody()))
+private J::Callable liftedImpl(J::Callable m) {
+  (
+    result = getARelevantOverride(m)
+    or
+    result = m and relevant(m)
+  ) and
+  not exists(getARelevantOverride(result))
 }
 
-predicate isRelevantForTypeBasedFlowModels = isRelevantForModels/1;
+private predicate hasManualModel(Callable api) {
+  api = any(FlowSummaryImpl::Public::SummarizedCallable sc | sc.applyManualModel()).asCallable() or
+  api = any(FlowSummaryImpl::Public::NeutralSummaryCallable sc | sc.hasManualModel()).asCallable()
+}
+
+/**
+ * Holds if it is irrelevant to generate models for `api` based on data flow analysis.
+ *
+ * This serves as an extra filter for the `relevant` predicate.
+ */
+predicate isUninterestingForDataFlowModels(Callable api) {
+  api.getDeclaringType() instanceof J::Interface and not exists(api.getBody())
+}
+
+/**
+ * Holds if it is irrelevant to generate models for `api` based on type-based analysis.
+ *
+ * This serves as an extra filter for the `relevant` predicate.
+ */
+predicate isUninterestingForTypeBasedFlowModels(Callable api) { none() }
 
 /**
  * A class of Callables that are relevant for generating summary, source and sinks models for.
@@ -64,63 +85,54 @@ predicate isRelevantForTypeBasedFlowModels = isRelevantForModels/1;
  * from outside the library itself.
  */
 class TargetApiSpecific extends Callable {
+  private Callable lift;
+
   TargetApiSpecific() {
-    this.isPublic() and
-    this.fromSource() and
-    (
-      this.getDeclaringType().isPublic() or
-      superImpl(this).getDeclaringType().isPublic()
-    ) and
-    isRelevantForModels(this)
+    lift = liftedImpl(this) and
+    not hasManualModel(lift)
   }
 
   /**
-   * Gets the callable that a model will be lifted to, if any.
+   * Gets the callable that a model will be lifted to.
    */
-  Callable lift() {
-    exists(Method m | m = superImpl(this) and m.fromSource() | result = m)
-    or
-    not exists(superImpl(this)) and result = this
-  }
+  Callable lift() { result = lift }
 }
 
-private string isExtensible(J::RefType ref) {
-  if ref.isFinal() then result = "false" else result = "true"
-}
-
-private string typeAsModel(J::RefType type) {
-  result =
-    type.getCompilationUnit().getPackage().getName() + ";" +
-      type.getErasure().(J::RefType).nestedName()
-}
-
-private J::RefType bestTypeForModel(TargetApiSpecific api) {
-  result = api.lift().getDeclaringType()
+private string isExtensible(Callable c) {
+  if c.getDeclaringType().isFinal() then result = "false" else result = "true"
 }
 
 /**
- * Returns the appropriate type name for the model. Either the type
- * declaring the method or the supertype introducing the method.
+ * Returns the appropriate type name for the model.
  */
-private string typeAsSummaryModel(TargetApiSpecific api) {
-  result = typeAsModel(bestTypeForModel(api))
+private string typeAsModel(Callable c) {
+  exists(RefType type | type = c.getDeclaringType() |
+    result =
+      type.getCompilationUnit().getPackage().getName() + ";" +
+        type.getErasure().(J::RefType).nestedName()
+  )
 }
 
-private predicate partialModel(TargetApiSpecific api, string type, string name, string parameters) {
-  type = typeAsSummaryModel(api) and
-  name = api.getName() and
-  parameters = ExternalFlow::paramsString(api)
+private predicate partialLiftedModel(
+  TargetApiSpecific api, string type, string extensible, string name, string parameters
+) {
+  exists(Callable c | c = api.lift() |
+    type = typeAsModel(c) and
+    extensible = isExtensible(c) and
+    name = c.getName() and
+    parameters = ExternalFlow::paramsString(c)
+  )
 }
 
 /**
  * Computes the first 6 columns for MaD rows.
  */
 string asPartialModel(TargetApiSpecific api) {
-  exists(string type, string name, string parameters |
-    partialModel(api, type, name, parameters) and
+  exists(string type, string extensible, string name, string parameters |
+    partialLiftedModel(api, type, extensible, name, parameters) and
     result =
       type + ";" //
-        + isExtensible(bestTypeForModel(api)) + ";" //
+        + extensible + ";" //
         + name + ";" //
         + parameters + ";" //
         + /* ext + */ ";" //
@@ -132,7 +144,7 @@ string asPartialModel(TargetApiSpecific api) {
  */
 string asPartialNeutralModel(TargetApiSpecific api) {
   exists(string type, string name, string parameters |
-    partialModel(api, type, name, parameters) and
+    partialLiftedModel(api, type, _, name, parameters) and
     result =
       type + ";" //
         + name + ";" //
@@ -228,6 +240,15 @@ predicate sinkModelSanitizer(DataFlow::Node node) {
   )
 }
 
+private class ManualNeutralSinkCallable extends Callable {
+  ManualNeutralSinkCallable() {
+    this =
+      any(FlowSummaryImpl::Public::NeutralCallable nc |
+        nc.hasManualModel() and nc.getKind() = "sink"
+      ).asCallable()
+  }
+}
+
 /**
  * Holds if `source` is an api entrypoint relevant for creating sink models.
  */
@@ -236,14 +257,15 @@ predicate apiSource(DataFlow::Node source) {
     source.asExpr().(J::FieldAccess).isOwnFieldAccess() or
     source instanceof DataFlow::ParameterNode
   ) and
-  source.getEnclosingCallable().isPublic() and
-  exists(J::RefType t |
-    t = source.getEnclosingCallable().getDeclaringType().getAnAncestor() and
-    not t instanceof J::TypeObject and
-    t.isPublic()
-  ) and
-  isRelevantForModels(source.getEnclosingCallable()) and
-  exists(asPartialModel(source.getEnclosingCallable()))
+  exists(Callable enclosing | enclosing = source.getEnclosingCallable() |
+    exists(liftedImpl(enclosing)) and
+    not enclosing instanceof ManualNeutralSinkCallable and
+    exists(J::RefType t |
+      t = enclosing.getDeclaringType().getAnAncestor() and
+      not t instanceof J::TypeObject and
+      t.isPublic()
+    )
+  )
 }
 
 /**

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

@@ -296,7 +296,7 @@ private module ModelPrinting = PrintingImpl<Printing>;
  * on the Theorems for Free approach.
  */
 class TypeBasedFlowTargetApi extends Specific::TargetApiSpecific {
-  TypeBasedFlowTargetApi() { Specific::isRelevantForTypeBasedFlowModels(this) }
+  TypeBasedFlowTargetApi() { not Specific::isUninterestingForTypeBasedFlowModels(this) }
 
   /**
    * Gets the string representation of all type based summaries for `this`

java/ql/test/utils/modelgenerator/dataflow/p/AbstractImplOfExternalSPI.java

@@ -5,9 +5,9 @@ import java.io.FileFilter;
 
 public abstract class AbstractImplOfExternalSPI implements FileFilter {
 
-    @Override
-    public boolean accept(File pathname) {
-        return false;
-    }
-
-}
+  // neutral=p;AbstractImplOfExternalSPI;accept;(File);summary;df-generated
+  @Override
+  public boolean accept(File pathname) {
+    return false;
+  }
+}

java/ql/test/utils/modelgenerator/dataflow/p/ImplOfExternalSPI.java

@@ -6,14 +6,15 @@ import java.nio.file.Files;
 
 public class ImplOfExternalSPI extends AbstractImplOfExternalSPI {
 
-    @Override
-    public boolean accept(File pathname) {
-        try {
-            Files.createFile(pathname.toPath());
-        } catch (IOException e) {
-            e.printStackTrace();
-        }
-        return false;
+  // sink=p;AbstractImplOfExternalSPI;true;accept;(File);;Argument[0];path-injection;df-generated
+  // neutral=p;AbstractImplOfExternalSPI;accept;(File);summary;df-generated
+  @Override
+  public boolean accept(File pathname) {
+    try {
+      Files.createFile(pathname.toPath());
+    } catch (IOException e) {
+      e.printStackTrace();
     }
-
-}
+    return false;
+  }
+}

java/ql/test/utils/modelgenerator/dataflow/p/Inheritance.java

@@ -0,0 +1,91 @@
+package p;
+
+public class Inheritance {
+  private abstract class BasePrivate {
+    public abstract String id(String s);
+  }
+
+  public abstract class BasePublic {
+    public abstract String id(String s);
+  }
+
+  public class AImplBasePrivateImpl extends BasePrivate {
+    // summary=p;Inheritance$AImplBasePrivateImpl;true;id;(String);;Argument[0];ReturnValue;taint;df-generated
+    @Override
+    public String id(String s) {
+      return s;
+    }
+  }
+
+  public class AImplBasePublic extends BasePublic {
+    // summary=p;Inheritance$BasePublic;true;id;(String);;Argument[0];ReturnValue;taint;df-generated
+    @Override
+    public String id(String s) {
+      return s;
+    }
+  }
+
+  private interface IPrivate1 {
+    String id(String s);
+  }
+
+  private interface IPrivate2 {
+    String id(String s);
+  }
+
+  public interface IPublic1 {
+    String id(String s);
+  }
+
+  public interface IPublic2 {
+    String id(String s);
+  }
+
+  public abstract class B implements IPublic1 {
+    public abstract String id(String s);
+  }
+
+  public abstract class C implements IPrivate1 {
+    public abstract String id(String s);
+  }
+
+  private abstract class D implements IPublic2 {
+    public abstract String id(String s);
+  }
+
+  private abstract class E implements IPrivate2 {
+    public abstract String id(String s);
+  }
+
+  public class BImpl extends B {
+    // summary=p;Inheritance$IPublic1;true;id;(String);;Argument[0];ReturnValue;taint;df-generated
+    @Override
+    public String id(String s) {
+      return s;
+    }
+  }
+
+  public class CImpl extends C {
+    // summary=p;Inheritance$C;true;id;(String);;Argument[0];ReturnValue;taint;df-generated
+    @Override
+    public String id(String s) {
+      return s;
+    }
+  }
+
+  public class DImpl extends D {
+    // summary=p;Inheritance$IPublic2;true;id;(String);;Argument[0];ReturnValue;taint;df-generated
+    @Override
+    public String id(String s) {
+      return s;
+    }
+  }
+
+  public class EImpl extends E {
+    // summary=p;Inheritance$EImpl;true;id;(String);;Argument[0];ReturnValue;taint;df-generated
+    @Override
+    public String id(String s) {
+      return s;
+    }
+  }
+}

java/ql/test/utils/modelgenerator/dataflow/p/MultipleImpls.java

@@ -18,6 +18,7 @@ public class MultipleImpls {
   // implements in different library should not count as impl
   public static class Strat3 implements Callable<String> {
 
+    // neutral=p;MultipleImpls$Strat3;call;();summary;df-generated
     @Override
     public String call() throws Exception {
       return null;