Skip to content

Commit e4aa196

Browse files
hvitvedhvitved
committed
C#: Teach guards library about custom null guards
1 parent a5dfc10 commit e4aa196

File tree

5 files changed

+171
-33
lines changed

5 files changed

+171
-33
lines changed

‎csharp/ql/src/semmle/code/csharp/controlflow/ControlFlowGraph.qll‎

Lines changed: 35 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2473,7 +2473,7 @@ module ControlFlow {
24732473
not startsBB(succ)
24742474
}
24752475

2476-
predicate bbIndex(ControlFlowElement bbStart, ControlFlowElement cfe, int i) =
2476+
private predicate bbIndex(ControlFlowElement bbStart, ControlFlowElement cfe, int i) =
24772477
shortestDistances(startsBB/1, intraBBSucc/2)(bbStart, cfe, i)
24782478

24792479
private predicate succBB(PreBasicBlock pred, PreBasicBlock succ) {
@@ -2488,13 +2488,9 @@ module ControlFlow {
24882488
idominance(entryBB/1, succBB/2)(_, dom, bb)
24892489

24902490
class PreBasicBlock extends ControlFlowElement {
2491-
PreBasicBlock() {
2492-
startsBB(this)
2493-
}
2491+
PreBasicBlock() { startsBB(this) }
24942492

2495-
PreBasicBlock getASuccessor() {
2496-
result = succ(this.getLastElement(), _)
2497-
}
2493+
PreBasicBlock getASuccessor() { result = succ(this.getLastElement(), _) }
24982494

24992495
PreBasicBlock getAPredecessor() {
25002496
result.getASuccessor() = this
@@ -2548,6 +2544,22 @@ module ControlFlow {
25482544
exists(succExit(this.getLastElement(), c))
25492545
) > 1
25502546
}
2547+
2548+
private predicate isCandidateSuccessor(PreBasicBlock succ, ConditionalCompletion c) {
2549+
succ = succ(this.getLastElement(), c) and
2550+
forall(PreBasicBlock pred |
2551+
pred = succ.getAPredecessor() and pred != this |
2552+
succ.dominates(pred)
2553+
)
2554+
}
2555+
2556+
predicate controls(PreBasicBlock controlled, ConditionalSuccessor s) {
2557+
exists(PreBasicBlock succ, ConditionalCompletion c |
2558+
isCandidateSuccessor(succ, c) |
2559+
succ.dominates(controlled) and
2560+
s.matchesCompletion(c)
2561+
)
2562+
}
25512563
}
25522564
}
25532565

@@ -2729,29 +2741,21 @@ module ControlFlow {
27292741
ssaRefRank(bb2, i2, v, _) = 1
27302742
}
27312743

2732-
private cached module PreSsaCached {
2733-
cached
2734-
predicate firstReadSameVar(Definition def, LocalScopeVariableRead read) {
2735-
exists(SimpleLocalScopeVariable v, PreBasicBlock b1, int i1, PreBasicBlock b2, int i2 |
2736-
adjacentVarRefs(v, b1, i1, b2, i2) and
2737-
defAt(b1, i1, def, v) and
2738-
readAt(b2, i2, read, v)
2739-
)
2740-
}
2741-
2742-
cached
2743-
predicate adjacentReadPairSameVar(LocalScopeVariableRead read1, LocalScopeVariableRead read2) {
2744-
exists(SimpleLocalScopeVariable v, PreBasicBlock bb1, int i1, PreBasicBlock bb2, int i2 |
2745-
adjacentVarRefs(v, bb1, i1, bb2, i2) and
2746-
readAt(bb1, i1, read1, v) and
2747-
readAt(bb2, i2, read2, v)
2748-
)
2749-
}
2744+
predicate firstReadSameVar(Definition def, LocalScopeVariableRead read) {
2745+
exists(SimpleLocalScopeVariable v, PreBasicBlock b1, int i1, PreBasicBlock b2, int i2 |
2746+
adjacentVarRefs(v, b1, i1, b2, i2) and
2747+
defAt(b1, i1, def, v) and
2748+
readAt(b2, i2, read, v)
2749+
)
2750+
}
27502751

2751-
cached
2752-
predicate forceCachingInSameStage() { any() }
2752+
predicate adjacentReadPairSameVar(LocalScopeVariableRead read1, LocalScopeVariableRead read2) {
2753+
exists(SimpleLocalScopeVariable v, PreBasicBlock bb1, int i1, PreBasicBlock bb2, int i2 |
2754+
adjacentVarRefs(v, bb1, i1, bb2, i2) and
2755+
readAt(bb1, i1, read1, v) and
2756+
readAt(bb2, i2, read2, v)
2757+
)
27532758
}
2754-
import PreSsaCached
27552759
}
27562760

27572761
/**
@@ -3837,10 +3841,12 @@ module ControlFlow {
38373841
}
38383842

38393843
private cached module Cached {
3844+
private import semmle.code.csharp.controlflow.Guards as Guards
3845+
38403846
cached
38413847
newtype TPreSsaDef =
38423848
TExplicitPreSsaDef(PreBasicBlocks::PreBasicBlock bb, int i, AssignableDefinition def, LocalScopeVariable v) {
3843-
PreSsa::forceCachingInSameStage() and
3849+
Guards::Internal::CachedWithCFG::forceCachingInSameStage() and
38443850
PreSsa::assignableDefAt(bb, i, def, v)
38453851
}
38463852
or

‎csharp/ql/src/semmle/code/csharp/controlflow/Guards.qll‎

Lines changed: 115 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,11 @@ class DereferenceableExpr extends Expr {
162162
if ck.isInequality() then branch = false else branch = true
163163
)
164164
or
165-
// Call to `string.IsNullOrEmpty()`
166-
result = any(MethodCall mc |
167-
mc.getTarget() = any(SystemStringClass c).getIsNullOrEmptyMethod() and
165+
// Call to `string.IsNullOrEmpty()` or `string.IsNullOrWhiteSpace()`
166+
exists(MethodCall mc, string name |
167+
result = mc |
168+
mc.getTarget() = any(SystemStringClass c).getAMethod(name) and
169+
name.regexpMatch("IsNullOr(Empty|WhiteSpace)") and
168170
mc.getArgument(0) = this and
169171
branch = false and
170172
isNull = false
@@ -179,6 +181,8 @@ class DereferenceableExpr extends Expr {
179181
// E.g. `x is string` or `x is ""`
180182
(branch = true and isNull = false)
181183
)
184+
or
185+
isCustomNullCheck(result, this, v, isNull)
182186
)
183187
}
184188

@@ -491,6 +495,15 @@ module Internal {
491495
)
492496
}
493497

498+
/** Holds if pre basic block `bb` only is reached when `e` has abstract value `v`. */
499+
private predicate preControls(PreBasicBlocks::PreBasicBlock bb, Expr e, AbstractValue v) {
500+
exists(PreBasicBlocks::ConditionBlock cb, ConditionalSuccessor s, AbstractValue v0, Expr cond |
501+
cb.controls(bb, s) |
502+
v0.branchImplies(cb.getLastElement(), s, cond) and
503+
impliesSteps(cond, v0, e, v)
504+
)
505+
}
506+
494507
/**
495508
* Holds if assertion `a` directly asserts that expression `e` evaluates to
496509
* value `v`.
@@ -521,6 +534,86 @@ module Internal {
521534
)
522535
}
523536

537+
private Expr stripConditionalExpr(Expr e) {
538+
e = any(ConditionalExpr ce |
539+
result = stripConditionalExpr(ce.getThen())
540+
or
541+
result = stripConditionalExpr(ce.getElse())
542+
)
543+
or
544+
not e instanceof ConditionalExpr and
545+
result = e
546+
}
547+
548+
private predicate canReturn(Callable c, Expr ret) {
549+
exists(Expr e | c.canReturn(e) | ret = stripConditionalExpr(e))
550+
}
551+
552+
private class PreSsaImplicitParameterDefinition extends PreSsa::Definition {
553+
private Parameter p;
554+
555+
PreSsaImplicitParameterDefinition() {
556+
p = this.getDefinition().(AssignableDefinitions::ImplicitParameterDefinition).getParameter()
557+
}
558+
559+
Parameter getParameter() { result = p }
560+
561+
/**
562+
* Holds if the callable that this parameter belongs to can return `ret`, but
563+
* only if this parameter is `null` or non-`null`, as specified by `isNull`.
564+
*/
565+
predicate nullGuardedReturn(Expr ret, boolean isNull) {
566+
canReturn(p.getCallable(), ret) and
567+
exists(PreBasicBlocks::PreBasicBlock bb, NullValue nv |
568+
preControls(bb, this.getARead(), nv) |
569+
ret = bb.getAnElement() and
570+
if nv.isNull() then isNull = true else isNull = false
571+
)
572+
}
573+
}
574+
575+
/**
576+
* Holds if `ret` is an expression returned by the callable to which parameter
577+
* `p` belongs, and `ret` having Boolean value `retVal` allows the conclusion
578+
* that the parameter `p` either is `null` or non-`null`, as specified by `isNull`.
579+
*/
580+
private predicate validReturnInCustomNullCheck(Expr ret, Parameter p, BooleanValue retVal, boolean isNull) {
581+
exists(Callable c |
582+
canReturn(c, ret) |
583+
p.getCallable() = c and
584+
c.getReturnType() instanceof BoolType
585+
) and
586+
exists(PreSsaImplicitParameterDefinition def |
587+
p = def.getParameter() |
588+
def.nullGuardedReturn(ret, isNull)
589+
or
590+
exists(NullValue nv |
591+
impliesSteps(ret, retVal, def.getARead(), nv) |
592+
if nv.isNull() then isNull = true else isNull = false
593+
)
594+
)
595+
}
596+
597+
/**
598+
* Gets a non-overridable callable with a Boolean return value that performs a
599+
* `null`-check on parameter `p`. A return value having Boolean value `retVal`
600+
* allows us to conclude that the argument either is `null` or non-`null`, as
601+
* specified by `isNull`.
602+
*/
603+
private Callable customNullCheck(Parameter p, BooleanValue retVal, boolean isNull) {
604+
result.getReturnType() instanceof BoolType and
605+
not result.(Virtualizable).isOverridableOrImplementable() and
606+
p.getCallable() = result and
607+
not p.isParams() and
608+
p.getType() = any(Type t | t instanceof RefType or t instanceof NullableType) and
609+
forex(Expr ret |
610+
canReturn(result, ret) and
611+
not ret.(BoolLiteral).getBoolValue() = retVal.getValue().booleanNot()
612+
|
613+
validReturnInCustomNullCheck(ret, p, retVal, isNull)
614+
)
615+
}
616+
524617
private cached module Cached {
525618
pragma[noinline]
526619
private predicate isGuardedBy0(AccessOrCallExpr guarded, Expr e, AccessOrCallExpr sub, AbstractValue v) {
@@ -541,6 +634,21 @@ module Internal {
541634
guarded.getSsaQualifier() = sub.getSsaQualifier()
542635
)
543636
}
637+
}
638+
import Cached
639+
640+
// The predicates in this module should be cached in the same stage as the cache stage
641+
// in ControlFlowGraph.qll. This is to avoid recomputation of pre-basic-blocks and
642+
// pre-SSA predicates
643+
cached module CachedWithCFG {
644+
cached
645+
predicate isCustomNullCheck(Call call, Expr arg, BooleanValue v, boolean isNull) {
646+
exists(Callable callable, Parameter p |
647+
arg = call.getArgumentForParameter(any(Parameter p0 | p0.getSourceDeclaration() = p)) and
648+
call.getTarget().getSourceDeclaration() = callable and
649+
callable = customNullCheck(p, v, isNull)
650+
)
651+
}
544652

545653
/**
546654
* Holds if `e1` having abstract value `v1` implies that `e2` has abstract
@@ -659,8 +767,11 @@ module Internal {
659767
v1 instanceof NullValue and
660768
v1 = v2
661769
}
770+
771+
cached
772+
predicate forceCachingInSameStage() { any() }
662773
}
663-
import Cached
774+
import CachedWithCFG
664775

665776
/**
666777
* Holds if `e1` having some abstract value, `v`, implies that `e2` has the same

‎csharp/ql/test/library-tests/controlflow/guards/GuardedExpr.expected‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,12 +150,17 @@
150150
| Guards.cs:162:24:162:24 | access to parameter o | Guards.cs:151:17:151:17 | access to parameter o | Guards.cs:151:17:151:17 | access to parameter o | non-null |
151151
| Guards.cs:169:31:169:31 | access to parameter x | Guards.cs:168:13:168:41 | !... | Guards.cs:168:40:168:40 | access to parameter x | true |
152152
| Guards.cs:169:31:169:31 | access to parameter x | Guards.cs:168:14:168:41 | call to method IsNullOrWhiteSpace | Guards.cs:168:40:168:40 | access to parameter x | false |
153+
| Guards.cs:169:31:169:31 | access to parameter x | Guards.cs:168:40:168:40 | access to parameter x | Guards.cs:168:40:168:40 | access to parameter x | non-null |
153154
| Guards.cs:190:31:190:31 | access to parameter s | Guards.cs:189:13:189:25 | !... | Guards.cs:189:24:189:24 | access to parameter s | true |
154155
| Guards.cs:190:31:190:31 | access to parameter s | Guards.cs:189:14:189:25 | call to method NullTest1 | Guards.cs:189:24:189:24 | access to parameter s | false |
156+
| Guards.cs:190:31:190:31 | access to parameter s | Guards.cs:189:24:189:24 | access to parameter s | Guards.cs:189:24:189:24 | access to parameter s | non-null |
155157
| Guards.cs:192:31:192:31 | access to parameter s | Guards.cs:191:13:191:25 | !... | Guards.cs:191:24:191:24 | access to parameter s | true |
156158
| Guards.cs:192:31:192:31 | access to parameter s | Guards.cs:191:14:191:25 | call to method NullTest2 | Guards.cs:191:24:191:24 | access to parameter s | false |
159+
| Guards.cs:192:31:192:31 | access to parameter s | Guards.cs:191:24:191:24 | access to parameter s | Guards.cs:191:24:191:24 | access to parameter s | non-null |
157160
| Guards.cs:194:31:194:31 | access to parameter s | Guards.cs:193:13:193:25 | !... | Guards.cs:193:24:193:24 | access to parameter s | true |
158161
| Guards.cs:194:31:194:31 | access to parameter s | Guards.cs:193:14:193:25 | call to method NullTest3 | Guards.cs:193:24:193:24 | access to parameter s | false |
162+
| Guards.cs:194:31:194:31 | access to parameter s | Guards.cs:193:24:193:24 | access to parameter s | Guards.cs:193:24:193:24 | access to parameter s | non-null |
159163
| Guards.cs:196:31:196:31 | access to parameter s | Guards.cs:195:13:195:27 | call to method NotNullTest4 | Guards.cs:195:26:195:26 | access to parameter s | true |
164+
| Guards.cs:196:31:196:31 | access to parameter s | Guards.cs:195:26:195:26 | access to parameter s | Guards.cs:195:26:195:26 | access to parameter s | non-null |
160165
| Guards.cs:198:31:198:31 | access to parameter s | Guards.cs:197:13:197:29 | !... | Guards.cs:197:28:197:28 | access to parameter s | true |
161166
| Guards.cs:198:31:198:31 | access to parameter s | Guards.cs:197:14:197:29 | call to method NullTestWrong | Guards.cs:197:28:197:28 | access to parameter s | false |

‎csharp/ql/test/library-tests/controlflow/guards/Implications.expected‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ impliesStep
151151
| Guards.cs:168:13:168:41 | !... | false | Guards.cs:168:14:168:41 | call to method IsNullOrWhiteSpace | true |
152152
| Guards.cs:168:13:168:41 | !... | true | Guards.cs:168:14:168:41 | call to method IsNullOrWhiteSpace | false |
153153
| Guards.cs:168:14:168:41 | call to method IsNullOrWhiteSpace | false | Guards.cs:168:13:168:41 | !... | true |
154+
| Guards.cs:168:14:168:41 | call to method IsNullOrWhiteSpace | false | Guards.cs:168:40:168:40 | access to parameter x | non-null |
154155
| Guards.cs:168:14:168:41 | call to method IsNullOrWhiteSpace | true | Guards.cs:168:13:168:41 | !... | false |
155156
| Guards.cs:172:33:172:41 | ... == ... | false | Guards.cs:172:33:172:33 | access to parameter o | non-null |
156157
| Guards.cs:172:33:172:41 | ... == ... | true | Guards.cs:172:33:172:33 | access to parameter o | null |
@@ -167,7 +168,9 @@ impliesStep
167168
| Guards.cs:183:36:183:48 | !... | false | Guards.cs:183:37:183:48 | call to method NullTest3 | true |
168169
| Guards.cs:183:36:183:48 | !... | true | Guards.cs:183:37:183:48 | call to method NullTest3 | false |
169170
| Guards.cs:183:37:183:48 | call to method NullTest3 | false | Guards.cs:183:36:183:48 | !... | true |
171+
| Guards.cs:183:37:183:48 | call to method NullTest3 | false | Guards.cs:183:47:183:47 | access to parameter o | non-null |
170172
| Guards.cs:183:37:183:48 | call to method NullTest3 | true | Guards.cs:183:36:183:48 | !... | false |
173+
| Guards.cs:183:37:183:48 | call to method NullTest3 | true | Guards.cs:183:47:183:47 | access to parameter o | null |
171174
| Guards.cs:185:37:185:45 | ... == ... | false | Guards.cs:185:37:185:37 | access to parameter o | non-null |
172175
| Guards.cs:185:37:185:45 | ... == ... | false | Guards.cs:185:37:185:59 | ... ? ... : ... | true |
173176
| Guards.cs:185:37:185:45 | ... == ... | true | Guards.cs:185:37:185:37 | access to parameter o | null |
@@ -179,15 +182,23 @@ impliesStep
179182
| Guards.cs:189:13:189:25 | !... | false | Guards.cs:189:14:189:25 | call to method NullTest1 | true |
180183
| Guards.cs:189:13:189:25 | !... | true | Guards.cs:189:14:189:25 | call to method NullTest1 | false |
181184
| Guards.cs:189:14:189:25 | call to method NullTest1 | false | Guards.cs:189:13:189:25 | !... | true |
185+
| Guards.cs:189:14:189:25 | call to method NullTest1 | false | Guards.cs:189:24:189:24 | access to parameter s | non-null |
182186
| Guards.cs:189:14:189:25 | call to method NullTest1 | true | Guards.cs:189:13:189:25 | !... | false |
187+
| Guards.cs:189:14:189:25 | call to method NullTest1 | true | Guards.cs:189:24:189:24 | access to parameter s | null |
183188
| Guards.cs:191:13:191:25 | !... | false | Guards.cs:191:14:191:25 | call to method NullTest2 | true |
184189
| Guards.cs:191:13:191:25 | !... | true | Guards.cs:191:14:191:25 | call to method NullTest2 | false |
185190
| Guards.cs:191:14:191:25 | call to method NullTest2 | false | Guards.cs:191:13:191:25 | !... | true |
191+
| Guards.cs:191:14:191:25 | call to method NullTest2 | false | Guards.cs:191:24:191:24 | access to parameter s | non-null |
186192
| Guards.cs:191:14:191:25 | call to method NullTest2 | true | Guards.cs:191:13:191:25 | !... | false |
193+
| Guards.cs:191:14:191:25 | call to method NullTest2 | true | Guards.cs:191:24:191:24 | access to parameter s | null |
187194
| Guards.cs:193:13:193:25 | !... | false | Guards.cs:193:14:193:25 | call to method NullTest3 | true |
188195
| Guards.cs:193:13:193:25 | !... | true | Guards.cs:193:14:193:25 | call to method NullTest3 | false |
189196
| Guards.cs:193:14:193:25 | call to method NullTest3 | false | Guards.cs:193:13:193:25 | !... | true |
197+
| Guards.cs:193:14:193:25 | call to method NullTest3 | false | Guards.cs:193:24:193:24 | access to parameter s | non-null |
190198
| Guards.cs:193:14:193:25 | call to method NullTest3 | true | Guards.cs:193:13:193:25 | !... | false |
199+
| Guards.cs:193:14:193:25 | call to method NullTest3 | true | Guards.cs:193:24:193:24 | access to parameter s | null |
200+
| Guards.cs:195:13:195:27 | call to method NotNullTest4 | false | Guards.cs:195:26:195:26 | access to parameter s | null |
201+
| Guards.cs:195:13:195:27 | call to method NotNullTest4 | true | Guards.cs:195:26:195:26 | access to parameter s | non-null |
191202
| Guards.cs:197:13:197:29 | !... | false | Guards.cs:197:14:197:29 | call to method NullTestWrong | true |
192203
| Guards.cs:197:13:197:29 | !... | true | Guards.cs:197:14:197:29 | call to method NullTestWrong | false |
193204
| Guards.cs:197:14:197:29 | call to method NullTestWrong | false | Guards.cs:197:13:197:29 | !... | true |

‎csharp/ql/test/library-tests/controlflow/guards/NullGuardedExpr.expected‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,3 +34,8 @@
3434
| Guards.cs:154:24:154:24 | access to parameter o |
3535
| Guards.cs:158:24:158:24 | access to parameter o |
3636
| Guards.cs:162:24:162:24 | access to parameter o |
37+
| Guards.cs:169:31:169:31 | access to parameter x |
38+
| Guards.cs:190:31:190:31 | access to parameter s |
39+
| Guards.cs:192:31:192:31 | access to parameter s |
40+
| Guards.cs:194:31:194:31 | access to parameter s |
41+
| Guards.cs:196:31:196:31 | access to parameter s |

0 commit comments

Comments
 (0)