Skip to content

Commit 9353c4a

Browse files
aschackmullaschackmull
committed
Guards: Generalise wrapper guards.
1 parent a8ad67e commit 9353c4a

File tree

4 files changed

+128
-29
lines changed

4 files changed

+128
-29
lines changed

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

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -344,11 +344,8 @@ private module GuardsInput implements SharedGuards::InputSig<Location> {
344344

345345
final private class FinalMethod = Method;
346346

347-
class BooleanMethod extends FinalMethod {
348-
BooleanMethod() {
349-
super.getReturnType().(PrimitiveType).hasName("boolean") and
350-
not super.isOverridable()
351-
}
347+
class NonOverridableMethod extends FinalMethod {
348+
NonOverridableMethod() { not super.isOverridable() }
352349

353350
Parameter getParameter(ParameterPosition ppos) {
354351
super.getParameter(ppos) = result and
@@ -363,14 +360,14 @@ private module GuardsInput implements SharedGuards::InputSig<Location> {
363360
}
364361
}
365362

366-
private predicate booleanMethodCall(MethodCall call, BooleanMethod m) {
363+
private predicate nonOverridableMethodCall(MethodCall call, NonOverridableMethod m) {
367364
call.getMethod().getSourceDeclaration() = m
368365
}
369366

370-
class BooleanMethodCall extends GuardsInput::Expr instanceof MethodCall {
371-
BooleanMethodCall() { booleanMethodCall(this, _) }
367+
class NonOverridableMethodCall extends GuardsInput::Expr instanceof MethodCall {
368+
NonOverridableMethodCall() { nonOverridableMethodCall(this, _) }
372369

373-
BooleanMethod getMethod() { booleanMethodCall(this, result) }
370+
NonOverridableMethod getMethod() { nonOverridableMethodCall(this, result) }
374371

375372
GuardsInput::Expr getArgument(ArgumentPosition apos) { result = super.getArgument(apos) }
376373
}

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

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,4 +143,63 @@ 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+
}
146205
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,3 +89,26 @@
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 |

‎shared/controlflow/codeql/controlflow/Guards.qll

Lines changed: 40 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -234,16 +234,16 @@ signature module InputSig<LocationSig Location> {
234234
*/
235235
predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos);
236236

237-
/** A non-overridable method with a boolean return value. */
238-
class BooleanMethod {
237+
/** A non-overridable method. */
238+
class NonOverridableMethod {
239239
Parameter getParameter(ParameterPosition ppos);
240240

241241
/** Gets an expression being returned by this method. */
242242
Expr getAReturnExpr();
243243
}
244244

245-
class BooleanMethodCall extends Expr {
246-
BooleanMethod getMethod();
245+
class NonOverridableMethodCall extends Expr {
246+
NonOverridableMethod getMethod();
247247

248248
Expr getArgument(ArgumentPosition apos);
249249
}
@@ -998,50 +998,69 @@ module Make<LocationSig Location, InputSig<Location> Input> {
998998
final private class FinalExpr = Expr;
999999

10001000
private class ReturnExpr extends FinalExpr {
1001-
ReturnExpr() { any(BooleanMethod m).getAReturnExpr() = this }
1001+
ReturnExpr() { any(NonOverridableMethod m).getAReturnExpr() = this }
1002+
1003+
NonOverridableMethod getMethod() { result.getAReturnExpr() = this }
10021004

10031005
pragma[nomagic]
10041006
BasicBlock getBasicBlock() { result = super.getBasicBlock() }
10051007
}
10061008

1007-
private predicate booleanReturnGuard(Guard guard, GuardValue val) {
1008-
guard instanceof ReturnExpr and exists(val.asBooleanValue())
1009+
private predicate relevantCallValue(NonOverridableMethodCall call, GuardValue val) {
1010+
BranchImplies::guardControls(call, val, _, _) or
1011+
ReturnImplies::guardControls(call, val, _, _)
1012+
}
1013+
1014+
private predicate relevantReturnValue(NonOverridableMethod m, GuardValue val) {
1015+
exists(NonOverridableMethodCall call |
1016+
relevantCallValue(call, val) and
1017+
call.getMethod() = m and
1018+
not val instanceof TException
1019+
)
1020+
}
1021+
1022+
private predicate returnGuard(Guard guard, GuardValue val) {
1023+
relevantReturnValue(guard.(ReturnExpr).getMethod(), val)
10091024
}
10101025

1011-
private module ReturnImplies = ImpliesTC<booleanReturnGuard/2>;
1026+
private module ReturnImplies = ImpliesTC<returnGuard/2>;
10121027

10131028
/**
10141029
* Holds if `ret` is a return expression in a non-overridable method that
10151030
* on a return value of `retval` allows the conclusion that the `ppos`th
10161031
* parameter has the value `val`.
10171032
*/
10181033
private predicate validReturnInCustomGuard(
1019-
ReturnExpr ret, ParameterPosition ppos, boolean retval, GuardValue val
1034+
ReturnExpr ret, ParameterPosition ppos, GuardValue retval, GuardValue val
10201035
) {
1021-
exists(BooleanMethod m, SsaDefinition param |
1036+
exists(NonOverridableMethod m, SsaDefinition param |
10221037
m.getAReturnExpr() = ret and
10231038
parameterDefinition(m.getParameter(ppos), param)
10241039
|
10251040
exists(Guard g0, GuardValue v0 |
10261041
g0.directlyValueControls(ret.getBasicBlock(), v0) and
10271042
BranchImplies::ssaControls(param, val, g0, v0) and
1028-
retval = [true, false]
1043+
relevantReturnValue(m, retval)
10291044
)
10301045
or
1031-
ReturnImplies::ssaControls(param, val, ret,
1032-
any(GuardValue r | r.asBooleanValue() = retval))
1046+
ReturnImplies::ssaControls(param, val, ret, retval)
10331047
)
10341048
}
10351049

10361050
/**
1037-
* Gets a non-overridable method with a boolean return value that performs a check
1038-
* on the `ppos`th parameter. A return value equal to `retval` allows us to conclude
1051+
* Gets a non-overridable method that performs a check on the `ppos`th
1052+
* parameter. A return value equal to `retval` allows us to conclude
10391053
* that the argument has the value `val`.
10401054
*/
1041-
private BooleanMethod customGuard(ParameterPosition ppos, boolean retval, GuardValue val) {
1055+
private NonOverridableMethod customGuard(
1056+
ParameterPosition ppos, GuardValue retval, GuardValue val
1057+
) {
10421058
forex(ReturnExpr ret |
10431059
result.getAReturnExpr() = ret and
1044-
not ret.(ConstantExpr).asBooleanValue() = retval.booleanNot()
1060+
not exists(GuardValue notRetval |
1061+
exprHasValue(ret, notRetval) and
1062+
disjointValues(notRetval, retval)
1063+
)
10451064
|
10461065
validReturnInCustomGuard(ret, ppos, retval, val)
10471066
)
@@ -1056,11 +1075,12 @@ module Make<LocationSig Location, InputSig<Location> Input> {
10561075
* custom guard wrappers.
10571076
*/
10581077
predicate additionalImpliesStep(PreGuard g1, GuardValue v1, PreGuard g2, GuardValue v2) {
1059-
exists(BooleanMethodCall call, ParameterPosition ppos, ArgumentPosition apos |
1078+
exists(NonOverridableMethodCall call, ParameterPosition ppos, ArgumentPosition apos |
10601079
g1 = call and
1061-
call.getMethod() = customGuard(ppos, v1.asBooleanValue(), v2) and
1080+
call.getMethod() = customGuard(ppos, v1, v2) and
10621081
call.getArgument(apos) = g2 and
1063-
parameterMatch(pragma[only_bind_out](ppos), pragma[only_bind_out](apos))
1082+
parameterMatch(pragma[only_bind_out](ppos), pragma[only_bind_out](apos)) and
1083+
not exprHasValue(g2, v2) // disregard trivial guard
10641084
)
10651085
}
10661086
}

0 commit comments

Comments
 (0)