Skip to content

Commit 3674966

Browse files
aschackmullaschackmull
authored
Merge pull request #20121 from aschackmull/guards/wrapperguard
Guards: Improve support for wrapped guards
2 parents dfe4401 + 2909def commit 3674966

File tree

7 files changed

+385
-140
lines changed

7 files changed

+385
-140
lines changed

‎java/ql/lib/change-notes/2025-07-28-guardwrappers.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Guard implication logic involving wrapper methods has been improved. In particular, this means fewer false positives for `java/dereferenced-value-may-be-null`.

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

Lines changed: 73 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,8 @@ private module GuardsInput implements SharedGuards::InputSig<Location> {
146146

147147
class ControlFlowNode = J::ControlFlowNode;
148148

149+
class NormalExitNode = ControlFlow::NormalExitNode;
150+
149151
class BasicBlock = J::BasicBlock;
150152

151153
predicate dominatingEdge(BasicBlock bb1, BasicBlock bb2) { J::dominatingEdge(bb1, bb2) }
@@ -322,6 +324,55 @@ private module GuardsInput implements SharedGuards::InputSig<Location> {
322324

323325
Expr getElse() { result = super.getFalseExpr() }
324326
}
327+
328+
class Parameter = J::Parameter;
329+
330+
private int parameterPosition() { result in [-1, any(Parameter p).getPosition()] }
331+
332+
/** A parameter position represented by an integer. */
333+
class ParameterPosition extends int {
334+
ParameterPosition() { this = parameterPosition() }
335+
}
336+
337+
/** An argument position represented by an integer. */
338+
class ArgumentPosition extends int {
339+
ArgumentPosition() { this = parameterPosition() }
340+
}
341+
342+
/** Holds if arguments at position `apos` match parameters at position `ppos`. */
343+
overlay[caller?]
344+
pragma[inline]
345+
predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
346+
347+
final private class FinalMethod = Method;
348+
349+
class NonOverridableMethod extends FinalMethod {
350+
NonOverridableMethod() { not super.isOverridable() }
351+
352+
Parameter getParameter(ParameterPosition ppos) {
353+
super.getParameter(ppos) = result and
354+
not result.isVarargs()
355+
}
356+
357+
GuardsInput::Expr getAReturnExpr() {
358+
exists(ReturnStmt ret |
359+
this = ret.getEnclosingCallable() and
360+
ret.getResult() = result
361+
)
362+
}
363+
}
364+
365+
private predicate nonOverridableMethodCall(MethodCall call, NonOverridableMethod m) {
366+
call.getMethod().getSourceDeclaration() = m
367+
}
368+
369+
class NonOverridableMethodCall extends GuardsInput::Expr instanceof MethodCall {
370+
NonOverridableMethodCall() { nonOverridableMethodCall(this, _) }
371+
372+
NonOverridableMethod getMethod() { nonOverridableMethodCall(this, result) }
373+
374+
GuardsInput::Expr getArgument(ArgumentPosition apos) { result = super.getArgument(apos) }
375+
}
325376
}
326377

327378
private module GuardsImpl = SharedGuards::Make<Location, GuardsInput>;
@@ -340,6 +391,17 @@ private module LogicInputCommon {
340391
NullGuards::nullCheckMethod(call.getMethod(), val.asBooleanValue(), isNull)
341392
)
342393
}
394+
395+
predicate additionalImpliesStep(
396+
GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
397+
) {
398+
exists(MethodCall check, int argIndex |
399+
g1 = check and
400+
v1.getDualValue().isThrowsException() and
401+
conditionCheckArgument(check, argIndex, v2.asBooleanValue()) and
402+
g2 = check.getArgument(argIndex)
403+
)
404+
}
343405
}
344406

345407
private module LogicInput_v1 implements GuardsImpl::LogicInputSig {
@@ -364,18 +426,13 @@ private module LogicInput_v1 implements GuardsImpl::LogicInputSig {
364426
}
365427
}
366428

