Address some parts of Anders' review.

2026-02-28 21:03:50 +01:00 · 2019-09-30 13:50:44 +02:00
parent 0f5dd5d7c7
commit 812a0bcb16
6 changed files with 53 additions and 33 deletions
--- a/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll
+++ b/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl.qll
@@ -917,6 +917,7 @@ private predicate localFlowStepPlus(
  ) and
  node1 != node2 and
    cc.validFor(node1) and
+    not isUnreachableInCall(node1, cc.(LocalCallContextSpecificCall).getCall()) and
  nodeCand(node2, unbind(config))
  or
  exists(Node mid |
@@ -1733,7 +1734,7 @@ private class PathNodeSink extends PathNode, TPathNodeSink {
 * a callable is recorded by `cc`.
 */
 private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPath ap) {
-  exists(LocalCallContext localCC | localCC.matchesCallContext(cc) |
+  exists(LocalCallContext localCC | localCC = getMatchingLocalCallContext(cc) |
    localFlowBigStep(mid.getNode(), node, true, mid.getConfiguration(), localCC) and
  cc = mid.getCallContext() and
  ap = mid.getAp()
@@ -1744,8 +1745,6 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  ap = node.(AccessPathNilNode).getAp()
  )
  or
-  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
-  (
  jumpStep(mid.getNode(), node, mid.getConfiguration()) and
  cc instanceof CallContextAny and
  ap = mid.getAp()
@@ -1755,6 +1754,8 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  mid.getAp() instanceof AccessPathNil and
  ap = node.(AccessPathNilNode).getAp()
  or
+  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+  (
  contentReadStep(mid, node, ap) and cc = mid.getCallContext()
  or
  exists(Content f, AccessPath ap0 | contentStoreStep(mid, node, ap0, f, cc) and push(ap0, f, ap))
@@ -2236,6 +2237,8 @@ private module FlowExploration {
    ap = TPartialNil(getErasedRepr(node.getType())) and
    config = mid.getConfiguration()
    or
+    not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+    (
    partialPathStoreStep(mid, _, _, node, ap) and
    cc = mid.getCallContext() and
    config = mid.getConfiguration()
@@ -2254,6 +2257,7 @@ private module FlowExploration {
    partialPathThroughCallable(mid, node, cc, ap, config)
    or
    valuePartialPathThroughCallable(mid, node, cc, ap, config)
+    )
  }

  bindingset[result, i]
--- a/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl2.qll
+++ b/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl2.qll
@@ -917,6 +917,7 @@ private predicate localFlowStepPlus(
  ) and
  node1 != node2 and
    cc.validFor(node1) and
+    not isUnreachableInCall(node1, cc.(LocalCallContextSpecificCall).getCall()) and
  nodeCand(node2, unbind(config))
  or
  exists(Node mid |
@@ -1733,7 +1734,7 @@ private class PathNodeSink extends PathNode, TPathNodeSink {
 * a callable is recorded by `cc`.
 */
 private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPath ap) {
-  exists(LocalCallContext localCC | localCC.matchesCallContext(cc) |
+  exists(LocalCallContext localCC | localCC = getMatchingLocalCallContext(cc) |
    localFlowBigStep(mid.getNode(), node, true, mid.getConfiguration(), localCC) and
  cc = mid.getCallContext() and
  ap = mid.getAp()
@@ -1744,8 +1745,6 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  ap = node.(AccessPathNilNode).getAp()
  )
  or
-  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
-  (
  jumpStep(mid.getNode(), node, mid.getConfiguration()) and
  cc instanceof CallContextAny and
  ap = mid.getAp()
@@ -1755,6 +1754,8 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  mid.getAp() instanceof AccessPathNil and
  ap = node.(AccessPathNilNode).getAp()
  or
+  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+  (
  contentReadStep(mid, node, ap) and cc = mid.getCallContext()
  or
  exists(Content f, AccessPath ap0 | contentStoreStep(mid, node, ap0, f, cc) and push(ap0, f, ap))
@@ -2236,6 +2237,8 @@ private module FlowExploration {
    ap = TPartialNil(getErasedRepr(node.getType())) and
    config = mid.getConfiguration()
    or
+    not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+    (
    partialPathStoreStep(mid, _, _, node, ap) and
    cc = mid.getCallContext() and
    config = mid.getConfiguration()
@@ -2254,6 +2257,7 @@ private module FlowExploration {
    partialPathThroughCallable(mid, node, cc, ap, config)
    or
    valuePartialPathThroughCallable(mid, node, cc, ap, config)
+    )
  }

  bindingset[result, i]
--- a/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl3.qll
+++ b/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl3.qll
@@ -917,6 +917,7 @@ private predicate localFlowStepPlus(
  ) and
  node1 != node2 and
    cc.validFor(node1) and
+    not isUnreachableInCall(node1, cc.(LocalCallContextSpecificCall).getCall()) and
  nodeCand(node2, unbind(config))
  or
  exists(Node mid |
@@ -1733,7 +1734,7 @@ private class PathNodeSink extends PathNode, TPathNodeSink {
 * a callable is recorded by `cc`.
 */
 private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPath ap) {
-  exists(LocalCallContext localCC | localCC.matchesCallContext(cc) |
+  exists(LocalCallContext localCC | localCC = getMatchingLocalCallContext(cc) |
    localFlowBigStep(mid.getNode(), node, true, mid.getConfiguration(), localCC) and
  cc = mid.getCallContext() and
  ap = mid.getAp()
@@ -1744,8 +1745,6 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  ap = node.(AccessPathNilNode).getAp()
  )
  or
-  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
-  (
  jumpStep(mid.getNode(), node, mid.getConfiguration()) and
  cc instanceof CallContextAny and
  ap = mid.getAp()
@@ -1755,6 +1754,8 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  mid.getAp() instanceof AccessPathNil and
  ap = node.(AccessPathNilNode).getAp()
  or
+  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+  (
  contentReadStep(mid, node, ap) and cc = mid.getCallContext()
  or
  exists(Content f, AccessPath ap0 | contentStoreStep(mid, node, ap0, f, cc) and push(ap0, f, ap))
@@ -2236,6 +2237,8 @@ private module FlowExploration {
    ap = TPartialNil(getErasedRepr(node.getType())) and
    config = mid.getConfiguration()
    or
+    not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+    (
    partialPathStoreStep(mid, _, _, node, ap) and
    cc = mid.getCallContext() and
    config = mid.getConfiguration()
@@ -2254,6 +2257,7 @@ private module FlowExploration {
    partialPathThroughCallable(mid, node, cc, ap, config)
    or
    valuePartialPathThroughCallable(mid, node, cc, ap, config)
+    )
  }

  bindingset[result, i]
--- a/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl4.qll
+++ b/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl4.qll
@@ -917,6 +917,7 @@ private predicate localFlowStepPlus(
  ) and
  node1 != node2 and
    cc.validFor(node1) and
+    not isUnreachableInCall(node1, cc.(LocalCallContextSpecificCall).getCall()) and
  nodeCand(node2, unbind(config))
  or
  exists(Node mid |
@@ -1733,7 +1734,7 @@ private class PathNodeSink extends PathNode, TPathNodeSink {
 * a callable is recorded by `cc`.
 */
 private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPath ap) {
-  exists(LocalCallContext localCC | localCC.matchesCallContext(cc) |
+  exists(LocalCallContext localCC | localCC = getMatchingLocalCallContext(cc) |
    localFlowBigStep(mid.getNode(), node, true, mid.getConfiguration(), localCC) and
  cc = mid.getCallContext() and
  ap = mid.getAp()
@@ -1744,8 +1745,6 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  ap = node.(AccessPathNilNode).getAp()
  )
  or
-  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
-  (
  jumpStep(mid.getNode(), node, mid.getConfiguration()) and
  cc instanceof CallContextAny and
  ap = mid.getAp()
@@ -1755,6 +1754,8 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  mid.getAp() instanceof AccessPathNil and
  ap = node.(AccessPathNilNode).getAp()
  or
+  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+  (
  contentReadStep(mid, node, ap) and cc = mid.getCallContext()
  or
  exists(Content f, AccessPath ap0 | contentStoreStep(mid, node, ap0, f, cc) and push(ap0, f, ap))
@@ -2236,6 +2237,8 @@ private module FlowExploration {
    ap = TPartialNil(getErasedRepr(node.getType())) and
    config = mid.getConfiguration()
    or
+    not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+    (
    partialPathStoreStep(mid, _, _, node, ap) and
    cc = mid.getCallContext() and
    config = mid.getConfiguration()
@@ -2254,6 +2257,7 @@ private module FlowExploration {
    partialPathThroughCallable(mid, node, cc, ap, config)
    or
    valuePartialPathThroughCallable(mid, node, cc, ap, config)
+    )
  }

  bindingset[result, i]
--- a/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl5.qll
+++ b/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImpl5.qll
@@ -917,6 +917,7 @@ private predicate localFlowStepPlus(
  ) and
  node1 != node2 and
    cc.validFor(node1) and
+    not isUnreachableInCall(node1, cc.(LocalCallContextSpecificCall).getCall()) and
  nodeCand(node2, unbind(config))
  or
  exists(Node mid |
@@ -1733,7 +1734,7 @@ private class PathNodeSink extends PathNode, TPathNodeSink {
 * a callable is recorded by `cc`.
 */
 private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPath ap) {
-  exists(LocalCallContext localCC | localCC.matchesCallContext(cc) |
+  exists(LocalCallContext localCC | localCC = getMatchingLocalCallContext(cc) |
    localFlowBigStep(mid.getNode(), node, true, mid.getConfiguration(), localCC) and
  cc = mid.getCallContext() and
  ap = mid.getAp()
@@ -1744,8 +1745,6 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  ap = node.(AccessPathNilNode).getAp()
  )
  or
-  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
-  (
  jumpStep(mid.getNode(), node, mid.getConfiguration()) and
  cc instanceof CallContextAny and
  ap = mid.getAp()
@@ -1755,6 +1754,8 @@ private predicate pathStep(PathNodeMid mid, Node node, CallContext cc, AccessPat
  mid.getAp() instanceof AccessPathNil and
  ap = node.(AccessPathNilNode).getAp()
  or
+  not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+  (
  contentReadStep(mid, node, ap) and cc = mid.getCallContext()
  or
  exists(Content f, AccessPath ap0 | contentStoreStep(mid, node, ap0, f, cc) and push(ap0, f, ap))
@@ -2236,6 +2237,8 @@ private module FlowExploration {
    ap = TPartialNil(getErasedRepr(node.getType())) and
    config = mid.getConfiguration()
    or
+    not isUnreachableInCall(node, cc.(CallContextSpecificCall).getCall()) and
+    (
    partialPathStoreStep(mid, _, _, node, ap) and
    cc = mid.getCallContext() and
    config = mid.getConfiguration()
@@ -2254,6 +2257,7 @@ private module FlowExploration {
    partialPathThroughCallable(mid, node, cc, ap, config)
    or
    valuePartialPathThroughCallable(mid, node, cc, ap, config)
+    )
  }

  bindingset[result, i]
--- a/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImplCommon.qll
+++ b/java/ql/src/semmle/code/java/dataflow/internal/DataFlowImplCommon.qll
@@ -576,8 +576,9 @@ private module ImplCommon {
    }

    /**
-     * Record a call site in the dataflow graph if it either improves
-     * virtual dispatch or if we can remove unreachable edges by recoring this call site
+   * Holds if recording a dataflow call site either improves
+   * virtual dispatch or if we can remove unreachable edges in the dataflow graph
+   * by recoring this call site
     */
    cached
    predicate recordDataFlowCallSite(DataFlowCall call, DataFlowCallable callable) {
@@ -626,7 +627,7 @@ private module ImplCommon {
   * - `TSpecificCall(DataFlowCall call, int i)` : Flow entered through the `i`th
   *    parameter at the given `call`. This call improves the set of viable
   *    dispatch targets for at least one method call in the current callable
-   *    or helps pruning unreachable nodes from the data flow graph.
+ *    or helps to prune unreachable nodes from the data flow graph.
   * - `TSomeCall(ParameterNode p)` : Flow entered through parameter `p`. The
   *    originating call does not improve the set of dispatch targets for any
   *    method call in the current callable and was therefore not recorded.
@@ -665,24 +666,29 @@ private module ImplCommon {
  }

  /**
-   * A call context which is used to restrict local data flow nodes
+   * A call context that is used to restrict local data flow nodes
   * to nodes which are actually reachable in a call context.
   */
  abstract class LocalCallContext extends TLocalFlowCallContext {
    abstract string toString();

-    abstract predicate matchesCallContext(CallContext ctx);
-
    abstract predicate validFor(Node n);
  }

-  class LocalCallContextAny extends LocalCallContext, TAnyLocalCall {
-    override string toString() { result = "LocalCcAny" }
-
-    override predicate matchesCallContext(CallContext ctx) {
+  LocalCallContext getMatchingLocalCallContext(CallContext ctx) {
+    (
      not ctx instanceof CallContextSpecificCall or
      not exists(TSpecificLocalCall(ctx.(CallContextSpecificCall).getCall()))
-    }
+  ) and
+  exists(LocalCallContextAny l | result = l)
+  or
+  exists(LocalCallContextSpecificCall l |
+    ctx.(CallContextSpecificCall).getCall() = l.getCall() and result = l
+  )
+}
+
+class LocalCallContextAny extends LocalCallContext, TAnyLocalCall {
+  override string toString() { result = "LocalCcAny" }

    override predicate validFor(Node n) { any() }
  }
@@ -701,13 +707,7 @@ private module ImplCommon {

    override string toString() { result = "LocalCcCall(" + call + ")" }

-    override predicate matchesCallContext(CallContext ctx) {
-      ctx.(CallContextSpecificCall).getCall() = call
-    }
-
-    override predicate validFor(Node n) {
-    hasUnreachableNode(call, n.getEnclosingCallable())
-    }
+    override predicate validFor(Node n) { hasUnreachableNode(call, n.getEnclosingCallable()) }
  }

  /** A callable tagged with a relevant return kind. */