Capture flow: Take overwrites in nested scopes into account

hvitved · hvitved · commit 1ea7717714e8 · 2024-02-09T14:49:23.000+01:00
diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll b/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)
 }
 
 /**
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/DataFlowPrivate.qll b/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)
 }
 
 /**
diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/VariableCapture.qll b/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))
 }
diff --git a/ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll b/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)))
 }
 
 /**
diff --git a/ruby/ql/test/library-tests/dataflow/global/Flow.expected b/ruby/ql/test/library-tests/dataflow/global/Flow.expected
@@ -1,5 +1,4 @@
 testFailures
-| captured_variables.rb:212:14:212:14 | x | Unexpected result: hasValueFlow=17 |
 edges
 | blocks.rb:14:12:14:20 | call to source | blocks.rb:8:10:8:14 | yield ... | provenance |  |
 | captured_variables.rb:9:24:9:24 | x | captured_variables.rb:10:10:10:23 | -> { ... } [captured x] | provenance |  |
@@ -117,7 +116,6 @@ edges
 | 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 |  |
-| captured_variables.rb:206:13:206:21 | call to taint | captured_variables.rb:212:14:212: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 |  |
@@ -376,7 +374,6 @@ nodes
 | 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 |
-| captured_variables.rb:212:14:212: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 |
@@ -586,7 +583,6 @@ subpaths
 | 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 |
-| captured_variables.rb:212:14:212:14 | x | captured_variables.rb:206:13:206:21 | call to taint | captured_variables.rb:212:14:212: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 |
diff --git a/shared/dataflow/codeql/dataflow/VariableCapture.qll b/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)
+    )
+  }
 }

Original file line number	Diff line number	Diff line change
`@@ -178,6 +178,10 @@ private predicate captureReadStep(Node node1, CapturedVariableContent c, Node no`
`178`	`178`	`CaptureFlow::readStep(asClosureNode(node1), c.getVariable(), asClosureNode(node2))`
`179`	`179`	`}`
`180`	`180`
	`181`	`+private predicate captureClearsContent(Node node, CapturedVariableContent c) {`
	`182`	`+ CaptureFlow::clearsContent(asClosureNode(node), c.getVariable())`
	`183`	`+}`
	`184`	`+`
`181`	`185`	`predicate captureValueStep(Node node1, Node node2) {`
`182`	`186`	`CaptureFlow::localFlowStep(asClosureNode(node1), asClosureNode(node2))`
`183`	`187`	`}`
`@@ -311,6 +315,8 @@ predicate clearsContent(Node n, ContentSet c) {`
`311`	`315`	`)`
`312`	`316`	`or`
`313`	`317`	`FlowSummaryImpl::Private::Steps::summaryClearsContent(n.(FlowSummaryNode).getSummaryNode(), c)`
	`318`	`+ or`
	`319`	`+ captureClearsContent(n, c)`
`314`	`320`	`}`
`315`	`321`
`316`	`322`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -979,6 +979,8 @@ predicate clearsContent(Node n, ContentSet c) {`
`979`	`979`	`FlowSummaryImpl::Private::Steps::summaryClearsContent(n.(FlowSummaryNode).getSummaryNode(), c)`
`980`	`980`	`or`
`981`	`981`	`dictSplatParameterNodeClearStep(n, c)`
	`982`	`+ or`
	`983`	`+ VariableCapture::clearsContent(n, c)`
`982`	`984`	`}`
`983`	`985`
`984`	`986`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -144,6 +144,10 @@ predicate readStep(Node nodeFrom, CapturedVariableContent c, Node nodeTo) {`
`144`	`144`	`Flow::readStep(asClosureNode(nodeFrom), c.getVariable(), asClosureNode(nodeTo))`
`145`	`145`	`}`
`146`	`146`
	`147`	`+predicate clearsContent(Node node, CapturedVariableContent c) {`
	`148`	`+ Flow::clearsContent(asClosureNode(node), c.getVariable())`
	`149`	`+}`
	`150`	`+`
`147`	`151`	`predicate valueStep(Node nodeFrom, Node nodeTo) {`
`148`	`152`	`Flow::localFlowStep(asClosureNode(nodeFrom), asClosureNode(nodeTo))`
`149`	`153`	`}`
Original file line number	Diff line number	Diff line change
`@@ -453,6 +453,10 @@ module VariableCapture {`
`453`	`453`	`Flow::localFlowStep(asClosureNode(node1), asClosureNode(node2))`
`454`	`454`	`}`
`455`	`455`
	`456`	`+ predicate clearsContent(Node node, Content::CapturedVariableContent c) {`
	`457`	`+ Flow::clearsContent(asClosureNode(node), c.getVariable())`
	`458`	`+ }`
	`459`	`+`
`456`	`460`	`class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt {`
`457`	`461`	`CapturedSsaDefinitionExt() { this.getSourceVariable() instanceof CapturedVariable }`
`458`	`462`	`}`
`@@ -1930,6 +1934,8 @@ predicate clearsContent(Node n, ContentSet c) {`
`1930`	`1934`	`c.isKnownOrUnknownElement(TKnownElementContent(cv)) and`
`1931`	`1935`	`cv.isSymbol(name)`
`1932`	`1936`	`)`
	`1937`	`+ or`
	`1938`	`+ VariableCapture::clearsContent(n, any(Content::CapturedVariableContent v \| c.isSingleton(v)))`
`1933`	`1939`	`}`
`1934`	`1940`
`1935`	`1941`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -232,6 +232,9 @@ signature module OutputSig<LocationSig Location, InputSig<Location> I> {`
`232`	`232`
`233`	`233`	/** Holds if this-to-this summaries are expected for `c`. */
`234`	`234`	`predicate heuristicAllowInstanceParameterReturnInSelf(I::Callable c);`
	`235`	`+`
	`236`	+ /** Holds if captured variable `v` is cleared at `node`. */
	`237`	`+ predicate clearsContent(ClosureNode node, I::CapturedVariable v);`
`235`	`238`	`}`
`236`	`239`
`237`	`240`	`/**`
`@@ -959,4 +962,11 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig`
`959`	`962`	`)`
`960`	`963`	`)`
`961`	`964`	`}`
	`965`	`+`
	`966`	`+ predicate clearsContent(ClosureNode node, CapturedVariable v) {`
	`967`	`+ exists(BasicBlock bb, int i \|`
	`968`	`+ captureWrite(v, bb, i, false, _) and`
	`969`	`+ node = TSynthThisQualifier(bb, i, false)`
	`970`	`+ )`
	`971`	`+ }`
`962`	`972`	`}`