Java: Redesign and reimplement variable capture flow.

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

@@ -156,7 +156,7 @@ private module DispatchImpl {
 
   private module Unification = MkUnification<unificationTargetLeft/1, unificationTargetRight/1>;
 
-  private int parameterPosition() { result in [-2, -1, any(Parameter p).getPosition()] }
+  private int parameterPosition() { result in [-1, any(Parameter p).getPosition()] }
 
   /** A parameter position represented by an integer. */
   class ParameterPosition extends int {

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowNodes.qll

@@ -56,8 +56,7 @@ private module Cached {
     } or
     TFlowSummaryNode(FlowSummaryImpl::Private::SummaryNode sn) or
     TFieldValueNode(Field f) or
-    TParameterPostUpdate(CapturedParameter p) or
-    TClosureNode(CaptureFlow::ClosureNode cn)
+    TCaptureNode(CaptureFlow::SynthesizedCaptureNode cn)
 
   cached
   newtype TContent =
@@ -131,7 +130,7 @@ module Public {
       or
       result = this.(ImplicitPostUpdateNode).getPreUpdateNode().getType()
       or
-      result = this.(ClosureNode).getTypeImpl()
+      result = this.(CaptureNode).getTypeImpl()
       or
       result = this.(FieldValueNode).getField().getType()
     }
@@ -365,10 +364,6 @@ private class ImplicitExprPostUpdate extends ImplicitPostUpdateNode, TImplicitEx
   }
 }
 
