Skip to content

Commit 2837d25

Browse files
aschackmullaschackmull
authored
Merge pull request #17490 from aschackmull/java/capture-in-obinit
Java: Fix support for variable capture inside object initializers.
2 parents 295861d + a1a885e commit 2837d25

File tree

4 files changed

+130
-4
lines changed

4 files changed

+130
-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: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -248,4 +248,35 @@ void run() {
248248
sink(l.get(0)); // $ hasValueFlow=src
249249
sink(l2.get(0)); // $ hasValueFlow=src
250250
}
251+
252+
void testInstanceInitializer() {
253+
// Tests capture in the instance initializer ("<obinit>")
254+
String s = source("init");
255+
class MyLocal3 {
256+
String f = s;
257+
void run() {
258+
sink(this.f); // $ hasValueFlow=init
259+
}
260+
}
261+
new MyLocal3().run();
262+
}
263+
264+
void testConstructorIndirection() {
265+
// Tests capture in nested constructor call
266+
String s = source("init");
267+
class MyLocal4 {
268+
String f;
269+
MyLocal4() {
270+
this(42);
271+
}
272+
MyLocal4(int i) {
273+
f = s;
274+
}
275+
String get() {
276+
return this.f;
277+
}
278+
}
279+
sink(new MyLocal4().get()); // $ hasValueFlow=init
280+
sink(new MyLocal4(1).get()); // $ hasValueFlow=init
281+
}
251282
}

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

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,46 @@ 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:254:16:254:29 | source(...) : String | B.java:261:5:261:18 | String s : String | provenance | |
258+
| B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | B.java:255:11:255:18 | this <.method> : MyLocal3 [String s] : String | provenance | |
259+
| B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | B.java:256:18:256:18 | this : MyLocal3 [String s] : String | provenance | |
260+
| B.java:255:11:255:18 | this <.method> : MyLocal3 [String s] : String | B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | provenance | |
261+
| B.java:255:11:255:18 | this <.method> : MyLocal3 [String s] : String | B.java:255:11:255:18 | this <.method> [post update] : MyLocal3 [f] : String | provenance | |
262+
| B.java:255:11:255:18 | this <.method> [post update] : MyLocal3 [f] : String | B.java:255:11:255:18 | parameter this [Return] : MyLocal3 [f] : String | provenance | |
263+
| B.java:256:7:256:19 | this <.field> [post update] : MyLocal3 [f] : String | B.java:255:11:255:18 | parameter this [Return] : MyLocal3 [f] : String | provenance | |
264+
| B.java:256:18:256:18 | s : String | B.java:256:7:256:19 | this <.field> [post update] : MyLocal3 [f] : String | provenance | |
265+
| B.java:256:18:256:18 | this : MyLocal3 [String s] : String | B.java:256:18:256:18 | s : String | provenance | |
266+
| B.java:257:12:257:14 | parameter this : MyLocal3 [f] : String | B.java:258:14:258:17 | this : MyLocal3 [f] : String | provenance | |
267+
| B.java:258:14:258:17 | this : MyLocal3 [f] : String | B.java:258:14:258:19 | this.f | provenance | |
268+
| B.java:261:5:261:18 | String s : String | B.java:261:5:261:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | provenance | |
269+
| B.java:261:5:261:18 | new MyLocal3(...) : MyLocal3 [f] : String | B.java:257:12:257:14 | parameter this : MyLocal3 [f] : String | provenance | |
270+
| B.java:261:5:261:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | provenance | |
271+
| B.java:261:5:261:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | B.java:261:5:261:18 | new MyLocal3(...) : MyLocal3 [f] : String | provenance | |
272+
| B.java:266:16:266:29 | source(...) : String | B.java:279:10:279:23 | String s : String | provenance | |
273+
| B.java:266:16:266:29 | source(...) : String | B.java:280:10:280:24 | String s : String | provenance | |
274+
| B.java:269:7:269:14 | parameter this : MyLocal4 [String s] : String | B.java:270:9:270:17 | this <constr(this)> : MyLocal4 [String s] : String | provenance | |
275+
| B.java:270:9:270:17 | this <constr(this)> : MyLocal4 [String s] : String | B.java:270:9:270:17 | this <constr(this)> [post update] : MyLocal4 [f] : String | provenance | |
276+
| B.java:270:9:270:17 | this <constr(this)> : MyLocal4 [String s] : String | B.java:272:7:272:14 | parameter this : MyLocal4 [String s] : String | provenance | |
277+
| B.java:270:9:270:17 | this <constr(this)> [post update] : MyLocal4 [f] : String | B.java:269:7:269:14 | parameter this [Return] : MyLocal4 [f] : String | provenance | |
278+
| B.java:272:7:272:14 | parameter this : MyLocal4 [String s] : String | B.java:273:13:273:13 | this : MyLocal4 [String s] : String | provenance | |
279+
| B.java:273:9:273:9 | this <.field> [post update] : MyLocal4 [f] : String | B.java:272:7:272:14 | parameter this [Return] : MyLocal4 [f] : String | provenance | |
280+
| B.java:273:13:273:13 | s : String | B.java:273:9:273:9 | this <.field> [post update] : MyLocal4 [f] : String | provenance | |
281+
| B.java:273:13:273:13 | this : MyLocal4 [String s] : String | B.java:273:13:273:13 | s : String | provenance | |
282+
| B.java:275:14:275:16 | parameter this : MyLocal4 [f] : String | B.java:276:16:276:19 | this : MyLocal4 [f] : String | provenance | |
283+
| B.java:276:16:276:19 | this : MyLocal4 [f] : String | B.java:276:16:276:21 | this.f : String | provenance | |
284+
| B.java:279:10:279:23 | String s : String | B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [String s] : String | provenance | |
285+
| B.java:279:10:279:23 | String s : String | B.java:279:10:279:23 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | provenance | |
286+
| B.java:279:10:279:23 | String s : String | B.java:280:10:280:24 | String s : String | provenance | |
287+
| B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [String s] : String | B.java:279:10:279:23 | String s : String | provenance | |
288+
| B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [f] : String | B.java:275:14:275:16 | parameter this : MyLocal4 [f] : String | provenance | |
289+
| B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [f] : String | B.java:279:10:279:29 | get(...) | provenance | |
290+
| B.java:279:10:279:23 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | B.java:269:7:269:14 | parameter this : MyLocal4 [String s] : String | provenance | |
291+
| B.java:279:10:279:23 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [f] : String | provenance | |
292+
| B.java:280:10:280:24 | String s : String | B.java:280:10:280:24 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | provenance | |
293+
| B.java:280:10:280:24 | new MyLocal4(...) : MyLocal4 [f] : String | B.java:275:14:275:16 | parameter this : MyLocal4 [f] : String | provenance | |
294+
| B.java:280:10:280:24 | new MyLocal4(...) : MyLocal4 [f] : String | B.java:280:10:280:30 | get(...) | provenance | |
295+
| B.java:280:10:280:24 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | B.java:272:7:272:14 | parameter this : MyLocal4 [String s] : String | provenance | |
296+
| B.java:280:10:280:24 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | B.java:280:10:280:24 | new MyLocal4(...) : MyLocal4 [f] : String | provenance | |
257297
nodes
258298
| B.java:11:5:11:6 | l1 [post update] : ArrayList [<element>] : String | semmle.label | l1 [post update] : ArrayList [<element>] : String |
259299
| B.java:11:12:11:22 | source(...) : String | semmle.label | source(...) : String |
@@ -511,6 +551,45 @@ nodes
511551
| B.java:248:10:248:17 | get(...) | semmle.label | get(...) |
512552
| B.java:249:10:249:11 | l2 : ArrayList [<element>] : String | semmle.label | l2 : ArrayList [<element>] : String |
513553
| B.java:249:10:249:18 | get(...) | semmle.label | get(...) |
554+
| B.java:254:16:254:29 | source(...) : String | semmle.label | source(...) : String |
555+
| B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | semmle.label | parameter this : MyLocal3 [String s] : String |
556+
| B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | semmle.label | parameter this : MyLocal3 [String s] : String |
557+
| B.java:255:11:255:18 | parameter this [Return] : MyLocal3 [f] : String | semmle.label | parameter this [Return] : MyLocal3 [f] : String |
558+
| B.java:255:11:255:18 | parameter this [Return] : MyLocal3 [f] : String | semmle.label | parameter this [Return] : MyLocal3 [f] : String |
559+
| B.java:255:11:255:18 | this <.method> : MyLocal3 [String s] : String | semmle.label | this <.method> : MyLocal3 [String s] : String |
560+
| B.java:255:11:255:18 | this <.method> [post update] : MyLocal3 [f] : String | semmle.label | this <.method> [post update] : MyLocal3 [f] : String |
561+
| B.java:256:7:256:19 | this <.field> [post update] : MyLocal3 [f] : String | semmle.label | this <.field> [post update] : MyLocal3 [f] : String |
562+
| B.java:256:18:256:18 | s : String | semmle.label | s : String |
563+
| B.java:256:18:256:18 | this : MyLocal3 [String s] : String | semmle.label | this : MyLocal3 [String s] : String |
564+
| B.java:257:12:257:14 | parameter this : MyLocal3 [f] : String | semmle.label | parameter this : MyLocal3 [f] : String |
565+
| B.java:258:14:258:17 | this : MyLocal3 [f] : String | semmle.label | this : MyLocal3 [f] : String |
566+
| B.java:258:14:258:19 | this.f | semmle.label | this.f |
567+
| B.java:261:5:261:18 | String s : String | semmle.label | String s : String |
568+
| B.java:261:5:261:18 | new MyLocal3(...) : MyLocal3 [f] : String | semmle.label | new MyLocal3(...) : MyLocal3 [f] : String |
569+
| B.java:261:5:261:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | semmle.label | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String |
570+
| B.java:266:16:266:29 | source(...) : String | semmle.label | source(...) : String |
571+
| B.java:269:7:269:14 | parameter this : MyLocal4 [String s] : String | semmle.label | parameter this : MyLocal4 [String s] : String |
572+
| B.java:269:7:269:14 | parameter this [Return] : MyLocal4 [f] : String | semmle.label | parameter this [Return] : MyLocal4 [f] : String |
573+
| B.java:270:9:270:17 | this <constr(this)> : MyLocal4 [String s] : String | semmle.label | this <constr(this)> : MyLocal4 [String s] : String |
574+
| B.java:270:9:270:17 | this <constr(this)> [post update] : MyLocal4 [f] : String | semmle.label | this <constr(this)> [post update] : MyLocal4 [f] : String |
575+
| B.java:272:7:272:14 | parameter this : MyLocal4 [String s] : String | semmle.label | parameter this : MyLocal4 [String s] : String |
576+
| B.java:272:7:272:14 | parameter this [Return] : MyLocal4 [f] : String | semmle.label | parameter this [Return] : MyLocal4 [f] : String |
577+
| B.java:273:9:273:9 | this <.field> [post update] : MyLocal4 [f] : String | semmle.label | this <.field> [post update] : MyLocal4 [f] : String |
578+
| B.java:273:13:273:13 | s : String | semmle.label | s : String |
579+
| B.java:273:13:273:13 | this : MyLocal4 [String s] : String | semmle.label | this : MyLocal4 [String s] : String |
580+
| B.java:275:14:275:16 | parameter this : MyLocal4 [f] : String | semmle.label | parameter this : MyLocal4 [f] : String |
581+
| B.java:276:16:276:19 | this : MyLocal4 [f] : String | semmle.label | this : MyLocal4 [f] : String |
582+
| B.java:276:16:276:21 | this.f : String | semmle.label | this.f : String |
583+
| B.java:279:10:279:23 | String s : String | semmle.label | String s : String |
584+
| B.java:279:10:279:23 | String s : String | semmle.label | String s : String |
585+
| B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [String s] : String | semmle.label | new MyLocal4(...) : MyLocal4 [String s] : String |
586+
| B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [f] : String | semmle.label | new MyLocal4(...) : MyLocal4 [f] : String |
587+
| B.java:279:10:279:23 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | semmle.label | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String |
588+
| B.java:279:10:279:29 | get(...) | semmle.label | get(...) |
589+
| B.java:280:10:280:24 | String s : String | semmle.label | String s : String |
590+
| B.java:280:10:280:24 | new MyLocal4(...) : MyLocal4 [f] : String | semmle.label | new MyLocal4(...) : MyLocal4 [f] : String |
591+
| B.java:280:10:280:24 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | semmle.label | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String |
592+
| B.java:280:10:280:30 | get(...) | semmle.label | get(...) |
514593
subpaths
515594
| 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 |
516595
| 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 +609,11 @@ subpaths
530609
| 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(...) |
531610
| 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 |
532611
| 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 |
612+
| B.java:255:11:255:18 | this <.method> : MyLocal3 [String s] : String | B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | B.java:255:11:255:18 | parameter this [Return] : MyLocal3 [f] : String | B.java:255:11:255:18 | this <.method> [post update] : MyLocal3 [f] : String |
613+
| B.java:261:5:261:18 | new MyLocal3(...) [pre constructor] : MyLocal3 [String s] : String | B.java:255:11:255:18 | parameter this : MyLocal3 [String s] : String | B.java:255:11:255:18 | parameter this [Return] : MyLocal3 [f] : String | B.java:261:5:261:18 | new MyLocal3(...) : MyLocal3 [f] : String |
614+
| B.java:270:9:270:17 | this <constr(this)> : MyLocal4 [String s] : String | B.java:272:7:272:14 | parameter this : MyLocal4 [String s] : String | B.java:272:7:272:14 | parameter this [Return] : MyLocal4 [f] : String | B.java:270:9:270:17 | this <constr(this)> [post update] : MyLocal4 [f] : String |
615+
| B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [f] : String | B.java:275:14:275:16 | parameter this : MyLocal4 [f] : String | B.java:276:16:276:21 | this.f : String | B.java:279:10:279:29 | get(...) |
616+
| B.java:279:10:279:23 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | B.java:269:7:269:14 | parameter this : MyLocal4 [String s] : String | B.java:269:7:269:14 | parameter this [Return] : MyLocal4 [f] : String | B.java:279:10:279:23 | new MyLocal4(...) : MyLocal4 [f] : String |
617+
| B.java:280:10:280:24 | new MyLocal4(...) : MyLocal4 [f] : String | B.java:275:14:275:16 | parameter this : MyLocal4 [f] : String | B.java:276:16:276:21 | this.f : String | B.java:280:10:280:30 | get(...) |
618+
| B.java:280:10:280:24 | new MyLocal4(...) [pre constructor] : MyLocal4 [String s] : String | B.java:272:7:272:14 | parameter this : MyLocal4 [String s] : String | B.java:272:7:272:14 | parameter this [Return] : MyLocal4 [f] : String | B.java:280:10:280:24 | new MyLocal4(...) : MyLocal4 [f] : String |
533619
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)