429+
predicate parameterDefinition(Parameter p, SsaDefinition def) {
430+
def.(BaseSsaImplicitInit).isParameterDefinition(p)
431+
}
432+
367433
predicate additionalNullCheck = LogicInputCommon::additionalNullCheck/4;
368434

369-
predicate additionalImpliesStep(
370-
GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
371-
) {
372-
exists(MethodCall check, int argIndex |
373-
g1 = check and
374-
v1.getDualValue().isThrowsException() and
375-
conditionCheckArgument(check, argIndex, v2.asBooleanValue()) and
376-
g2 = check.getArgument(argIndex)
377-
)
378-
}
435+
predicate additionalImpliesStep = LogicInputCommon::additionalImpliesStep/4;
379436
}
380437

381438
private module LogicInput_v2 implements GuardsImpl::LogicInputSig {
@@ -400,15 +457,13 @@ private module LogicInput_v2 implements GuardsImpl::LogicInputSig {
400457
}
401458
}
402459

460+
predicate parameterDefinition(Parameter p, SsaDefinition def) {
461+
def.(SSA::SsaImplicitInit).isParameterDefinition(p)
462+
}
463+
403464
predicate additionalNullCheck = LogicInputCommon::additionalNullCheck/4;
404465

405-
predicate additionalImpliesStep(
406-
GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
407-
) {
408-
LogicInput_v1::additionalImpliesStep(g1, v1, g2, v2)
409-
or
410-
CustomGuard::additionalImpliesStep(g1, v1, g2, v2)
411-
}
466+
predicate additionalImpliesStep = LogicInputCommon::additionalImpliesStep/4;
412467
}
413468

