Merge pull request github#15540 from hvitved/variable-capture-overwrite

Author: hvitved

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll

@@ -178,6 +178,10 @@ private predicate captureReadStep(Node node1, CapturedVariableContent c, Node no
   CaptureFlow::readStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2))
 }
 
+private predicate captureClearsContent(Node node, CapturedVariableContent c) {
+  CaptureFlow::clearsContent(asClosureNode(node), c.getVariable())
+}
+
 predicate captureValueStep(Node node1, Node node2) {
   CaptureFlow::localFlowStep(asClosureNode(node1), asClosureNode(node2))
 }
@@ -311,6 +315,8 @@ predicate clearsContent(Node n, ContentSet c) {
   )
   or
   FlowSummaryImpl::Private::Steps::summaryClearsContent(n.(FlowSummaryNode).getSummaryNode(), c)
+  or
+  captureClearsContent(n, c)
 }
 
 /**

python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll

@@ -979,6 +979,8 @@ predicate clearsContent(Node n, ContentSet c) {
   FlowSummaryImpl::Private::Steps::summaryClearsContent(n.(FlowSummaryNode).getSummaryNode(), c)
   or
   dictSplatParameterNodeClearStep(n, c)
+  or
+  VariableCapture::clearsContent(n, c)
 }
 
 /**

python/ql/lib/semmle/python/dataflow/new/internal/VariableCapture.qll

@@ -144,6 +144,10 @@ predicate readStep(Node nodeFrom, CapturedVariableContent c, Node nodeTo) {
   Flow::readStep(asClosureNode(nodeFrom), c.getVariable(), asClosureNode(nodeTo))
 }
 
+predicate clearsContent(Node node, CapturedVariableContent c) {
+  Flow::clearsContent(asClosureNode(node), c.getVariable())
+}
+
 predicate valueStep(Node nodeFrom, Node nodeTo) {
   Flow::localFlowStep(asClosureNode(nodeFrom), asClosureNode(nodeTo))
 }

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

@@ -453,6 +453,10 @@ module VariableCapture {
     Flow::localFlowStep(asClosureNode(node1), asClosureNode(node2))
   }
 
+  predicate clearsContent(Node node, Content::CapturedVariableContent c) {
+    Flow::clearsContent(asClosureNode(node), c.getVariable())
+  }
+
   class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt {
     CapturedSsaDefinitionExt() { this.getSourceVariable() instanceof CapturedVariable }
   }
@@ -1930,6 +1934,8 @@ predicate clearsContent(Node n, ContentSet c) {
     c.isKnownOrUnknownElement(TKnownElementContent(cv)) and
     cv.isSymbol(name)
   )
+  or
+  VariableCapture::clearsContent(n, any(Content::CapturedVariableContent v | c.isSingleton(v)))
 }
 
 /**

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

@@ -143,23 +143,23 @@ private predicate hasCapturedRead(Variable v, Cfg::CfgScope scope) {
 }
 
 /**
- * Holds if `v` is written inside basic block `bb`, which is in the immediate
- * outer scope of `scope`.
+ * Holds if `v` is written inside basic block `bb` at index `i`, which is in
+ * the immediate outer scope of `scope`.
  */
 pragma[noinline]
