Ruby: Reduce captureFlow(In|Out)

When there is flow in/out of a block through a captured variable, we can restrict
the calls that give rise to the flow to the method calls to which the blocks
belong.

Author: hvitved

ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

@@ -14,7 +14,7 @@ class EntryNode extends CfgNode, TEntryNode {
 
   EntryNode() { this = TEntryNode(scope) }
 
-  final override EntryBasicBlock getBasicBlock() { result = CfgNode.super.getBasicBlock() }
+  final override EntryBasicBlock getBasicBlock() { result = super.getBasicBlock() }
 
   final override Location getLocation() { result = scope.getLocation() }
 
@@ -31,7 +31,7 @@ class AnnotatedExitNode extends CfgNode, TAnnotatedExitNode {
   /** Holds if this node represent a normal exit. */
   final predicate isNormal() { normal = true }
 
-  final override AnnotatedExitBasicBlock getBasicBlock() { result = CfgNode.super.getBasicBlock() }
+  final override AnnotatedExitBasicBlock getBasicBlock() { result = super.getBasicBlock() }
 
   final override Location getLocation() { result = scope.getLocation() }
 

ruby/ql/lib/codeql/ruby/dataflow/SSA.qll

@@ -316,7 +316,7 @@ module Ssa {
     CapturedCallDefinition() {
       exists(Variable v, BasicBlock bb, int i |
         this.definesAt(v, bb, i) and
-        SsaImpl::capturedCallWrite(bb, i, v)
+        SsaImpl::capturedCallWrite(_, bb, i, v)
       )
     }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll

@@ -41,23 +41,21 @@ private predicate capturedExitRead(AnnotatedExitBasicBlock bb, int i, LocalVaria
   i = bb.length()
 }
 
-private CfgScope getCaptureOuterCfgScope(CfgScope scope) {
-  result = scope.getOuterCfgScope() and
-  (
-    scope instanceof Block
-    or
-    scope instanceof Lambda
-  )
-}
-
-/** Holds if captured variable `v` is read inside `scope`. */
+/**
+ * Holds if captured variable `v` is read directly inside `scope`,
+ * or inside a (transitively) nested scope of `scope`.
+ */
 pragma[noinline]
 private predicate hasCapturedRead(Variable v, CfgScope scope) {
   any(LocalVariableReadAccess read |
-    read.getVariable() = v and scope = getCaptureOuterCfgScope*(read.getCfgScope())
+    read.getVariable() = v and scope = read.getCfgScope().getOuterCfgScope*()
   ).isCapturedAccess()
 }
 
+/**
+ * Holds if `v` is written inside basic block `bb`, which is in the immediate
+ * outer scope of `scope`.
+ */
 pragma[noinline]
 private predicate variableWriteInOuterScope(BasicBlock bb, LocalVariable v, CfgScope scope) {
   SsaImplSpecific::variableWrite(bb, _, v, _) and
@@ -71,30 +69,22 @@ private predicate hasVariableWriteWithCapturedRead(BasicBlock bb, LocalVariable
 }
 
 /**
- * Holds if the call at index `i` in basic block `bb` may reach a callable
- * that reads captured variable `v`.
+ * Holds if the call `call` at index `i` in basic block `bb` may reach
+ * a callable that reads captured variable `v`.
  */
-private predicate capturedCallRead(BasicBlock bb, int i, LocalVariable v) {
+private predicate capturedCallRead(Call call, BasicBlock bb, int i, LocalVariable v) {
   exists(CfgScope scope |
     hasVariableWriteWithCapturedRead(bb.getAPredecessor*(), v, scope) and
-    bb.getNode(i).getNode() instanceof Call
+    call = bb.getNode(i).getNode()
   |
-    not scope instanceof Block
-    or
     // If the read happens inside a block, we restrict to the call that
     // contains the block
-    scope = any(MethodCall c | bb.getNode(i) = c.getAControlFlowNode()).getBlock()
+    not scope instanceof Block
+    or
+    scope = call.(MethodCall).getBlock()
   )
 }
 
-/** Holds if captured variable `v` is written inside `scope`. */
-pragma[noinline]
-private predicate hasCapturedWrite(Variable v, CfgScope scope) {
-  any(LocalVariableWriteAccess write |
-    write.getVariable() = v and scope = getCaptureOuterCfgScope*(write.getCfgScope())
-  ).isCapturedAccess()
-}
-
 /** Holds if `v` is read at index `i` in basic block `bb`. */
 private predicate variableReadActual(BasicBlock bb, int i, LocalVariable v) {
   exists(VariableReadAccess read |
@@ -107,21 +97,38 @@ predicate variableRead(BasicBlock bb, int i, LocalVariable v, boolean certain) {
   variableReadActual(bb, i, v) and
   certain = true
   or
-  capturedCallRead(bb, i, v) and
+  capturedCallRead(_, bb, i, v) and
   certain = false
   or
   capturedExitRead(bb, i, v) and
   certain = false
 }
 
+/**
+ * Holds if captured variable `v` is written directly inside `scope`,
+ * or inside a (transitively) nested scope of `scope`.
+ */
+pragma[noinline]
+private predicate hasCapturedWrite(Variable v, CfgScope scope) {
+  any(LocalVariableWriteAccess write |
+    write.getVariable() = v and scope = write.getCfgScope().getOuterCfgScope*()
+  ).isCapturedAccess()
+}
+
+/**
+ * Holds if `v` is read inside basic block `bb`, which is in the immediate
+ * outer scope of `scope`.
+ */
+pragma[noinline]
+private predicate variableReadActualInOuterScope(BasicBlock bb, LocalVariable v, CfgScope scope) {
+  variableReadActual(bb, _, v) and
+  bb.getScope() = scope.getOuterCfgScope()
+}
+
 pragma[noinline]
 private predicate hasVariableReadWithCapturedWrite(BasicBlock bb, LocalVariable v, CfgScope scope) {
   hasCapturedWrite(v, scope) and
-  exists(VariableReadAccess read |
-    read = bb.getANode().getNode() and
-    read.getVariable() = v and
-    bb.getScope() = scope.getOuterCfgScope()
-  )
+  variableReadActualInOuterScope(bb, v, scope)
 }
 
 cached
@@ -137,20 +144,20 @@ private module Cached {
   }
 
   /**
-   * Holds if the call at index `i` in basic block `bb` may reach a callable
+   * Holds if the call `call` at index `i` in basic block `bb` may reach a callable
    * that writes captured variable `v`.
    */
   cached
-  predicate capturedCallWrite(BasicBlock bb, int i, LocalVariable v) {
+  predicate capturedCallWrite(Call call, BasicBlock bb, int i, LocalVariable v) {
     exists(CfgScope scope |
       hasVariableReadWithCapturedWrite(bb.getASuccessor*(), v, scope) and
-      bb.getNode(i).getNode() instanceof Call
+      call = bb.getNode(i).getNode()
     |
-      not scope instanceof Block
-      or
       // If the write happens inside a block, we restrict to the call that
       // contains the block
-      scope = any(MethodCall c | bb.getNode(i) = c.getAControlFlowNode()).getBlock()
+      not scope instanceof Block
+      or
+      scope = call.(MethodCall).getBlock()
     )
   }
 
@@ -180,6 +187,26 @@ private module Cached {
     )
   }
 
+  pragma[noinline]
+  private predicate defReachesCallReadInOuterScope(
+    Definition def, Call call, LocalVariable v, CfgScope scope
+  ) {
+    exists(BasicBlock bb, int i |
+      ssaDefReachesRead(v, def, bb, i) and
+      capturedCallRead(call, bb, i, v) and
+      scope.getOuterCfgScope() = bb.getScope()
+    )
+  }
+
+  pragma[noinline]
+  private predicate hasCapturedEntryWrite(Definition entry, LocalVariable v, CfgScope scope) {
+    exists(BasicBlock bb, int i |
+      capturedEntryWrite(bb, i, v) and
+      entry.definesAt(v, bb, i) and
+      bb.getScope().getOuterCfgScope*() = scope
+    )
+  }
+
   /**
    * Holds if there is flow for a captured variable from the enclosing scope into a block.
    * ```rb
@@ -191,13 +218,35 @@ private module Cached {
    */
   cached
   predicate captureFlowIn(Definition def, Definition entry) {
-    exists(LocalVariable v, BasicBlock bb, int i |
+    exists(Call call, LocalVariable v, CfgScope scope |
+      defReachesCallReadInOuterScope(def, call, v, scope) and
+      hasCapturedEntryWrite(entry, v, scope)
+    |
+      // If the read happens inside a block, we restrict to the call that
+      // contains the block
+      not scope instanceof Block
+      or
+      scope = call.(MethodCall).getBlock()
+    )
+  }
+
+  private import codeql.ruby.dataflow.SSA
+
+  pragma[noinline]
+  private predicate defReachesExitReadInInnerScope(Definition def, LocalVariable v, CfgScope scope) {
+    exists(BasicBlock bb, int i |
       ssaDefReachesRead(v, def, bb, i) and
-      capturedCallRead(bb, i, v) and
-      exists(BasicBlock bb2, int i2 |
-        capturedEntryWrite(bb2, i2, v) and
-        entry.definesAt(v, bb2, i2)
-      )
+      capturedExitRead(bb, i, v) and
+      scope = bb.getScope().getOuterCfgScope*()
+    )
+  }
+
+  pragma[noinline]
+  private predicate hasCapturedExitRead(Definition exit, Call call, LocalVariable v, CfgScope scope) {
+    exists(BasicBlock bb, int i |
+      capturedCallWrite(call, bb, i, v) and
+      exit.definesAt(v, bb, i) and
+      bb.getScope() = scope.getOuterCfgScope()
     )
   }
 
@@ -213,13 +262,15 @@ private module Cached {
    */
   cached
   predicate captureFlowOut(Definition def, Definition exit) {
-    exists(LocalVariable v, BasicBlock bb, int i |
-      ssaDefReachesRead(v, def, bb, i) and
-      capturedExitRead(bb, i, v) and
-      exists(BasicBlock bb2, int i2 |
-        capturedCallWrite(bb2, i2, v) and
-        exit.definesAt(v, bb2, i2)
-      )
+    exists(Call call, LocalVariable v, CfgScope scope |
+      defReachesExitReadInInnerScope(def, v, scope) and
+      hasCapturedExitRead(exit, call, v, _)
+    |
+      // If the read happens inside a block, we restrict to the call that
+      // contains the block
+      not scope instanceof Block
+      or
+      scope = call.(MethodCall).getBlock()
     )
   }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImplSpecific.qll

@@ -40,7 +40,7 @@ predicate variableWrite(BasicBlock bb, int i, SourceVariable v, boolean certain)
   ) and
   certain = true
   or
-  SsaImpl::capturedCallWrite(bb, i, v) and
+  SsaImpl::capturedCallWrite(_, bb, i, v) and
   certain = false
 }
 

ruby/ql/test/library-tests/dataflow/string-flow/string-flow.expected

@@ -201,7 +201,6 @@ edges
 | string_flow.rb:241:10:241:10 | b [array element] :  | string_flow.rb:241:10:241:13 | ...[...] |
 | string_flow.rb:242:10:242:10 | b [array element] :  | string_flow.rb:242:10:242:13 | ...[...] |
 | string_flow.rb:246:5:246:18 | ... = ... :  | string_flow.rb:250:26:250:26 | a :  |
-| string_flow.rb:246:5:246:18 | ... = ... :  | string_flow.rb:258:27:258:27 | a :  |
 | string_flow.rb:246:9:246:18 | call to source :  | string_flow.rb:246:5:246:18 | ... = ... :  |
 | string_flow.rb:246:9:246:18 | call to source :  | string_flow.rb:247:10:247:10 | a :  |
 | string_flow.rb:246:9:246:18 | call to source :  | string_flow.rb:248:20:248:20 | a :  |
@@ -215,7 +214,6 @@ edges
 | string_flow.rb:250:26:250:26 | a :  | string_flow.rb:250:10:250:28 | call to scrub |
 | string_flow.rb:252:10:252:10 | a :  | string_flow.rb:252:10:252:22 | call to scrub! |
 | string_flow.rb:253:21:253:21 | a :  | string_flow.rb:253:10:253:22 | call to scrub! |
-| string_flow.rb:255:5:255:18 | ... = ... :  | string_flow.rb:250:26:250:26 | a :  |
 | string_flow.rb:255:5:255:18 | ... = ... :  | string_flow.rb:258:27:258:27 | a :  |
 | string_flow.rb:255:9:255:18 | call to source :  | string_flow.rb:255:5:255:18 | ... = ... :  |
 | string_flow.rb:255:9:255:18 | call to source :  | string_flow.rb:256:5:256:5 | a :  |