Skip to content

Commit 12194a1

Browse files
hvitvedhvitved
committed
C#: Remove FPs from cs/dereferenced-value-may-be-null
Apply a conservative approach by filtering out results for accesses to captured nullable values, when there is an (implicit) call to the capturing callable which is `null`-guarded. For example: ``` bool M(int? i, IEnumerable<int> @is) { if (i.HasValue) return @is.Any(j => j == i.Value); // GOOD return false; } ```
1 parent ffb1fed commit 12194a1

File tree

4 files changed

+71
-51
lines changed

4 files changed

+71
-51
lines changed

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

Lines changed: 42 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,18 @@ class Guard extends Expr {
3131
predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AccessOrCallExpr sub, AbstractValue v) {
3232
isGuardedByNode(cfn, this, sub, v)
3333
}
34+
35+
/**
36+
* Holds if `cfn` is guarded by this expression having value `v`.
37+
*
38+
* Note: This predicate is inlined.
39+
*/
40+
pragma[inline]
41+
predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AbstractValue v) {
42+
guardControls(this, cfn.getBasicBlock(), v)
43+
or
44+
guardAssertionControlsNode(this, cfn, v)
45+
}
3446
}
3547

3648
/** An abstract value. */
@@ -936,34 +948,6 @@ module Internal {
936948
e = any(BinaryArithmeticOperation bao | result = bao.getAnOperand())
937949
}
938950

939-
/** Holds if basic block `bb` only is reached when guard `g` has abstract value `v`. */
940-
private predicate guardControls(Guard g, BasicBlock bb, AbstractValue v) {
941-
exists(ControlFlowElement cfe, ConditionalSuccessor s, AbstractValue v0, Guard g0 |
942-
cfe.controlsBlock(bb, s)
943-
|
944-
v0.branch(cfe, s, g0) and
945-
impliesSteps(g0, v0, g, v)
946-
)
947-
}
948-
949-
/**
950-
* Holds if control flow node `cfn` only is reached when guard `g` evaluates to `v`,
951-
* because of an assertion.
952-
*/
953-
private predicate guardAssertionControlsNode(Guard g, ControlFlow::Node cfn, AbstractValue v) {
954-
exists(Assertion a, Guard g0, AbstractValue v0 |
955-
asserts(a, g0, v0) and
956-
impliesSteps(g0, v0, g, v)
957-
|
958-
a.strictlyDominates(cfn.getBasicBlock())
959-
or
960-
exists(BasicBlock bb, int i, int j | bb.getNode(i) = a.getAControlFlowNode() |
961-
bb.getNode(j) = cfn and
962-
j > i
963-
)
964-
)
965-
}
966-
967951
/**
968952
* Holds if control flow element `cfe` only is reached when guard `g` evaluates to `v`,
969953
* because of an assertion.
@@ -1707,6 +1691,36 @@ module Internal {
17071691
adjacentReadPairSameVarUniquePredecessor(mc.getQualifier(), e)
17081692
)
17091693
}
1694+
1695+
/** Holds if basic block `bb` only is reached when guard `g` has abstract value `v`. */
1696+
cached
1697+
predicate guardControls(Guard g, BasicBlock bb, AbstractValue v) {
1698+
exists(ControlFlowElement cfe, ConditionalSuccessor s, AbstractValue v0, Guard g0 |
1699+
cfe.controlsBlock(bb, s)
1700+
|
1701+
v0.branch(cfe, s, g0) and
1702+
impliesSteps(g0, v0, g, v)
1703+
)
1704+
}
1705+
1706+
/**
1707+
* Holds if control flow node `cfn` only is reached when guard `g` evaluates to `v`,
1708+
* because of an assertion.
1709+
*/
1710+
cached
1711+
predicate guardAssertionControlsNode(Guard g, ControlFlow::Node cfn, AbstractValue v) {
1712+
exists(Assertion a, Guard g0, AbstractValue v0 |
1713+
asserts(a, g0, v0) and
1714+
impliesSteps(g0, v0, g, v)
1715+
|
1716+
a.strictlyDominates(cfn.getBasicBlock())
1717+
or
1718+
exists(BasicBlock bb, int i, int j | bb.getNode(i) = a.getAControlFlowNode() |
1719+
bb.getNode(j) = cfn and
1720+
j > i
1721+
)
1722+
)
1723+
}
17101724
}
17111725

17121726
import CachedWithCFG

‎csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,19 @@ private predicate isNullDefaultArgument(Ssa::ExplicitDefinition def, AlwaysNullE
195195
)
196196
}
197197