-private predicate variableWriteInOuterScope(Cfg::BasicBlock bb, LocalVariable v, Cfg::CfgScope scope) {
-  SsaInput::variableWrite(bb, _, v, _) and
+private predicate variableWriteInOuterScope(
+  Cfg::BasicBlock bb, int i, LocalVariable v, Cfg::CfgScope scope
+) {
+  SsaInput::variableWrite(bb, i, v, _) and
   scope.getOuterCfgScope() = bb.getScope()
 }
 
 pragma[noinline]
-private predicate proceedsVariableWriteWithCapturedRead(
-  Cfg::BasicBlock bb, LocalVariable v, Cfg::CfgScope scope
+private predicate hasVariableWriteWithCapturedRead(
+  Cfg::BasicBlock bb, int i, LocalVariable v, Cfg::CfgScope scope
 ) {
   hasCapturedRead(v, scope) and
-  variableWriteInOuterScope(bb, v, scope)
-  or
-  proceedsVariableWriteWithCapturedRead(bb.getAPredecessor(), v, scope)
+  variableWriteInOuterScope(bb, i, v, scope)
 }
 
 /**
@@ -168,7 +168,10 @@ private predicate proceedsVariableWriteWithCapturedRead(
  */
 private predicate capturedCallRead(CallCfgNode call, Cfg::BasicBlock bb, int i, LocalVariable v) {
   exists(Cfg::CfgScope scope |
-    proceedsVariableWriteWithCapturedRead(bb, v, scope) and
+    (
+      hasVariableWriteWithCapturedRead(bb, any(int j | j < i), v, scope) or
+      hasVariableWriteWithCapturedRead(bb.getAPredecessor+(), _, v, scope)
+    ) and
     call = bb.getNode(i)
   |
     // If the read happens inside a block, we restrict to the call that
@@ -199,23 +202,23 @@ private predicate hasCapturedWrite(Variable v, Cfg::CfgScope scope) {
 }
 
 /**
- * Holds if `v` is read inside basic block `bb`, which is in the immediate
- * outer scope of `scope`.
+ * Holds if `v` is read inside basic block `bb` at index `i`, which is in the
+ * immediate outer scope of `scope`.
  */
 pragma[noinline]
 private predicate variableReadActualInOuterScope(
-  Cfg::BasicBlock bb, LocalVariable v, Cfg::CfgScope scope
+  Cfg::BasicBlock bb, int i, LocalVariable v, Cfg::CfgScope scope
 ) {
-  variableReadActual(bb, _, v) and
+  variableReadActual(bb, i, v) and
   bb.getScope() = scope.getOuterCfgScope()
 }
 
 pragma[noinline]
 private predicate hasVariableReadWithCapturedWrite(
-  Cfg::BasicBlock bb, LocalVariable v, Cfg::CfgScope scope
+  Cfg::BasicBlock bb, int i, LocalVariable v, Cfg::CfgScope scope
 ) {
   hasCapturedWrite(v, scope) and
-  variableReadActualInOuterScope(bb, v, scope)
+  variableReadActualInOuterScope(bb, i, v, scope)
 }
 
 pragma[noinline]
@@ -324,7 +327,11 @@ private module Cached {
   cached
   predicate capturedCallWrite(CallCfgNode call, Cfg::BasicBlock bb, int i, LocalVariable v) {
     exists(Cfg::CfgScope scope |
-      hasVariableReadWithCapturedWrite(bb.getASuccessor*(), v, scope) and
+      (
+        hasVariableReadWithCapturedWrite(bb, any(int j | j > i), v, scope)
+        or
+        hasVariableReadWithCapturedWrite(bb.getASuccessor+(), _, v, scope)
+      ) and
       call = bb.getNode(i)
     |
       // If the write happens inside a block, we restrict to the call that

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

@@ -114,6 +114,8 @@ edges
 | captured_variables.rb:187:18:187:19 | self [@x] | captured_variables.rb:187:18:187:19 | @x | provenance |  |
 | captured_variables.rb:193:1:193:1 | [post] c [@x] | captured_variables.rb:194:1:194:1 | c [@x] | provenance |  |
 | captured_variables.rb:194:1:194:1 | c [@x] | captured_variables.rb:185:5:189:7 | self in baz [@x] | provenance |  |
+| captured_variables.rb:197:9:197:17 | call to taint | captured_variables.rb:199:10:199:10 | x | provenance |  |
+| captured_variables.rb:206:13:206:21 | call to taint | captured_variables.rb:208:14:208:14 | x | provenance |  |
 | instance_variables.rb:10:19:10:19 | x | instance_variables.rb:11:18:11:18 | x | provenance |  |
 | instance_variables.rb:11:18:11:18 | x | instance_variables.rb:11:9:11:14 | [post] self [@field] | provenance |  |
 | instance_variables.rb:13:5:15:7 | self in get_field [@field] | instance_variables.rb:14:16:14:21 | self [@field] | provenance |  |
@@ -368,6 +370,10 @@ nodes
 | captured_variables.rb:187:18:187:19 | self [@x] | semmle.label | self [@x] |
 | captured_variables.rb:193:1:193:1 | [post] c [@x] | semmle.label | [post] c [@x] |
 | captured_variables.rb:194:1:194:1 | c [@x] | semmle.label | c [@x] |
+| captured_variables.rb:197:9:197:17 | call to taint | semmle.label | call to taint |
+| captured_variables.rb:199:10:199:10 | x | semmle.label | x |
+| captured_variables.rb:206:13:206:21 | call to taint | semmle.label | call to taint |
+| captured_variables.rb:208:14:208:14 | x | semmle.label | x |
 | instance_variables.rb:10:19:10:19 | x | semmle.label | x |
 | instance_variables.rb:11:9:11:14 | [post] self [@field] | semmle.label | [post] self [@field] |
 | instance_variables.rb:11:18:11:18 | x | semmle.label | x |
@@ -575,6 +581,8 @@ subpaths
 | captured_variables.rb:154:14:154:15 | @x | captured_variables.rb:147:10:147:18 | call to taint | captured_variables.rb:154:14:154:15 | @x | $@ | captured_variables.rb:147:10:147:18 | call to taint | call to taint |
 | captured_variables.rb:169:18:169:19 | @x | captured_variables.rb:160:14:160:22 | call to taint | captured_variables.rb:169:18:169:19 | @x | $@ | captured_variables.rb:160:14:160:22 | call to taint | call to taint |
 | captured_variables.rb:187:18:187:19 | @x | captured_variables.rb:178:14:178:22 | call to taint | captured_variables.rb:187:18:187:19 | @x | $@ | captured_variables.rb:178:14:178:22 | call to taint | call to taint |
+| captured_variables.rb:199:10:199:10 | x | captured_variables.rb:197:9:197:17 | call to taint | captured_variables.rb:199:10:199:10 | x | $@ | captured_variables.rb:197:9:197:17 | call to taint | call to taint |
+| captured_variables.rb:208:14:208:14 | x | captured_variables.rb:206:13:206:21 | call to taint | captured_variables.rb:208:14:208:14 | x | $@ | captured_variables.rb:206:13:206:21 | call to taint | call to taint |
 | instance_variables.rb:20:10:20:13 | @foo | instance_variables.rb:19:12:19:21 | call to taint | instance_variables.rb:20:10:20:13 | @foo | $@ | instance_variables.rb:19:12:19:21 | call to taint | call to taint |
 | instance_variables.rb:36:10:36:33 | call to get_field | instance_variables.rb:36:14:36:22 | call to taint | instance_variables.rb:36:10:36:33 | call to get_field | $@ | instance_variables.rb:36:14:36:22 | call to taint | call to taint |
 | instance_variables.rb:39:6:39:33 | call to get_field | instance_variables.rb:39:14:39:22 | call to taint | instance_variables.rb:39:6:39:33 | call to get_field | $@ | instance_variables.rb:39:14:39:22 | call to taint | call to taint |

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

@@ -192,3 +192,25 @@ def baz
 c = CaptureInstanceSelf2.new
 c.foo
 c.baz
+
+class CaptureOverwrite
+    x = taint(16)
+
+    sink(x) # $ hasValueFlow=16
+
+    x = nil
+
+    sink(x)
+
+    fn = -> {
+        x = taint(17)
+
+        sink(x) # $ hasValueFlow=17
+
+        x = nil
+
+        sink(x)
+    }
+
+    fn.call()
+end

ruby/ql/test/library-tests/variables/ssa.expected

@@ -90,7 +90,6 @@ definition
 | scopes.rb:2:9:6:3 | <captured entry> self | scopes.rb:1:1:49:4 | self |
 | scopes.rb:4:4:4:4 | a | scopes.rb:4:4:4:4 | a |
 | scopes.rb:7:1:7:1 | a | scopes.rb:7:1:7:1 | a |
-| scopes.rb:9:1:18:3 | <captured exit> a | scopes.rb:7:1:7:1 | a |
 | scopes.rb:9:9:18:3 | <captured entry> a | scopes.rb:7:1:7:1 | a |
 | scopes.rb:9:9:18:3 | <captured entry> self | scopes.rb:1:1:49:4 | self |
 | scopes.rb:11:4:11:4 | a | scopes.rb:7:1:7:1 | a |

shared/dataflow/codeql/dataflow/VariableCapture.qll

@@ -232,6 +232,9 @@ signature module OutputSig<LocationSig Location, InputSig<Location> I> {
 
   /** Holds if this-to-this summaries are expected for `c`. */
   predicate heuristicAllowInstanceParameterReturnInSelf(I::Callable c);
+
+  /** Holds if captured variable `v` is cleared at `node`. */
+  predicate clearsContent(ClosureNode node, I::CapturedVariable v);
 }
 
 /**
@@ -959,4 +962,11 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
       )
     )
   }
+
+  predicate clearsContent(ClosureNode node, CapturedVariable v) {
+    exists(BasicBlock bb, int i |
+      captureWrite(v, bb, i, false, _) and
+      node = TSynthThisQualifier(bb, i, false)
+    )
+  }
 }