414469
private module LogicInput_v3 implements GuardsImpl::LogicInputSig {
@@ -421,70 +476,11 @@ private module LogicInput_v3 implements GuardsImpl::LogicInputSig {
421476

422477
predicate additionalNullCheck = LogicInputCommon::additionalNullCheck/4;
423478

424-
predicate additionalImpliesStep = LogicInput_v2::additionalImpliesStep/4;
425-
}
426-
427-
private module CustomGuardInput implements Guards_v2::CustomGuardInputSig {
428-
private import semmle.code.java.dataflow.SSA
429-
430-
private int parameterPosition() { result in [-1, any(Parameter p).getPosition()] }
431-
432-
/** A parameter position represented by an integer. */
433-
class ParameterPosition extends int {
434-
ParameterPosition() { this = parameterPosition() }
435-
}
436-
437-
/** An argument position represented by an integer. */
438-
class ArgumentPosition extends int {
439-
ArgumentPosition() { this = parameterPosition() }
440-
}
441-
442-
/** Holds if arguments at position `apos` match parameters at position `ppos`. */
443-
overlay[caller?]
444-
pragma[inline]
445-
predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
446-
447-
final private class FinalMethod = Method;
448-
449-
class BooleanMethod extends FinalMethod {
450-
BooleanMethod() {
451-
super.getReturnType().(PrimitiveType).hasName("boolean") and
452-
not super.isOverridable()
453-
}
454-
455-
LogicInput_v2::SsaDefinition getParameter(ParameterPosition ppos) {
456-
exists(Parameter p |
457-
super.getParameter(ppos) = p and
458-
not p.isVarargs() and
459-
result.(SsaImplicitInit).isParameterDefinition(p)
460-
)
461-
}
462-
463-
GuardsInput::Expr getAReturnExpr() {
464-
exists(ReturnStmt ret |
465-
this = ret.getEnclosingCallable() and
466-
ret.getResult() = result
467-
)
468-
}
469-
}
470-
471-
private predicate booleanMethodCall(MethodCall call, BooleanMethod m) {
472-
call.getMethod().getSourceDeclaration() = m
473-
}
474-
475-
class BooleanMethodCall extends GuardsInput::Expr instanceof MethodCall {
476-
BooleanMethodCall() { booleanMethodCall(this, _) }
477-
478-
BooleanMethod getMethod() { booleanMethodCall(this, result) }
479-
480-
GuardsInput::Expr getArgument(ArgumentPosition apos) { result = super.getArgument(apos) }
481-
}
479+
predicate additionalImpliesStep = LogicInputCommon::additionalImpliesStep/4;
482480
}
483481

484482
class GuardValue = GuardsImpl::GuardValue;
485483

486-
private module CustomGuard = Guards_v2::CustomGuard<CustomGuardInput>;
487-
488484
/** INTERNAL: Don't use. */
489485
module Guards_v1 = GuardsImpl::Logic<LogicInput_v1>;
490486

‎java/ql/test/library-tests/guards/Guards.java

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,4 +143,73 @@ void t7(int[] a) {
143143
chk(); // $ guarded=found:true guarded='i < a.length:false'
144144
}
145145
}
146+
147+
public static boolean testNotNull1(String input) {
148+
return input != null && input.length() > 0;
149+
}
150+
151+
public static boolean testNotNull2(String input) {
152+
if (input == null) return false;
153+
return input.length() > 0;
154+
}
155+
156+
public static int getNumOrDefault(Integer number) {
157+
return number == null ? 0 : number;
158+
}
159+
160+
public static String concatNonNull(String s1, String s2) {
161+
if (s1 == null || s2 == null) return null;
162+
return s1 + s2;
163+
}
164+
165+
public static Status testEnumWrapper(boolean flag) {
166+
return flag ? Status.SUCCESS : Status.FAILURE;
167+
}
168+
169+
enum Status { SUCCESS, FAILURE }
170+
171+
void testWrappers(String s, Integer i) {
172+
if (testNotNull1(s)) {
173+
chk(); // $ guarded='s:not null' guarded=testNotNull1(...):true
174+
} else {
175+
chk(); // $ guarded=testNotNull1(...):false
176+
}
177+
178+
if (testNotNull2(s)) {
179+
chk(); // $ guarded='s:not null' guarded=testNotNull2(...):true
180+
} else {
181+
chk(); // $ guarded=testNotNull2(...):false
182+
}
183+
184+
if (0 == getNumOrDefault(i)) {
185+
chk(); // $ guarded='0 == getNumOrDefault(...):true' guarded='getNumOrDefault(...):0'
186+
} else {
187+
chk(); // $ guarded='0 == getNumOrDefault(...):false' guarded='getNumOrDefault(...):not 0' guarded='i:not 0' guarded='i:not null'
188+
}
189+
190+
if (null == concatNonNull(s, "suffix")) {
191+
chk(); // $ guarded='concatNonNull(...):null' guarded='null == concatNonNull(...):true'
192+
} else {
193+
chk(); // $ guarded='concatNonNull(...):not null' guarded='null == concatNonNull(...):false' guarded='s:not null'
194+
}
195+
196+
switch (testEnumWrapper(g(1))) {
197+
case SUCCESS:
198+
chk(); // $ guarded='testEnumWrapper(...):SUCCESS' guarded='testEnumWrapper(...):match SUCCESS' guarded=g(1):true
199+
break;
200+
case FAILURE:
201+
chk(); // $ guarded='testEnumWrapper(...):FAILURE' guarded='testEnumWrapper(...):match FAILURE' guarded=g(1):false
202+
break;
203+
}
204+
}
205+
206+
static void ensureNotNull(Object o) throws Exception {
207+
if (o == null) throw new Exception();
208+
}
209+
210+
void testExceptionWrapper(String s) throws Exception {
211+
chk(); // nothing guards here
212+
ensureNotNull(s);
213+
chk(); // $ guarded='ensureNotNull(...):no exception' guarded='s:not null'
214+
}
146215
}