198+
/**
199+
* Holds if `edef` is an implicit entry definition for a captured variable that
200+
* may be guarded, because a call to the capturing callable is guarded.
201+
*/
202+
private predicate isMaybeGuardedCapturedDef(Ssa::ImplicitEntryDefinition edef) {
203+
exists(Ssa::ExplicitDefinition def, ControlFlow::Nodes::ElementNode c, G::Guard g, NullValue nv |
204+
def.isCapturedVariableDefinitionFlowIn(edef, c, _) and
205+
g = def.getARead() and
206+
g.controlsNode(c, nv) and
207+
nv.isNonNull()
208+
)
209+
}
210+
198211
/** Holds if `def` is an SSA definition that may be `null`. */
199212
private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason) {
200213
// A variable compared to `null` might be `null`
@@ -231,6 +244,7 @@ private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason)
231244
exists(Dereference d | dereferenceAt(_, _, def, d) |
232245
d.hasNullableType() and
233246
not def instanceof Ssa::PseudoDefinition and
247+
not isMaybeGuardedCapturedDef(def) and
234248
reason = def.getSourceVariable().getAssignable() and
235249
msg = "because it has a nullable type"
236250
)

‎csharp/ql/test/query-tests/Nullness/E.cs‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@ public void Ex1(long[][][] a1, int ix, int len)
99
long[][] a2 = null;
1010
var haveA2 = ix < len && (a2 = a1[ix]) != null;
1111
long[] a3 = null;
12-
var haveA3 = haveA2 && (a3 = a2[ix]) != null; // GOOD (false positive)
12+
var haveA3 = haveA2 && (a3 = a2[ix]) != null; // GOOD (FALSE POSITIVE)
1313
if (haveA3)
14-
a3[0] = 0; // GOOD (false positive)
14+
a3[0] = 0; // GOOD (FALSE POSITIVE)
1515
}
1616

1717
public void Ex2(bool x, bool y)
@@ -24,7 +24,7 @@ public void Ex2(bool x, bool y)
2424
s2 = (s1 == null) ? null : "";
2525
}
2626
if (s2 != null)
27-
s1.ToString(); // GOOD (false positive)
27+
s1.ToString(); // GOOD (FALSE POSITIVE)
2828
}
2929

3030
public void Ex3(IEnumerable<string> ss)
@@ -58,7 +58,7 @@ public void Ex4(IEnumerable<string> list, int step)
5858
slice = new List<string>();
5959
result.Add(slice);
6060
}
61-
slice.Add(str); // GOOD (false positive)
61+
slice.Add(str); // GOOD (FALSE POSITIVE)
6262
++index;
6363
}
6464
}
@@ -70,7 +70,7 @@ public void Ex5(bool hasArr, int[] arr)
7070
arrLen = arr == null ? 0 : arr.Length;
7171

7272
if (arrLen > 0)
73-
arr[0] = 0; // GOOD (false positive)
73+
arr[0] = 0; // GOOD (FALSE POSITIVE)
7474
}
7575

7676
public const int MY_CONST_A = 1;
@@ -109,7 +109,7 @@ public void Ex7(int[] arr1)
109109
arr2 = new int[arr1.Length];
110110

111111
for (var i = 0; i < arr1.Length; i++)
112-
arr2[i] = arr1[i]; // GOOD (false positive)
112+
arr2[i] = arr1[i]; // GOOD (FALSE POSITIVE)
113113
}
114114

115115
public void Ex8(int x, int lim)
@@ -122,7 +122,7 @@ public void Ex8(int x, int lim)
122122
int j = 0;
123123
while (!stop && j < lim)
124124
{
125-
int step = (j * obj.GetHashCode()) % 10; // GOOD (false positive)
125+
int step = (j * obj.GetHashCode()) % 10; // GOOD (FALSE POSITIVE)
126126
if (step == 0)
127127
{
128128
obj.ToString(); // GOOD
@@ -156,15 +156,15 @@ public void Ex9(bool cond, object obj1)
156156
cond = true;
157157
}
158158
if (cond)
159-
obj2.ToString(); // GOOD (false positive)
159+
obj2.ToString(); // GOOD (FALSE POSITIVE)
160160
}
161161

162162
public void Ex10(int[] a)
163163
{
164164
int n = a == null ? 0 : a.Length;
165165
for (var i = 0; i < n; i++)
166166
{
167-
int x = a[i]; // GOOD (false positive)
167+
int x = a[i]; // GOOD (FALSE POSITIVE)
168168
if (x > 7)
169169
a = new int[n];
170170
}
@@ -175,15 +175,15 @@ public void Ex11(object obj, bool b1)
175175
bool b2 = obj == null ? false : b1;
176176
if (b2 == null)
177177
{
178-
obj.ToString(); // GOOD (false positive)
178+
obj.ToString(); // GOOD (FALSE POSITIVE)
179179
}
180180
if (obj == null)
181181
{
182182
b1 = true;
183183
}
184184
if (b1 == null)
185185
{
186-
obj.ToString(); // GOOD (false positive)
186+
obj.ToString(); // GOOD (FALSE POSITIVE)
187187
}
188188
}
189189

@@ -372,7 +372,7 @@ static int Ex36(object o)
372372
if (o is string)
373373
{
374374
var s = o as string;
375-
return s.Length; // GOOD (false positive)
375+
return s.Length; // GOOD (FALSE POSITIVE)
376376
}
377377
return -1;
378378
}
@@ -383,7 +383,7 @@ static bool Ex37(E e1, E e2)
383383
return false;
384384
if (e1 == null && e2 == null)
385385
return true;
386-
return e1.Long == e2.Long; // GOOD (false positive)
386+
return e1.Long == e2.Long; // GOOD (FALSE POSITIVE)
387387
}
388388

