Skip to content

Commit 4be995d

Browse files
authored
Merge pull request #20267 from aschackmull/java/nullness-fix
Java: Add more nullness tests and fix a bug causing false negatives.
2 parents 891ce62 + e343fd3 commit 4be995d

File tree

5 files changed

+95
-1
lines changed

5 files changed

+95
-1
lines changed

java/ql/lib/semmle/code/java/controlflow/Guards.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ private predicate isNonFallThroughPredecessor(SwitchCase sc, ControlFlowNode pre
141141

142142
private module GuardsInput implements SharedGuards::InputSig<Location> {
143143
private import java as J
144+
private import semmle.code.java.dataflow.internal.BaseSSA
144145
private import semmle.code.java.dataflow.NullGuards as NullGuards
145146
import SuccessorType
146147

@@ -216,6 +217,12 @@ private module GuardsInput implements SharedGuards::InputSig<Location> {
216217
f.isFinal() and
217218
f.getInitializer() = NullGuards::baseNotNullExpr()
218219
)
220+
or
221+
exists(CatchClause cc, LocalVariableDeclExpr decl, BaseSsaUpdate v |
222+
decl = cc.getVariable() and
223+
decl = v.getDefiningExpr() and
224+
this = v.getAUse()
225+
)
219226
}
220227
}
221228

java/ql/lib/semmle/code/java/dataflow/Nullness.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -653,7 +653,7 @@ private Expr trackingVarGuard(
653653
result = integerGuard(trackvar.getAnAccess(), branch, k, isA)
654654
or
655655
exists(int k2 |
656-
result = integerGuard(trackvar.getAnAccess(), branch.booleanNot(), k2, true) and
656+
result = integerGuard(trackvar.getAnAccess(), branch, k2, true) and
657657
isA = false and
658658
k2 != k
659659
)
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Fixed a bug that was causing false negatives in rare cases in the query `java/dereferenced-value-may-be-null`.

java/ql/test/query-tests/Nullness/B.java

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,4 +436,83 @@ public void corrCondLoop2(boolean a[]) {
436436
}
437437
}
438438
}
439+
440+
public void loopCorrTest1(int[] a) {
441+
boolean ready = a.length > 7;
442+
Object x = new Object();
443+
for (int i = 0; i < a.length; i++) {
444+
// condition correlates with itself through iterations when ready isn't updated
445+
if (!ready) {
446+
x = null;
447+
} else {
448+
x.hashCode(); // Spurious NPE - false positive
449+
}
450+
if ((a[i] & 1) != 0) {
451+
ready = (a[i] & 2) != 0;
452+
x = new Object();
453+
}
454+
}
455+
}
456+
457+
public void loopCorrTest2(boolean[] a) {
458+
Object x = new Object();
459+
boolean cur = a[0];
460+
for (int i = 1; i < a.length; i++) {
461+
boolean prev = cur;
462+
cur = a[i];
463+
if (!prev) {
464+
// correctly guarded by !cur from the _previous_ iteration
465+
x.hashCode(); // Spurious NPE - false positive
466+
} else {
467+
x = new Object();
468+
}
469+
if (cur) {
470+
x = null;
471+
}
472+
}
473+
}
474+
475+
public void loopCorrTest3(String[] ss) {
476+
Object x = null;
477+
Object t = null;
478+
for (String s : ss) {
479+
if (t == null) {
480+
t = s;
481+
} else {
482+
if (t instanceof String) {
483+
x = new Object();
484+
t = new Object();
485+
}
486+
// correctly guarded by t: null -> String -> Object
487+
x.hashCode(); // Spurious NPE - false positive
488+
}
489+
}
490+
}
491+
492+
public void initCorr(boolean b) {
493+
Object o2 = b ? null : "";
494+
if (b)
495+
o2 = "";
496+
else
497+
o2.hashCode(); // OK
498+
}
499+
500+
public void complexLoopTest(int[] xs, int[] ys) {
501+
int len = ys != null ? ys.length : 0;
502+
for (int i = 0, j = 0; i < xs.length; i++) {
503+
if (j < len && ys[j] == 42) { // OK
504+
j++;
505+
} else if (j > 0) {
506+
ys[0]++; // OK
507+
}
508+
}
509+
}
510+
511+
public void trackTest(Object o, int n) {
512+
boolean isnull = o == null;
513+
int c = -1;
514+
if (maybe) { }
515+
if (c == 100) { return; }
516+
o.hashCode(); // NPE
517+
}
439518
}

java/ql/test/query-tests/Nullness/NullMaybe.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@
1818
| B.java:279:7:279:7 | a | Variable $@ may be null at this access because of $@ assignment. | B.java:276:5:276:19 | int[] a | a | B.java:276:11:276:18 | a | this |
1919
| B.java:292:7:292:7 | b | Variable $@ may be null at this access because of $@ assignment. | B.java:287:5:287:44 | int[] b | b | B.java:287:11:287:43 | b | this |
2020
| B.java:408:7:408:7 | x | Variable $@ may be null at this access as suggested by $@ null guard. | B.java:374:23:374:30 | x | x | B.java:375:23:375:31 | ... != ... | this |
21+
| B.java:448:9:448:9 | x | Variable $@ may be null at this access because of $@ assignment. | B.java:442:5:442:28 | Object x | x | B.java:446:9:446:16 | ...=... | this |
22+
| B.java:465:9:465:9 | x | Variable $@ may be null at this access because of $@ assignment. | B.java:458:5:458:28 | Object x | x | B.java:470:9:470:16 | ...=... | this |
23+
| B.java:487:9:487:9 | x | Variable $@ may be null at this access because of $@ assignment. | B.java:476:5:476:20 | Object x | x | B.java:476:12:476:19 | x | this |
24+
| B.java:516:5:516:5 | o | Variable $@ may be null at this access as suggested by $@ null guard. | B.java:511:25:511:32 | o | o | B.java:512:22:512:30 | ... == ... | this |
2125
| C.java:9:44:9:45 | a2 | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:6:5:6:23 | long[][] a2 | a2 | C.java:7:34:7:54 | ... != ... | this |
2226
| C.java:9:44:9:45 | a2 | Variable $@ may be null at this access because of $@ assignment. | C.java:6:5:6:23 | long[][] a2 | a2 | C.java:6:14:6:22 | a2 | this |
2327
| C.java:10:17:10:18 | a3 | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:8:5:8:21 | long[] a3 | a3 | C.java:9:38:9:58 | ... != ... | this |

0 commit comments

Comments
 (0)