‎java/ql/test/library-tests/guards/GuardsInline.expected

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,3 +89,28 @@
8989
| Guards.java:139:9:139:13 | chk(...) | found:true |
9090
| Guards.java:143:7:143:11 | chk(...) | 'i < a.length:false' |
9191
| Guards.java:143:7:143:11 | chk(...) | found:true |
92+
| Guards.java:173:7:173:11 | chk(...) | 's:not null' |
93+
| Guards.java:173:7:173:11 | chk(...) | testNotNull1(...):true |
94+
| Guards.java:175:7:175:11 | chk(...) | testNotNull1(...):false |
95+
| Guards.java:179:7:179:11 | chk(...) | 's:not null' |
96+
| Guards.java:179:7:179:11 | chk(...) | testNotNull2(...):true |
97+
| Guards.java:181:7:181:11 | chk(...) | testNotNull2(...):false |
98+
| Guards.java:185:7:185:11 | chk(...) | '0 == getNumOrDefault(...):true' |
99+
| Guards.java:185:7:185:11 | chk(...) | 'getNumOrDefault(...):0' |
100+
| Guards.java:187:7:187:11 | chk(...) | '0 == getNumOrDefault(...):false' |
101+
| Guards.java:187:7:187:11 | chk(...) | 'getNumOrDefault(...):not 0' |
102+
| Guards.java:187:7:187:11 | chk(...) | 'i:not 0' |
103+
| Guards.java:187:7:187:11 | chk(...) | 'i:not null' |
104+
| Guards.java:191:7:191:11 | chk(...) | 'concatNonNull(...):null' |
105+
| Guards.java:191:7:191:11 | chk(...) | 'null == concatNonNull(...):true' |
106+
| Guards.java:193:7:193:11 | chk(...) | 'concatNonNull(...):not null' |
107+
| Guards.java:193:7:193:11 | chk(...) | 'null == concatNonNull(...):false' |
108+
| Guards.java:193:7:193:11 | chk(...) | 's:not null' |
109+
| Guards.java:198:9:198:13 | chk(...) | 'testEnumWrapper(...):SUCCESS' |
110+
| Guards.java:198:9:198:13 | chk(...) | 'testEnumWrapper(...):match SUCCESS' |
111+
| Guards.java:198:9:198:13 | chk(...) | g(1):true |
112+
| Guards.java:201:9:201:13 | chk(...) | 'testEnumWrapper(...):FAILURE' |
113+
| Guards.java:201:9:201:13 | chk(...) | 'testEnumWrapper(...):match FAILURE' |
114+
| Guards.java:201:9:201:13 | chk(...) | g(1):false |
115+
| Guards.java:213:5:213:9 | chk(...) | 'ensureNotNull(...):no exception' |
116+
| Guards.java:213:5:213:9 | chk(...) | 's:not null' |

‎java/ql/test/query-tests/Nullness/C.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,7 +204,7 @@ public void ex14(int[] a) {
204204
obj = new Object();
205205
} else if (a[i] == 2) {
206206
verifyNotNull(obj);
207-
obj.hashCode(); // NPE - false positive
207+
obj.hashCode(); // OK
208208
}
209209
}
210210
}

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
| C.java:137:7:137:10 | obj2 | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:131:5:131:23 | Object obj2 | obj2 | C.java:132:9:132:20 | ... != ... | this |
3030
| C.java:144:15:144:15 | a | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:141:20:141:26 | a | a | C.java:142:13:142:21 | ... == ... | this |
3131
| C.java:188:9:188:11 | obj | Variable $@ may be null at this access because of $@ assignment. | C.java:181:5:181:22 | Object obj | obj | C.java:181:12:181:21 | obj | this |
32-
| C.java:207:9:207:11 | obj | Variable $@ may be null at this access because of $@ assignment. | C.java:201:5:201:22 | Object obj | obj | C.java:201:12:201:21 | obj | this |
3332
| C.java:219:9:219:10 | o1 | Variable $@ may be null at this access as suggested by $@ null guard. | C.java:212:20:212:28 | o1 | o1 | C.java:213:9:213:18 | ... == ... | this |
3433
| C.java:233:7:233:8 | xs | Variable $@ may be null at this access because of $@ assignment. | C.java:231:5:231:56 | int[] xs | xs | C.java:231:11:231:55 | xs | this |
3534
| F.java:11:5:11:7 | obj | Variable $@ may be null at this access as suggested by $@ null guard. | F.java:8:18:8:27 | obj | obj | F.java:9:9:9:19 | ... == ... | this |

0 commit comments

Comments
 (0)