Ruby: Handle captured yield calls

Author: hvitved

ruby/ql/consistency-queries/DataFlowConsistency.ql

@@ -43,10 +43,6 @@ private module Input implements InputSig<RubyDataFlow> {
       arg.asExpr().getASuccessor(any(SuccessorTypes::ConditionalSuccessor c)).getASuccessor*() = n and
       n.getASplit() instanceof Split::ConditionalCompletionSplit
     )
-    or
-    // Synthetic block parameter nodes are passed directly as lambda-self reference
-    // arguments to all `yield` calls
-    arg instanceof ArgumentNodes::BlockParameterArgumentNode
   }
 }
 

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

@@ -208,7 +208,9 @@ private predicate moduleFlowsToMethodCallReceiver(RelevantCall call, Module m, s
   flowsToMethodCallReceiver(call, trackModuleAccess(m), method)
 }
 
-private Block blockCall(RelevantCall call) { lambdaSourceCall(call, _, trackBlock(result)) }
+private Block blockCall(RelevantCall call) {
+  lambdaSourceCall(call, _, trackBlock(result).(DataFlow::LocalSourceNode).getALocalUse())
+}
 
 pragma[nomagic]
 private predicate superCall(RelevantCall call, Module cls, string method) {

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

@@ -230,6 +230,8 @@ module LocalFlow {
         or
         p.(KeywordParameter).getDefaultValue() = nodeFrom.asExpr().getExpr()
       )
+    or
+    nodeTo.(BlockArgumentNode).getParameterNode(true) = nodeFrom
   }
 }
 
@@ -497,6 +499,9 @@ private module Cached {
     TSelfParameterNode(MethodBase m) or
     TLambdaSelfReferenceNode(Callable c) { lambdaCreationExpr(_, _, c) } or
     TBlockParameterNode(MethodBase m) or
+    TBlockArgumentNode(CfgNodes::ExprNodes::CallCfgNode yield) {
+      yield = any(BlockParameterNode b).getAYieldCall()
+    } or
     TSynthHashSplatParameterNode(DataFlowCallable c) {
       isParameterNode(_, c, any(ParameterPosition p | p.isKeyword(_)))
     } or
@@ -645,6 +650,8 @@ private module Cached {
     isStoreTargetNode(n)
     or
     TypeTrackingInput::loadStep(_, n, _)
+    or
+    n instanceof BlockArgumentNode
   }
 
   cached
@@ -770,6 +777,8 @@ predicate nodeIsHidden(Node n) {
   n instanceof LambdaSelfReferenceNode
   or
   n instanceof CaptureNode
+  or
+  n instanceof BlockArgumentNode
 }
 
 /** An SSA definition, viewed as a node in a data flow graph. */
@@ -1277,18 +1286,36 @@ module ArgumentNodes {
     }
   }
 