-private class ParameterPostUpdate extends ImplicitPostUpdateNode, TParameterPostUpdate {
-  override Node getPreUpdateNode() { this = TParameterPostUpdate(result.asParameter()) }
-}
-
 module Private {
   private import DataFlowDispatch
 
@@ -382,7 +377,7 @@ module Private {
     result.asCallable() = n.(MallocNode).getClassInstanceExpr().getEnclosingCallable() or
     result = nodeGetEnclosingCallable(n.(ImplicitPostUpdateNode).getPreUpdateNode()) or
     result.asSummarizedCallable() = n.(FlowSummaryNode).getSummarizedCallable() or
-    result = n.(ClosureNode).getCaptureFlowNode().getEnclosingCallable() or
+    result.asCallable() = n.(CaptureNode).getSynthesizedCaptureNode().getEnclosingCallable() or
     result.asFieldScope() = n.(FieldValueNode).getField()
   }
 
@@ -411,8 +406,6 @@ module Private {
       this = getInstanceArgument(_)
       or
       this.(FlowSummaryNode).isArgumentOf(_, _)
-      or
-      this.(ClosureNode).isArgumentOf(_, _)
     }
 
     /**
@@ -430,8 +423,6 @@ module Private {
       pos = -1 and this = getInstanceArgument(call.asCall())
       or
       this.(FlowSummaryNode).isArgumentOf(call, pos)
-      or
-      this.(ClosureNode).isArgumentOf(call, pos)
     }
 
     /** Gets the call in which this node is an argument. */
@@ -511,29 +502,16 @@ module Private {
    * A synthesized data flow node representing a closure object that tracks
    * captured variables.
    */
-  class ClosureNode extends Node, TClosureNode {
-    CaptureFlow::ClosureNode getCaptureFlowNode() { this = TClosureNode(result) }
+  class CaptureNode extends Node, TCaptureNode {
+    CaptureFlow::SynthesizedCaptureNode getSynthesizedCaptureNode() { this = TCaptureNode(result) }
 
-    override Location getLocation() { result = this.getCaptureFlowNode().getLocation() }
+    override Location getLocation() { result = this.getSynthesizedCaptureNode().getLocation() }
 
-    override string toString() { result = this.getCaptureFlowNode().toString() }
-
-    predicate isParameter(DataFlowCallable c) { this.getCaptureFlowNode().isParameter(c) }
-
-    predicate isArgumentOf(DataFlowCall call, int pos) {
-      this.getCaptureFlowNode().isArgument(call) and pos = -2
-    }
+    override string toString() { result = this.getSynthesizedCaptureNode().toString() }
 
+    // TODO: expose hasTypeProxy(var / callable)
     Type getTypeImpl() { result instanceof TypeObject }
   }
-
-  class ClosureParameterNode extends ParameterNode, ClosureNode {
-    ClosureParameterNode() { this.isParameter(_) }
-
-    override predicate isParameterOf(DataFlowCallable c, int pos) {
-      this.isParameter(c) and pos = -2
-    }
-  }
 }
 
 private import Private
@@ -564,11 +542,12 @@ private class SummaryPostUpdateNode extends FlowSummaryNode, PostUpdateNode {
   override Node getPreUpdateNode() { result = pre }
 }
 
-private class ClosurePostUpdateNode extends PostUpdateNode, ClosureNode {
-  private ClosureNode pre;
+private class CapturePostUpdateNode extends PostUpdateNode, CaptureNode {
+  private CaptureNode pre;
 
-  ClosurePostUpdateNode() {
-    CaptureFlow::closurePostUpdateNode(this.getCaptureFlowNode(), pre.getCaptureFlowNode())
+  CapturePostUpdateNode() {
+    CaptureFlow::capturePostUpdateNode(this.getSynthesizedCaptureNode(),
+      pre.getSynthesizedCaptureNode())
   }
 
   override Node getPreUpdateNode() { result = pre }

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll

@@ -52,6 +52,16 @@ private predicate fieldStep(Node node1, Node node2) {
   )
 }
 
+private predicate closureFlowStep(Expr e1, Expr e2) {
+  simpleAstFlowStep(e1, e2)
+  or
+  exists(SsaVariable v |
+    v.getAUse() = e2 and
+    v.getAnUltimateDefinition().(SsaExplicitUpdate).getDefiningExpr().(VariableAssign).getSource() =
+      e1
+  )
+}
+
 private module CaptureInput implements VariableCapture::InputSig {
   private import java as J
 
@@ -60,7 +70,9 @@ private module CaptureInput implements VariableCapture::InputSig {
   class BasicBlock instanceof J::BasicBlock {
     string toString() { result = super.toString() }
 
-    DataFlowCallable getEnclosingCallable() { result.asCallable() = super.getEnclosingCallable() }
+    Callable getEnclosingCallable() { result = super.getEnclosingCallable() }
+
+    Location getLocation() { result = super.getLocation() }
   }
 
   BasicBlock getImmediateBasicBlockDominator(BasicBlock bb) { bbIDominates(result, bb) }
@@ -80,39 +92,59 @@ private module CaptureInput implements VariableCapture::InputSig {
 
     string toString() { result = super.toString() }
 
-    DataFlowCallable getCallable() { result.asCallable() = super.getCallable() }
+    Callable getCallable() { result = super.getCallable() }
+
+    Location getLocation() { result = super.getLocation() }
   }
 
   class CapturedParameter extends CapturedVariable instanceof Parameter { }
 
-  additional predicate capturedVarUpdate(
-    J::BasicBlock bb, int i, CapturedVariable v, VariableUpdate upd
-  ) {
-    upd.getDestVar() = v and bb.getNode(i) = upd
-  }
-
-  additional predicate capturedVarRead(J::BasicBlock bb, int i, CapturedVariable v, RValue rv) {
-    v.(LocalScopeVariable).getAnAccess() = rv and bb.getNode(i) = rv
-  }
-
-  predicate variableWrite(BasicBlock bb, int i, CapturedVariable v, Location loc) {
-    exists(VariableUpdate upd | capturedVarUpdate(bb, i, v, upd) and loc = upd.getLocation())
-  }
-
-  predicate variableRead(BasicBlock bb, int i, CapturedVariable v, Location loc) {
-    exists(RValue rv | capturedVarRead(bb, i, v, rv) and loc = rv.getLocation())
-  }
-
-  class Callable = DataFlowCallable;
-
-  class Call instanceof DataFlowCall {
+  class Expr instanceof J::Expr {
     string toString() { result = super.toString() }
 
     Location getLocation() { result = super.getLocation() }
 
-    DataFlowCallable getEnclosingCallable() { result = super.getEnclosingCallable() }
+    predicate hasCfgNode(BasicBlock bb, int i) { this = bb.(J::BasicBlock).getNode(i) }
+  }
 
-    predicate hasCfgNode(BasicBlock bb, int i) { super.asCall() = bb.(J::BasicBlock).getNode(i) }
+  class VariableWrite extends Expr instanceof VariableUpdate {
+    CapturedVariable v;
+
+    VariableWrite() { super.getDestVar() = v }
+
+    CapturedVariable getVariable() { result = v }
+
+    Expr getSource() {
+      result = this.(VariableAssign).getSource() or
+      result = this.(AssignOp)
+    }
+  }
+
+  class VariableRead extends Expr instanceof RValue {
+    CapturedVariable v;
+
+    VariableRead() { super.getVariable() = v }
+
+    CapturedVariable getVariable() { result = v }
+  }
+
+  class ClosureExpr extends Expr instanceof ClassInstanceExpr {
+    NestedClass nc;
+
+    ClosureExpr() {
+      nc.(AnonymousClass).getClassInstanceExpr() = this
+      or
+      nc instanceof LocalClass and
+      super.getConstructedType().getASourceSupertype*().getSourceDeclaration() = nc
+    }
+
+    predicate hasBody(Callable body) { nc.getACallable() = body }
+
+    predicate hasAliasedAccess(Expr f) { closureFlowStep+(this, f) and not closureFlowStep(f, _) }
+  }
+
+  class Callable extends J::Callable {
+    predicate isConstructor() { this instanceof Constructor }
   }
 }
 
@@ -122,40 +154,27 @@ class CapturedParameter = CaptureInput::CapturedParameter;
 
 module CaptureFlow = VariableCapture::Flow<CaptureInput>;
 
+CaptureFlow::ClosureNode asClosureNode(Node n) {
+  result = n.(CaptureNode).getSynthesizedCaptureNode() or
+  result.(CaptureFlow::ExprNode).getExpr() = n.asExpr() or
+  result.(CaptureFlow::ExprPostUpdateNode).getExpr() =
+    n.(PostUpdateNode).getPreUpdateNode().asExpr() or
+  result.(CaptureFlow::ParameterNode).getParameter() = n.asParameter() or
+  result.(CaptureFlow::ThisParameterNode).getCallable() = n.(InstanceParameterNode).getCallable() or
+  exprNode(result.(CaptureFlow::MallocNode).getClosureExpr()).(PostUpdateNode).getPreUpdateNode() =
+    n
+}
+
 private predicate captureStoreStep(Node node1, ClosureContent c, Node node2) {
-  exists(BasicBlock bb, int i, CaptureInput::CapturedVariable v, VariableUpdate upd |
-    upd.(VariableAssign).getSource() = node1.asExpr() or
-    upd.(AssignOp) = node1.asExpr()
-  |
-    CaptureInput::capturedVarUpdate(bb, i, v, upd) and
-    c.getVariable() = v and
-    CaptureFlow::storeStep(bb, i, v, node2.(ClosureNode).getCaptureFlowNode())
-  )
-  or
-  exists(Parameter p |
-    node1.asParameter() = p and
-    c.getVariable() = p and
-    CaptureFlow::parameterStoreStep(p, node2.(ClosureNode).getCaptureFlowNode())
-  )
+  CaptureFlow::storeStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2))
 }
 
 private predicate captureReadStep(Node node1, ClosureContent c, Node node2) {
-  exists(BasicBlock bb, int i, CaptureInput::CapturedVariable v |
-    CaptureFlow::readStep(node1.(ClosureNode).getCaptureFlowNode(), bb, i, v) and
-    c.getVariable() = v and
-    CaptureInput::capturedVarRead(bb, i, v, node2.asExpr())
-  )
-  or
-  exists(Parameter p |
-    CaptureFlow::parameterReadStep(node1.(ClosureNode).getCaptureFlowNode(), p) and
-    c.getVariable() = p and
-    node2.(PostUpdateNode).getPreUpdateNode().asParameter() = p
-  )
+  CaptureFlow::readStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2))
 }
 
 predicate captureValueStep(Node node1, Node node2) {
-  CaptureFlow::localFlowStep(node1.(ClosureNode).getCaptureFlowNode(),
-    node2.(ClosureNode).getCaptureFlowNode())
+  CaptureFlow::localFlowStep(asClosureNode(node1), asClosureNode(node2))
 }
 
 /**
@@ -493,6 +512,8 @@ predicate additionalLambdaFlowStep(Node nodeFrom, Node nodeTo, boolean preserves
  */
 predicate allowParameterReturnInSelf(ParameterNode p) {
   FlowSummaryImpl::Private::summaryAllowParameterReturnInSelf(p)
+  or
+  CaptureFlow::heuristicAllowInstanceParameterReturnInSelf(p.(InstanceParameterNode).getCallable())
 }
 
 /** An approximated `Content`. */

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll

@@ -139,6 +139,22 @@ private predicate capturedVariableRead(Node n) {
   n.asExpr().(RValue).getVariable() instanceof CapturedVariable
 }
 
+predicate simpleAstFlowStep(Expr e1, Expr e2) {
+  e2.(CastingExpr).getExpr() = e1
+  or
+  e2.(ChooseExpr).getAResultExpr() = e1
+  or
+  e2.(AssignExpr).getSource() = e1
+  or
+  e2.(ArrayCreationExpr).getInit() = e1
+  or
+  e2 = any(StmtExpr stmtExpr | e1 = stmtExpr.getResultExpr())
+  or
+  e2 = any(NotNullExpr nne | e1 = nne.getExpr())
+  or
+  e2.(WhenExpr).getBranch(_).getAResult() = e1
+}
+
 private predicate simpleLocalFlowStep0(Node node1, Node node2) {
   TaintTrackingUtil::forceCachingInSameStage() and
   // Variable flow steps through adjacent def-use and use-use pairs.
@@ -170,19 +186,7 @@ private predicate simpleLocalFlowStep0(Node node1, Node node2) {
   or
   ThisFlow::adjacentThisRefs(node1.(PostUpdateNode).getPreUpdateNode(), node2)
   or
-  node2.asExpr().(CastingExpr).getExpr() = node1.asExpr()
-  or
-  node2.asExpr().(ChooseExpr).getAResultExpr() = node1.asExpr()
-  or
-  node2.asExpr().(AssignExpr).getSource() = node1.asExpr()
-  or
-  node2.asExpr().(ArrayCreationExpr).getInit() = node1.asExpr()
-  or
-  node2.asExpr() = any(StmtExpr stmtExpr | node1.asExpr() = stmtExpr.getResultExpr())
-  or
-  node2.asExpr() = any(NotNullExpr nne | node1.asExpr() = nne.getExpr())
-  or
-  node2.asExpr().(WhenExpr).getBranch(_).getAResult() = node1.asExpr()
+  simpleAstFlowStep(node1.asExpr(), node2.asExpr())
   or
   exists(MethodAccess ma, ValuePreservingMethod m, int argNo |
     ma.getCallee().getSourceDeclaration() = m and m.returnsValue(argNo)

java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImpl.qll

@@ -1222,18 +1222,14 @@ module Private {
           node.asNode().(PostUpdateNode).getPreUpdateNode().(ArgNode).argumentOf(mid.asCall(), apos) and
           parameterMatch(ppos, apos)
         |
-          c = "Argument" and not heapParameter(ppos)
-          or
-          parseArg(c, ppos)
+          c = "Argument" or parseArg(c, ppos)
         )
         or
         exists(ArgumentPosition apos, ParameterPosition ppos |
           node.asNode().(ParamNode).isParameterOf(mid.asCallable(), ppos) and
           parameterMatch(ppos, apos)
         |
-          c = "Parameter" and not heapParameter(ppos)
-          or
-          parseParam(c, apos)
+          c = "Parameter" or parseParam(c, apos)
         )
         or
         c = "ReturnValue" and
@@ -1263,9 +1259,7 @@ module Private {
           node.asNode().(ArgNode).argumentOf(mid.asCall(), apos) and
           parameterMatch(ppos, apos)
         |
-          c = "Argument" and not heapParameter(ppos)
-          or
-          parseArg(c, ppos)
+          c = "Argument" or parseArg(c, ppos)
         )
         or
         exists(ReturnNodeExt ret |

java/ql/lib/semmle/code/java/dataflow/internal/FlowSummaryImplSpecific.qll

@@ -319,12 +319,6 @@ predicate interpretInputSpecific(string c, InterpretNode mid, InterpretNode n) {
   )
 }
 
-/**
- * Holds if `pos` is the position of the `heap` parameter, and thus should not
- * be included by models that specify "any argument" or "any parameter".
- */
-predicate heapParameter(ParameterPosition pos) { pos = -2 }
-
 /** Gets the argument position obtained by parsing `X` in `Parameter[X]`. */
 bindingset[s]
 ArgumentPosition parseParamBody(string s) {