Merge pull request #15507 from asgerf/shared/outbarrier-bugfix

Shared: fix a bug in stateful outbarriers

java/ql/test/library-tests/dataflow/inoutbarriers/A.java

@@ -1,10 +1,17 @@
 class A {
   static String fsrc = "";
+  String fsink = "";
 
   String src(String s) { return s; }
 
   void sink(String s) { }
 
+  static String flowThroughSink(String s) {
+    A obj = new A();
+    obj.fsink = s;
+    return obj.fsink;
+  }
+
   void foo() {
     String s = fsrc;
     sink(fsrc);
@@ -13,5 +20,9 @@ class A {
     sink(s);
 
     sink(s);
+
+    s = fsrc;
+    s = flowThroughSink(s);
+    sink(s);
   }
 }

java/ql/test/library-tests/dataflow/inoutbarriers/test.expected

@@ -1,7 +1,9 @@
 inconsistentFlow
 #select
-| A.java:9:16:9:19 | fsrc | A.java:13:10:13:10 | s | nobarrier, sinkbarrier |
-| A.java:9:16:9:19 | fsrc | A.java:15:10:15:10 | s | nobarrier |
-| A.java:10:10:10:13 | fsrc | A.java:10:10:10:13 | fsrc | both, nobarrier, sinkbarrier, srcbarrier |
-| A.java:12:9:12:14 | src(...) | A.java:13:10:13:10 | s | both, nobarrier, sinkbarrier, srcbarrier |
-| A.java:12:9:12:14 | src(...) | A.java:15:10:15:10 | s | nobarrier, srcbarrier |
+| A.java:16:16:16:19 | fsrc | A.java:20:10:20:10 | s | nobarrier, sinkbarrier |
+| A.java:16:16:16:19 | fsrc | A.java:22:10:22:10 | s | nobarrier |
+| A.java:17:10:17:13 | fsrc | A.java:17:10:17:13 | fsrc | both, nobarrier, sinkbarrier, srcbarrier |
+| A.java:19:9:19:14 | src(...) | A.java:20:10:20:10 | s | both, nobarrier, sinkbarrier, srcbarrier |
+| A.java:19:9:19:14 | src(...) | A.java:22:10:22:10 | s | nobarrier, srcbarrier |
+| A.java:24:9:24:12 | fsrc | A.java:11:17:11:17 | s | both, nobarrier, sinkbarrier, srcbarrier |
+| A.java:24:9:24:12 | fsrc | A.java:26:10:26:10 | s | nobarrier, srcbarrier |

java/ql/test/library-tests/dataflow/inoutbarriers/test.ql

@@ -12,6 +12,11 @@ predicate sink0(Node n) {
     sink.getMethod().hasName("sink") and
     sink.getAnArgument() = n.asExpr()
   )
+  or
+  exists(AssignExpr assign |
+    assign.getDest().(FieldAccess).getField().hasName("fsink") and
+    n.asExpr() = assign.getSource()
+  )
 }
 
 module Conf1 implements ConfigSig {

shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll

@@ -2682,6 +2682,7 @@ module MakeImpl<InputSig Lang> {
       ) {
         not isUnreachableInCall1(node2, cc) and
         not inBarrier(node2, state) and
+        not outBarrier(node1, state) and
         (
           localFlowEntry(node1, pragma[only_bind_into](state)) and
           (
@@ -3761,6 +3762,9 @@ module MakeImpl<InputSig Lang> {
 
       override NodeEx getNodeEx() { result = node }
 
+      pragma[inline]
+      final NodeEx getNodeExOutgoing() { result = node and not outBarrier(node, state) }
+
       override FlowState getState() { result = state }
 
       CallContext getCallContext() { result = cc }
@@ -3777,14 +3781,11 @@ module MakeImpl<InputSig Lang> {
       }
 
       override PathNodeImpl getASuccessorImpl() {
-        not outBarrier(node, state) and
-        (
-          // an intermediate step to another intermediate node
-          result = this.getSuccMid()
-          or
-          // a final step to a sink
-          result = this.getSuccMid().projectToSink()
-        )
+        // an intermediate step to another intermediate node
+        result = this.getSuccMid()
+        or
+        // a final step to a sink
+        result = this.getSuccMid().projectToSink()
       }
 
       override predicate isSource() {
@@ -3935,14 +3936,14 @@ module MakeImpl<InputSig Lang> {
         ap instanceof AccessPathNil
       )
       or
-      jumpStepEx(mid.getNodeEx(), node) and
+      jumpStepEx(mid.getNodeExOutgoing(), node) and
       state = mid.getState() and
       cc instanceof CallContextAny and
       sc instanceof SummaryCtxNone and
       t = mid.getType() and
       ap = mid.getAp()
       or
-      additionalJumpStep(mid.getNodeEx(), node) and
+      additionalJumpStep(mid.getNodeExOutgoing(), node) and
       state = mid.getState() and
       cc instanceof CallContextAny and
       sc instanceof SummaryCtxNone and
@@ -3950,7 +3951,7 @@ module MakeImpl<InputSig Lang> {
       t = node.getDataFlowType() and
       ap = TAccessPathNil()
       or
-      additionalJumpStateStep(mid.getNodeEx(), mid.getState(), node, state) and
+      additionalJumpStateStep(mid.getNodeExOutgoing(), mid.getState(), node, state) and
       cc instanceof CallContextAny and
       sc instanceof SummaryCtxNone and
       mid.getAp() instanceof AccessPathNil and
@@ -3985,7 +3986,7 @@ module MakeImpl<InputSig Lang> {
     ) {
       ap0 = mid.getAp() and
       c = ap0.getHead() and
-      Stage5::readStepCand(mid.getNodeEx(), c, node) and
+      Stage5::readStepCand(mid.getNodeExOutgoing(), c, node) and
       state = mid.getState() and
       cc = mid.getCallContext()
     }
@@ -3998,7 +3999,7 @@ module MakeImpl<InputSig Lang> {
       exists(DataFlowType contentType |
         t0 = mid.getType() and
         ap0 = mid.getAp() and
-        Stage5::storeStepCand(mid.getNodeEx(), _, c, node, contentType, t) and
+        Stage5::storeStepCand(mid.getNodeExOutgoing(), _, c, node, contentType, t) and
         state = mid.getState() and
         cc = mid.getCallContext() and
         compatibleTypes(t0, contentType)
@@ -4016,7 +4017,8 @@ module MakeImpl<InputSig Lang> {
         not outBarrier(retNode, state) and
         innercc = mid.getCallContext() and
         innercc instanceof CallContextNoCall and
-        apa = mid.getAp().getApprox()
+        apa = mid.getAp().getApprox() and
+        not outBarrier(retNode, state)
       )
     }
 
@@ -4137,7 +4139,8 @@ module MakeImpl<InputSig Lang> {
         pathNode(_, ret, state, cc, sc, t, ap, _) and
         kind = ret.getKind() and
         apa = ap.getApprox() and
-        parameterFlowThroughAllowed(sc.getParamNode(), kind)
+        parameterFlowThroughAllowed(sc.getParamNode(), kind) and
+        not outBarrier(ret, state)
       )
     }