Skip to content

Commit 20661a3

Browse files
aschackmullaschackmull
committed
Java: Fix support for variable capture inside object initializers.
1 parent add0332 commit 20661a3

File tree

4 files changed

+57
-4
lines changed

4 files changed

+57
-4
lines changed

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,14 @@ private module CaptureInput implements VariableCapture::InputSig<Location> {
147147
}
148148

149149
class Callable extends J::Callable {
150-
predicate isConstructor() { this instanceof Constructor }
150+
predicate isConstructor() {
151+
// InstanceInitializers are called from constructors and are equally likely
152+
// to capture variables for the purpose of field initialization, so we treat
153+
// them as constructors for the heuristic identification of whether to allow
154+
// this-to-this summaries.
155+
this instanceof Constructor or
156+
this instanceof InstanceInitializer
157+
}
151158
}
152159
}
153160

‎java/ql/test/library-tests/dataflow/capture/B.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,4 +248,15 @@ void run() {
248248
sink(l.get(0)); // $ hasValueFlow=src
249249
sink(l2.get(0)); // $ hasValueFlow=src
250250
}
251+
252+
void testInstanceInitializer() {
253+
String s = source("init");
254+
class MyLocal3 {
255+
String f = s;
256+
void run() {
257+
sink(this.f); // $ hasValueFlow=init
258+
}
259+
}
260+
new MyLocal3().run();
261+
}
251262
}