-  class BlockParameterArgumentNode extends BlockParameterNode, ArgumentNode {
-    BlockParameterArgumentNode() { exists(this.getAYieldCall()) }
+  class BlockArgumentNode extends NodeImpl, ArgumentNode, TBlockArgumentNode {
+    CfgNodes::ExprNodes::CallCfgNode yield;
+
+    BlockArgumentNode() { this = TBlockArgumentNode(yield) }
+
+    CfgNodes::ExprNodes::CallCfgNode getYieldCall() { result = yield }
+
+    pragma[nomagic]
+    BlockParameterNode getParameterNode(boolean inSameScope) {
+      result.getAYieldCall() = yield and
+      if nodeGetEnclosingCallable(this) = nodeGetEnclosingCallable(result)
+      then inSameScope = true
+      else inSameScope = false
+    }
 
     // needed for variable capture flow
     override predicate sourceArgumentOf(CfgNodes::ExprNodes::CallCfgNode call, ArgumentPosition pos) {
-      call = this.getAYieldCall() and
+      call = yield and
       pos.isLambdaSelf()
     }
 
     override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
       this.sourceArgumentOf(call.asCall(), pos)
     }
+
+    override CfgScope getCfgScope() { result = yield.getScope() }
+
+    override Location getLocationImpl() { result = yield.getLocation() }
+
+    override string toStringImpl() { result = "yield block argument" }
   }
 
   private class SummaryArgumentNode extends FlowSummaryNode, ArgumentNode {
@@ -1699,6 +1726,8 @@ predicate jumpStep(Node pred, Node succ) {
     succ.(FlowSummaryNode).getSummaryNode())
   or
   any(AdditionalJumpStep s).step(pred, succ)
+  or
+  succ.(BlockArgumentNode).getParameterNode(false) = pred
 }
 
 private ContentSet getArrayContent(int n) {
@@ -2037,7 +2066,7 @@ private predicate lambdaCallExpr(
  */
 predicate lambdaSourceCall(CfgNodes::ExprNodes::CallCfgNode call, LambdaCallKind kind, Node receiver) {
   kind = TYieldCallKind() and
-  call = receiver.(BlockParameterNode).getAYieldCall()
+  call = receiver.(BlockArgumentNode).getYieldCall()
   or
   kind = TLambdaCallKind() and
   lambdaCallExpr(call, receiver.asExpr())

ruby/ql/test/library-tests/dataflow/call-sensitivity/call-sensitivity.expected

@@ -1,5 +1,4 @@
 testFailures
-| call_sensitivity.rb:200:10:200:28 | # $ hasValueFlow=37 | Missing result:hasValueFlow=37 |
 edges
 | call_sensitivity.rb:9:7:9:13 | call to taint | call_sensitivity.rb:9:6:9:14 | ( ... ) |
 | call_sensitivity.rb:11:13:11:13 | x | call_sensitivity.rb:12:11:12:11 | x |
@@ -82,6 +81,17 @@ edges
 | call_sensitivity.rb:178:11:178:19 | call to taint | call_sensitivity.rb:174:19:174:19 | x |
 | call_sensitivity.rb:187:11:187:20 | ( ... ) | call_sensitivity.rb:104:18:104:18 | x |
 | call_sensitivity.rb:187:12:187:19 | call to taint | call_sensitivity.rb:187:11:187:20 | ( ... ) |
+| call_sensitivity.rb:189:19:189:19 | x | call_sensitivity.rb:190:9:190:9 | x |
+| call_sensitivity.rb:190:9:190:9 | x | call_sensitivity.rb:194:23:194:23 | x |
+| call_sensitivity.rb:193:19:193:19 | x | call_sensitivity.rb:194:17:194:17 | x |
+| call_sensitivity.rb:194:17:194:17 | x | call_sensitivity.rb:189:19:189:19 | x |
+| call_sensitivity.rb:194:23:194:23 | x | call_sensitivity.rb:195:11:195:11 | x |
+| call_sensitivity.rb:195:11:195:11 | x | call_sensitivity.rb:199:30:199:30 | x |
+| call_sensitivity.rb:195:11:195:11 | x | call_sensitivity.rb:203:26:203:26 | x |
+| call_sensitivity.rb:199:15:199:24 | ( ... ) | call_sensitivity.rb:193:19:193:19 | x |
+| call_sensitivity.rb:199:16:199:23 | call to taint | call_sensitivity.rb:199:15:199:24 | ( ... ) |
+| call_sensitivity.rb:199:30:199:30 | x | call_sensitivity.rb:200:8:200:8 | x |
+| call_sensitivity.rb:203:26:203:26 | x | call_sensitivity.rb:204:8:204:8 | x |
 nodes
 | call_sensitivity.rb:9:6:9:14 | ( ... ) | semmle.label | ( ... ) |
 | call_sensitivity.rb:9:7:9:13 | call to taint | semmle.label | call to taint |
@@ -169,6 +179,18 @@ nodes
 | call_sensitivity.rb:178:11:178:19 | call to taint | semmle.label | call to taint |
 | call_sensitivity.rb:187:11:187:20 | ( ... ) | semmle.label | ( ... ) |
 | call_sensitivity.rb:187:12:187:19 | call to taint | semmle.label | call to taint |
+| call_sensitivity.rb:189:19:189:19 | x | semmle.label | x |
+| call_sensitivity.rb:190:9:190:9 | x | semmle.label | x |
+| call_sensitivity.rb:193:19:193:19 | x | semmle.label | x |
+| call_sensitivity.rb:194:17:194:17 | x | semmle.label | x |
+| call_sensitivity.rb:194:23:194:23 | x | semmle.label | x |
+| call_sensitivity.rb:195:11:195:11 | x | semmle.label | x |
+| call_sensitivity.rb:199:15:199:24 | ( ... ) | semmle.label | ( ... ) |
+| call_sensitivity.rb:199:16:199:23 | call to taint | semmle.label | call to taint |
+| call_sensitivity.rb:199:30:199:30 | x | semmle.label | x |
+| call_sensitivity.rb:200:8:200:8 | x | semmle.label | x |
+| call_sensitivity.rb:203:26:203:26 | x | semmle.label | x |
+| call_sensitivity.rb:204:8:204:8 | x | semmle.label | x |
 subpaths
 #select
 | call_sensitivity.rb:9:6:9:14 | ( ... ) | call_sensitivity.rb:9:7:9:13 | call to taint | call_sensitivity.rb:9:6:9:14 | ( ... ) | $@ | call_sensitivity.rb:9:7:9:13 | call to taint | call to taint |
@@ -194,6 +216,8 @@ subpaths
 | call_sensitivity.rb:105:10:105:10 | x | call_sensitivity.rb:125:12:125:19 | call to taint | call_sensitivity.rb:105:10:105:10 | x | $@ | call_sensitivity.rb:125:12:125:19 | call to taint | call to taint |
 | call_sensitivity.rb:105:10:105:10 | x | call_sensitivity.rb:178:11:178:19 | call to taint | call_sensitivity.rb:105:10:105:10 | x | $@ | call_sensitivity.rb:178:11:178:19 | call to taint | call to taint |
 | call_sensitivity.rb:105:10:105:10 | x | call_sensitivity.rb:187:12:187:19 | call to taint | call_sensitivity.rb:105:10:105:10 | x | $@ | call_sensitivity.rb:187:12:187:19 | call to taint | call to taint |
+| call_sensitivity.rb:200:8:200:8 | x | call_sensitivity.rb:199:16:199:23 | call to taint | call_sensitivity.rb:200:8:200:8 | x | $@ | call_sensitivity.rb:199:16:199:23 | call to taint | call to taint |
+| call_sensitivity.rb:204:8:204:8 | x | call_sensitivity.rb:199:16:199:23 | call to taint | call_sensitivity.rb:204:8:204:8 | x | $@ | call_sensitivity.rb:199:16:199:23 | call to taint | call to taint |
 mayBenefitFromCallContext
 | call_sensitivity.rb:51:5:51:10 | call to sink |
 | call_sensitivity.rb:55:5:55:13 | call to method1 |

ruby/ql/test/library-tests/dataflow/call-sensitivity/call_sensitivity.rb

@@ -201,5 +201,5 @@ def invoke_block2 x
 end
 
 invoke_block2 "safe" do |x|
-  sink x
+  sink x # $ SPURIOUS hasValueFlow=37
 end

ruby/ql/test/library-tests/dataflow/global/TypeTrackingInlineTest.expected

@@ -1,4 +1,5 @@
 testFailures
+| blocks.rb:4:10:4:10 | r | Fixed missing result:hasValueFlow=1 |
 | captured_variables.rb:50:10:50:10 | x | Fixed missing result:hasValueFlow=2 |
 | captured_variables.rb:68:25:68:68 | # $ hasValueFlow=3 $ MISSING: hasValueFlow=4 | Missing result:hasValueFlow=3 |
 | captured_variables.rb:72:21:72:66 | # $ hasValueFlow=4 $ SPURIOUS: hasValueFlow=3  | Fixed spurious result:hasValueFlow=3 |

ruby/ql/test/library-tests/dataflow/global/blocks.rb

@@ -1,7 +1,7 @@
 class A
   def m1(&block)
-    r = block.call() # $ MISSING: hasValueFlow=1
-    sink r
+    r = block.call()
+    sink r # $ MISSING: hasValueFlow=1
   end
 
   def m2

shared/dataflow/codeql/dataflow/internal/DataFlowImplConsistency.qll

@@ -319,4 +319,9 @@ module MakeConsistency<
     strictcount(DataFlowCall call0 | multipleArgumentCallInclude(arg, call0)) > 1 and
     msg = "Multiple calls for argument node."
   }
+
+  query predicate lambdaCallEnclosingCallableMismatch(DataFlowCall call, Node receiver) {
+    lambdaCall(call, _, receiver) and
+    not nodeGetEnclosingCallable(receiver) = call.getEnclosingCallable()
+  }
 }