389389
static bool Ex38(int? i, IEnumerable<int> @is)
@@ -394,14 +394,14 @@ static bool Ex38(int? i, IEnumerable<int> @is)
394394
static bool Ex39(int? i, IEnumerable<int> @is)
395395
{
396396
if (i.HasValue)
397-
return @is.Any(j => j == i.Value); // GOOD (FALSE POSITIVE)
397+
return @is.Any(j => j == i.Value); // GOOD
398398
return false;
399399
}
400400

401401
static bool Ex40(int? i, IEnumerable<int> @is)
402402
{
403403
if (i.HasValue)
404-
@is = @is.Where(j => j == i.Value); // BAD (always)
404+
@is = @is.Where(j => j == i.Value); // BAD (always) (FALSE NEGATIVE)
405405
i = null;
406406
return @is.Any();
407407
}

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -372,10 +372,6 @@ nodes
372372
| E.cs:386:27:386:28 | access to parameter e2 |
373373
| E.cs:391:24:391:40 | SSA capture def(i) |
374374
| E.cs:391:34:391:34 | access to parameter i |
375-
| E.cs:397:28:397:44 | SSA capture def(i) |
376-
| E.cs:397:38:397:38 | access to parameter i |
377-
| E.cs:404:29:404:45 | SSA capture def(i) |
378-
| E.cs:404:39:404:39 | access to parameter i |
379375
| Forwarding.cs:7:16:7:23 | SSA def(s) |
380376
| Forwarding.cs:14:9:17:9 | if (...) ... |
381377
| Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -726,8 +722,6 @@ edges
726722
| E.cs:384:9:385:24 | if (...) ... | E.cs:386:27:386:28 | access to parameter e2 |
727723
| E.cs:384:27:384:28 | access to parameter e2 | E.cs:386:16:386:17 | access to parameter e1 |
728724
| E.cs:391:24:391:40 | SSA capture def(i) | E.cs:391:34:391:34 | access to parameter i |
729-
| E.cs:397:28:397:44 | SSA capture def(i) | E.cs:397:38:397:38 | access to parameter i |
730-
| E.cs:404:29:404:45 | SSA capture def(i) | E.cs:404:39:404:39 | access to parameter i |
731725
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:14:9:17:9 | if (...) ... |
732726
| Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
733727
| Forwarding.cs:19:9:22:9 | if (...) ... | Forwarding.cs:24:9:27:9 | if (...) ... |
@@ -837,8 +831,6 @@ edges
837831
| E.cs:386:27:386:28 | access to parameter e2 | E.cs:380:30:380:31 | SSA param(e2) | E.cs:386:27:386:28 | access to parameter e2 | Variable $@ may be null here as suggested by $@ null check. | E.cs:380:30:380:31 | e2 | e2 | E.cs:382:58:382:67 | ... == ... | this |
838832
| E.cs:386:27:386:28 | access to parameter e2 | E.cs:380:30:380:31 | SSA param(e2) | E.cs:386:27:386:28 | access to parameter e2 | Variable $@ may be null here as suggested by $@ null check. | E.cs:380:30:380:31 | e2 | e2 | E.cs:384:27:384:36 | ... == ... | this |
839833
| E.cs:391:34:391:34 | access to parameter i | E.cs:391:24:391:40 | SSA capture def(i) | E.cs:391:34:391:34 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:389:27:389:27 | i | i | E.cs:389:27:389:27 | i | this |
840-
| E.cs:397:38:397:38 | access to parameter i | E.cs:397:28:397:44 | SSA capture def(i) | E.cs:397:38:397:38 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:394:27:394:27 | i | i | E.cs:394:27:394:27 | i | this |
841-
| E.cs:404:39:404:39 | access to parameter i | E.cs:404:29:404:45 | SSA capture def(i) | E.cs:404:39:404:39 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:401:27:401:27 | i | i | E.cs:401:27:401:27 | i | this |
842834
| GuardedString.cs:35:31:35:31 | access to local variable s | GuardedString.cs:7:16:7:32 | SSA def(s) | GuardedString.cs:35:31:35:31 | access to local variable s | Variable $@ may be null here because of $@ assignment. | GuardedString.cs:7:16:7:16 | s | s | GuardedString.cs:7:16:7:32 | String s = ... | this |
843835
| NullMaybeBad.cs:7:27:7:27 | access to parameter o | NullMaybeBad.cs:13:17:13:20 | null | NullMaybeBad.cs:7:27:7:27 | access to parameter o | Variable $@ may be null here because of $@ null argument. | NullMaybeBad.cs:5:25:5:25 | o | o | NullMaybeBad.cs:13:17:13:20 | null | this |
844836
| StringConcatenation.cs:16:17:16:17 | access to local variable s | StringConcatenation.cs:14:16:14:23 | SSA def(s) | StringConcatenation.cs:16:17:16:17 | access to local variable s | Variable $@ may be null here because of $@ assignment. | StringConcatenation.cs:14:16:14:16 | s | s | StringConcatenation.cs:14:16:14:23 | String s = ... | this |

0 commit comments

Comments
 (0)