Merge pull request #15273 from hvitved/ruby/captured-yield

hvitved · web-flow · commit a1036c81eecf · 2024-01-11T14:34:34.000+01:00
Ruby: Handle captured `yield` calls
diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected
@@ -166,3 +166,4 @@ uniqueContentApprox
 identityLocalStep
 missingArgumentCall
 multipleArgumentCall
+lambdaCallEnclosingCallableMismatch
diff --git a/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected
@@ -31,3 +31,4 @@ uniqueContentApprox
 identityLocalStep
 missingArgumentCall
 multipleArgumentCall
+lambdaCallEnclosingCallableMismatch
diff --git a/cpp/ql/test/library-tests/dataflow/fields/dataflow-consistency.expected b/cpp/ql/test/library-tests/dataflow/fields/dataflow-consistency.expected
@@ -193,3 +193,4 @@ uniqueContentApprox
 identityLocalStep
 missingArgumentCall
 multipleArgumentCall
+lambdaCallEnclosingCallableMismatch
diff --git a/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/dataflow/fields/dataflow-ir-consistency.expected
@@ -27,3 +27,4 @@ uniqueContentApprox
 identityLocalStep
 missingArgumentCall
 multipleArgumentCall
+lambdaCallEnclosingCallableMismatch
diff --git a/cpp/ql/test/library-tests/syntax-zoo/dataflow-consistency.expected b/cpp/ql/test/library-tests/syntax-zoo/dataflow-consistency.expected
@@ -99,3 +99,4 @@ uniqueContentApprox
 identityLocalStep
 missingArgumentCall
 multipleArgumentCall
+lambdaCallEnclosingCallableMismatch
diff --git a/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected b/cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected
@@ -42,3 +42,4 @@ uniqueContentApprox
 identityLocalStep
 missingArgumentCall
 multipleArgumentCall
+lambdaCallEnclosingCallableMismatch
diff --git a/ruby/ql/consistency-queries/DataFlowConsistency.ql b/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
   }
 }
 
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll b/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) {
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/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())
diff --git a/ruby/ql/test/library-tests/dataflow/call-sensitivity/call-sensitivity.expected b/ruby/ql/test/library-tests/dataflow/call-sensitivity/call-sensitivity.expected
@@ -81,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 |
@@ -168,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 |
@@ -193,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 |
@@ -212,6 +237,7 @@ mayBenefitFromCallContext
 | call_sensitivity.rb:149:5:149:28 | call to singleton_method2 |
 | call_sensitivity.rb:153:5:153:35 | call to singleton_method3 |
 | call_sensitivity.rb:175:3:175:12 | call to new |
+| call_sensitivity.rb:194:3:196:5 | call to invoke_block1 |
 viableImplInCallContext
 | call_sensitivity.rb:51:5:51:10 | call to sink | call_sensitivity.rb:55:5:55:13 | call to method1 | call_sensitivity.rb:5:1:7:3 | sink |
 | call_sensitivity.rb:51:5:51:10 | call to sink | call_sensitivity.rb:63:5:63:16 | call to method1 | call_sensitivity.rb:5:1:7:3 | sink |
@@ -267,3 +293,5 @@ viableImplInCallContext
 | call_sensitivity.rb:153:5:153:35 | call to singleton_method3 | call_sensitivity.rb:171:1:171:34 | call to call_singleton_method3 | call_sensitivity.rb:96:3:98:5 | singleton_method3 |
 | call_sensitivity.rb:175:3:175:12 | call to new | call_sensitivity.rb:178:1:178:20 | call to create | call_sensitivity.rb:104:3:107:5 | initialize |
 | call_sensitivity.rb:175:3:175:12 | call to new | call_sensitivity.rb:179:1:179:20 | call to create | call_sensitivity.rb:156:3:158:5 | initialize |
+| call_sensitivity.rb:194:3:196:5 | call to invoke_block1 | call_sensitivity.rb:199:1:201:3 | call to invoke_block2 | call_sensitivity.rb:189:1:191:3 | invoke_block1 |
+| call_sensitivity.rb:194:3:196:5 | call to invoke_block1 | call_sensitivity.rb:203:1:205:3 | call to invoke_block2 | call_sensitivity.rb:189:1:191:3 | invoke_block1 |
diff --git a/ruby/ql/test/library-tests/dataflow/call-sensitivity/call_sensitivity.rb b/ruby/ql/test/library-tests/dataflow/call-sensitivity/call_sensitivity.rb
@@ -185,3 +185,21 @@ def method1 x
 end
 
 c = C.new (taint 32)
+
+def invoke_block1 x
+  yield x
+end
+
+def invoke_block2 x
+  invoke_block1 x do |x|
+    yield x
+  end
+end
+
+invoke_block2 (taint 37) do |x|
+  sink x # $ hasValueFlow=37
+end
+
+invoke_block2 "safe" do |x|
+  sink x # $ SPURIOUS hasValueFlow=37
+end
diff --git a/ruby/ql/test/library-tests/dataflow/global/TypeTrackingInlineTest.expected b/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 |
diff --git a/ruby/ql/test/library-tests/dataflow/global/blocks.rb b/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
diff --git a/shared/dataflow/codeql/dataflow/internal/DataFlowImplConsistency.qll b/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()
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -43,10 +43,6 @@ private module Input implements InputSig<RubyDataFlow> {`
`43`	`43`	`arg.asExpr().getASuccessor(any(SuccessorTypes::ConditionalSuccessor c)).getASuccessor*() = n and`
`44`	`44`	`n.getASplit() instanceof Split::ConditionalCompletionSplit`
`45`	`45`	`)`
`46`		`- or`
`47`		`- // Synthetic block parameter nodes are passed directly as lambda-self reference`
`48`		- // arguments to all `yield` calls
`49`		`- arg instanceof ArgumentNodes::BlockParameterArgumentNode`
`50`	`46`	`}`
`51`	`47`	`}`
`52`	`48`
Original file line number	Diff line number	Diff line change
`@@ -208,7 +208,9 @@ private predicate moduleFlowsToMethodCallReceiver(RelevantCall call, Module m, s`
`208`	`208`	`flowsToMethodCallReceiver(call, trackModuleAccess(m), method)`
`209`	`209`	`}`
`210`	`210`
`211`		`-private Block blockCall(RelevantCall call) { lambdaSourceCall(call, _, trackBlock(result)) }`
	`211`	`+private Block blockCall(RelevantCall call) {`
	`212`	`+ lambdaSourceCall(call, _, trackBlock(result).(DataFlow::LocalSourceNode).getALocalUse())`
	`213`	`+}`
`212`	`214`
`213`	`215`	`pragma[nomagic]`
`214`	`216`	`private predicate superCall(RelevantCall call, Module cls, string method) {`