‎java/ql/test/library-tests/dataflow/capture/inlinetest.expected‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,21 @@ edges
254254
| B.java:247:5:247:18 | new MyLocal2(...) [pre constructor] : MyLocal2 [String s] : String | B.java:247:5:247:18 | new MyLocal2(...) : MyLocal2 [List<String> l, <element>] : String | provenance | MaD:2 |
255255
| B.java:248:10:248:10 | l : ArrayList [<element>] : String | B.java:248:10:248:17 | get(...) | provenance | MaD:3 |
256256
| B.java:249:10:249:11 | l2 : ArrayList [<element>] : String | B.java:249:10:249:18 | get(...) | provenance | MaD:3 |
257+
| B.java:253:16:253:29 | source(...) : String | B.java:260:5:260:18 | String s : String | provenance | |
258+
| B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | B.java:254:11:254:18 | this <.method> : MyLocal3 [String s] : String | provenance | |
259+
| B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | B.java:255:18:255:18 | this : MyLocal3 [String s] : String | provenance | |
260+
| B.java:254:11:254:18 | this <.method> : MyLocal3 [String s] : String | B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | provenance | |
261+
| B.java:254:11:254:18 | this <.method> : MyLocal3 [String s] : String | B.java:254:11:254:18 | this <.method> [post update] : MyLocal3 [f] : String | provenance | |
262+
| B.java:254:11:254:18 | this <.method> [post update] : MyLocal3 [f] : String | B.java:254:11:254:18 | parameter this [Return] : MyLocal3 [f] : String | provenance | |
263+
| B.java:255:7:255:19 | this <.field> [post update] : MyLocal3 [f] : String | B.java:254:11:254:18 | parameter this [Return] : MyLocal3 [f] : String | provenance | |
264+
| B.java:255:18:255:18 | s : String | B.java:255:7:255:19 | this <.field> [post update] : MyLocal3 [f] : String | provenance | |
265+
| B.java:255:18:255:18 | this : MyLocal3 [String s] : String | B.java:255:18:255:18 | s : String | provenance | |
266+
| B.java:256:12:256:14 | parameter this : MyLocal3 [f] : String | B.java:257:14:257:17 | this : MyLocal3 [f] : String | provenance | |
267+
| B.java:257:14:257:17 | this : MyLocal3 [f] : String | B.java:257:14:257:19 | this.f | provenance | |
268+
| B.java:260:5:260:18 | String s : String | B.java:260:5:260:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | provenance | |
269+
| B.java:260:5:260:18 | new MyLocal3(...) : MyLocal3 [f] : String | B.java:256:12:256:14 | parameter this : MyLocal3 [f] : String | provenance | |
270+
| B.java:260:5:260:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | provenance | |
271+
| B.java:260:5:260:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | B.java:260:5:260:18 | new MyLocal3(...) : MyLocal3 [f] : String | provenance | |
257272
nodes
258273
| B.java:11:5:11:6 | l1 [post update] : ArrayList [<element>] : String | semmle.label | l1 [post update] : ArrayList [<element>] : String |
259274
| B.java:11:12:11:22 | source(...) : String | semmle.label | source(...) : String |
@@ -511,6 +526,22 @@ nodes
511526
| B.java:248:10:248:17 | get(...) | semmle.label | get(...) |
512527
| B.java:249:10:249:11 | l2 : ArrayList [<element>] : String | semmle.label | l2 : ArrayList [<element>] : String |
513528
| B.java:249:10:249:18 | get(...) | semmle.label | get(...) |
529+
| B.java:253:16:253:29 | source(...) : String | semmle.label | source(...) : String |
530+
| B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | semmle.label | parameter this : MyLocal3 [String s] : String |
531+
| B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | semmle.label | parameter this : MyLocal3 [String s] : String |
532+
| B.java:254:11:254:18 | parameter this [Return] : MyLocal3 [f] : String | semmle.label | parameter this [Return] : MyLocal3 [f] : String |
533+
| B.java:254:11:254:18 | parameter this [Return] : MyLocal3 [f] : String | semmle.label | parameter this [Return] : MyLocal3 [f] : String |
534+
| B.java:254:11:254:18 | this <.method> : MyLocal3 [String s] : String | semmle.label | this <.method> : MyLocal3 [String s] : String |
535+
| B.java:254:11:254:18 | this <.method> [post update] : MyLocal3 [f] : String | semmle.label | this <.method> [post update] : MyLocal3 [f] : String |
536+
| B.java:255:7:255:19 | this <.field> [post update] : MyLocal3 [f] : String | semmle.label | this <.field> [post update] : MyLocal3 [f] : String |
537+
| B.java:255:18:255:18 | s : String | semmle.label | s : String |
538+
| B.java:255:18:255:18 | this : MyLocal3 [String s] : String | semmle.label | this : MyLocal3 [String s] : String |
539+
| B.java:256:12:256:14 | parameter this : MyLocal3 [f] : String | semmle.label | parameter this : MyLocal3 [f] : String |
540+
| B.java:257:14:257:17 | this : MyLocal3 [f] : String | semmle.label | this : MyLocal3 [f] : String |
541+
| B.java:257:14:257:19 | this.f | semmle.label | this.f |
542+
| B.java:260:5:260:18 | String s : String | semmle.label | String s : String |
543+
| B.java:260:5:260:18 | new MyLocal3(...) : MyLocal3 [f] : String | semmle.label | new MyLocal3(...) : MyLocal3 [f] : String |
544+
| B.java:260:5:260:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | semmle.label | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String |
514545
subpaths
515546
| B.java:13:5:13:6 | l1 : ArrayList [<element>] : String | B.java:13:16:13:16 | e : String | B.java:13:16:13:29 | parameter this [Return] : new Consumer<String>(...) { ... } [List<String> l2, <element>] : String | B.java:13:16:13:29 | ...->... [post update] : new Consumer<String>(...) { ... } [List<String> l2, <element>] : String |
516547
| B.java:30:14:30:24 | source(...) : String | B.java:22:26:22:26 | x : String | B.java:22:26:22:71 | parameter this [Return] : new Consumer<String>(...) { ... } [B other, bf1] : String | B.java:30:5:30:5 | f [post update] : new Consumer<String>(...) { ... } [B other, bf1] : String |
@@ -530,4 +561,6 @@ subpaths
530561
| B.java:178:10:178:11 | m2 : MyLocal [List<String> l, <element>] : String | B.java:169:14:169:16 | parameter this : MyLocal [List<String> l, <element>] : String | B.java:170:16:170:23 | get(...) : String | B.java:178:10:178:17 | get(...) |
531562
| B.java:247:5:247:18 | new MyLocal2(...) : MyLocal2 [List<String> l, <element>] : String | B.java:240:12:240:14 | parameter this : MyLocal2 [List<String> l, <element>] : String | B.java:240:12:240:14 | parameter this [Return] : MyLocal2 [List<String> l2, <element>] : String | B.java:247:5:247:18 | new MyLocal2(...) [post update] : MyLocal2 [List<String> l2, <element>] : String |
532563
| B.java:247:5:247:18 | new MyLocal2(...) [pre constructor] : MyLocal2 [String s] : String | B.java:235:7:235:14 | parameter this : MyLocal2 [String s] : String | B.java:235:7:235:14 | parameter this [Return] : MyLocal2 [List<String> l, <element>] : String | B.java:247:5:247:18 | new MyLocal2(...) : MyLocal2 [List<String> l, <element>] : String |
564+
| B.java:254:11:254:18 | this <.method> : MyLocal3 [String s] : String | B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | B.java:254:11:254:18 | parameter this [Return] : MyLocal3 [f] : String | B.java:254:11:254:18 | this <.method> [post update] : MyLocal3 [f] : String |
565+
| B.java:260:5:260:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | B.java:254:11:254:18 | parameter this : MyLocal3 [String s] : String | B.java:254:11:254:18 | parameter this [Return] : MyLocal3 [f] : String | B.java:260:5:260:18 | new MyLocal3(...) : MyLocal3 [f] : String |
533566
testFailures

‎shared/dataflow/codeql/dataflow/VariableCapture.qll‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -585,11 +585,13 @@ module Flow<LocationSig Location, InputSig<Location> Input> implements OutputSig
585585
2 <= strictcount(CapturedVariable v | captureAccess(v, c))
586586
or
587587
// Constructors that capture a variable may assign it to a field, which also
588-
// entails a this-to-this summary.
589-
captureAccess(_, c) and c.isConstructor()
588+
// entails a this-to-this summary. If there are multiple constructors, then
589+
// they might call each other, so if one constructor captures a variable we
590+
// allow this-to-this summaries for all of them.
591+
exists(ClosureExpr ce | ce.hasBody(c) and c.isConstructor() and hasConstructorCapture(ce, _))
590592
}
591593

592-
/** Holds if the constructor, if any, for the closure defined by `ce` captures `v`. */
594+
/** Holds if a constructor, if any, for the closure defined by `ce` captures `v`. */
593595
private predicate hasConstructorCapture(ClosureExpr ce, CapturedVariable v) {
594596
exists(Callable c | ce.hasBody(c) and c.isConstructor() and captureAccess(v, c))
595597
}

0 commit comments

Comments
 (0)