Skip to content

Commit 0f60401

Browse files
hvitvedhvitved
authored
Merge pull request #2513 from hvitved/csharp/null-maybe-capture
C#: Remove FPs from `cs/dereferenced-value-may-be-null`
2 parents 3791b15 + 984e01e commit 0f60401

File tree

7 files changed

+81
-17
lines changed

7 files changed

+81
-17
lines changed

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

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,20 +33,28 @@ class Guard extends Expr {
3333
}
3434

3535
/**
36-
* Holds if basic block `bb` is guarded by this expression having value `v`.
36+
* Holds if `cfn` is guarded by this expression having value `v`.
37+
*
38+
* Note: This predicate is inlined.
3739
*/
38-
predicate controlsBasicBlock(BasicBlock bb, AbstractValue v) {
39-
Internal::guardControls(this, bb, v)
40+
pragma[inline]
41+
predicate controlsNode(ControlFlow::Nodes::ElementNode cfn, AbstractValue v) {
42+
guardControls(this, cfn.getBasicBlock(), v)
4043
}
4144

45+
/**
46+
* Holds if basic block `bb` is guarded by this expression having value `v`.
47+
*/
48+
predicate controlsBasicBlock(BasicBlock bb, AbstractValue v) { guardControls(this, bb, v) }
49+
4250
/**
4351
* Holds if this guard is an equality test between `e1` and `e2`. If the test is
4452
* negated, that is `!=`, then `polarity` is false, otherwise `polarity` is
4553
* true.
4654
*/
4755
predicate isEquality(Expr e1, Expr e2, boolean polarity) {
4856
exists(BooleanValue v |
49-
this = Internal::getAnEqualityCheck(e1, v, e2) and
57+
this = getAnEqualityCheck(e1, v, e2) and
5058
polarity = v.getValue()
5159
)
5260
}

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,19 @@ private predicate isNullDefaultArgument(Ssa::ExplicitDefinition def, AlwaysNullE
177177
)
178178
}
179179

180+
/**
181+
* Holds if `edef` is an implicit entry definition for a captured variable that
182+
* may be guarded, because a call to the capturing callable is guarded.
183+
*/
184+
private predicate isMaybeGuardedCapturedDef(Ssa::ImplicitEntryDefinition edef) {
185+
exists(Ssa::ExplicitDefinition def, ControlFlow::Nodes::ElementNode c, G::Guard g, NullValue nv |
186+
def.isCapturedVariableDefinitionFlowIn(edef, c, _) and
187+
g = def.getARead() and
188+
g.controlsNode(c, nv) and
189+
nv.isNonNull()
190+
)
191+
}
192+
180193
/** Holds if `def` is an SSA definition that may be `null`. */
181194
private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason) {
182195
not nonNullDef(def) and
@@ -214,6 +227,7 @@ private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason)
214227
exists(Dereference d | dereferenceAt(_, _, def, d) |
215228
d.hasNullableType() and
216229
not def instanceof Ssa::PhiNode and
230+
not isMaybeGuardedCapturedDef(def) and
217231
reason = def.getSourceVariable().getAssignable() and
218232
msg = "because it has a nullable type"
219233
)

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

Lines changed: 33 additions & 13 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
int Ex38(int? i)
@@ -411,6 +411,26 @@ int Ex41()
411411
i ??= null;
412412
return i.Value; // GOOD
413413
}
414+
415+
static bool Ex42(int? i, IEnumerable<int> @is)
416+
{
417+
return @is.Any(j => j == i.Value); // BAD (maybe)
418+
}
419+
420+
static bool Ex43(int? i, IEnumerable<int> @is)
421+
{
422+
if (i.HasValue)
423+
return @is.Any(j => j == i.Value); // GOOD
424+
return false;
425+
}
426+
427+
static bool Ex44(int? i, IEnumerable<int> @is)
428+
{
429+
if (i.HasValue)
430+
@is = @is.Where(j => j == i.Value); // BAD (always) (FALSE NEGATIVE)
431+
i = null;
432+
return @is.Any();
433+
}
414434
}
415435

416436
public static class Extensions

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,12 @@
238238
| E.cs:384:27:384:36 | ... == ... | true | E.cs:384:33:384:36 | null | E.cs:384:27:384:28 | access to parameter e2 |
239239
| E.cs:386:16:386:33 | ... == ... | true | E.cs:386:16:386:22 | access to property Long | E.cs:386:27:386:33 | access to property Long |
240240
| E.cs:386:16:386:33 | ... == ... | true | E.cs:386:27:386:33 | access to property Long | E.cs:386:16:386:22 | access to property Long |
241+
| E.cs:417:29:417:40 | ... == ... | true | E.cs:417:29:417:29 | access to parameter j | E.cs:417:34:417:40 | access to property Value |
242+
| E.cs:417:29:417:40 | ... == ... | true | E.cs:417:34:417:40 | access to property Value | E.cs:417:29:417:29 | access to parameter j |
243+
| E.cs:423:33:423:44 | ... == ... | true | E.cs:423:33:423:33 | access to parameter j | E.cs:423:38:423:44 | access to property Value |
244+
| E.cs:423:33:423:44 | ... == ... | true | E.cs:423:38:423:44 | access to property Value | E.cs:423:33:423:33 | access to parameter j |
245+
| E.cs:430:34:430:45 | ... == ... | true | E.cs:430:34:430:34 | access to parameter j | E.cs:430:39:430:45 | access to property Value |
246+
| E.cs:430:34:430:45 | ... == ... | true | E.cs:430:39:430:45 | access to property Value | E.cs:430:34:430:34 | access to parameter j |
241247
| Forwarding.cs:59:13:59:21 | ... == ... | true | Forwarding.cs:59:13:59:13 | access to parameter o | Forwarding.cs:59:18:59:21 | null |
242248
| Forwarding.cs:59:13:59:21 | ... == ... | true | Forwarding.cs:59:18:59:21 | null | Forwarding.cs:59:13:59:13 | access to parameter o |
243249
| Forwarding.cs:78:16:78:39 | call to method ReferenceEquals | true | Forwarding.cs:78:32:78:32 | access to parameter o | Forwarding.cs:78:35:78:38 | null |

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1292,6 +1292,14 @@
12921292
| E.cs:411:9:411:18 | ... ?? ... | null | E.cs:411:15:411:18 | null | null |
12931293
| E.cs:412:16:412:16 | access to local variable i | non-null | E.cs:411:9:411:18 | ... ?? ... | non-null |
12941294
| E.cs:412:16:412:16 | access to local variable i | null | E.cs:411:9:411:18 | ... ?? ... | null |
1295+
| E.cs:417:16:417:41 | call to method Any<Int32> | true | E.cs:417:16:417:18 | access to parameter is | non-empty |
1296+
| E.cs:422:13:422:22 | access to property HasValue | false | E.cs:422:13:422:13 | access to parameter i | null |
1297+
| E.cs:422:13:422:22 | access to property HasValue | true | E.cs:422:13:422:13 | access to parameter i | non-null |
1298+
| E.cs:423:20:423:45 | call to method Any<Int32> | true | E.cs:423:20:423:22 | access to parameter is | non-empty |
1299+
| E.cs:429:13:429:22 | access to property HasValue | false | E.cs:429:13:429:13 | access to parameter i | null |
1300+
| E.cs:429:13:429:22 | access to property HasValue | true | E.cs:429:13:429:13 | access to parameter i | non-null |
1301+
| E.cs:432:16:432:24 | call to method Any<Int32> | false | E.cs:432:16:432:18 | access to parameter is | empty |
1302+
| E.cs:432:16:432:24 | call to method Any<Int32> | true | E.cs:432:16:432:18 | access to parameter is | non-empty |
12951303
| Forwarding.cs:9:13:9:30 | !... | false | Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | true |
12961304
| Forwarding.cs:9:13:9:30 | !... | true | Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | false |
12971305
| Forwarding.cs:9:14:9:14 | access to local variable s | empty | Forwarding.cs:7:20:7:23 | null | empty |

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,10 @@
294294
| E.cs:404:9:404:9 | access to local variable i | E.cs:404:9:404:9 | access to local variable i | null | true |
295295
| E.cs:411:9:411:9 | access to local variable i | E.cs:411:9:411:9 | access to local variable i | non-null | false |
296296
| E.cs:411:9:411:9 | access to local variable i | E.cs:411:9:411:9 | access to local variable i | null | true |
297+
| E.cs:422:13:422:22 | access to property HasValue | E.cs:422:13:422:13 | access to parameter i | false | true |
298+
| E.cs:422:13:422:22 | access to property HasValue | E.cs:422:13:422:13 | access to parameter i | true | false |
299+
| E.cs:429:13:429:22 | access to property HasValue | E.cs:429:13:429:13 | access to parameter i | false | true |
300+
| E.cs:429:13:429:22 | access to property HasValue | E.cs:429:13:429:13 | access to parameter i | true | false |
297301
| Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | Forwarding.cs:9:14:9:14 | access to local variable s | false | false |
298302
| Forwarding.cs:14:13:14:32 | call to method IsNotNullOrEmpty | Forwarding.cs:14:13:14:13 | access to local variable s | true | false |
299303
| Forwarding.cs:19:14:19:23 | call to method IsNull | Forwarding.cs:19:14:19:14 | access to local variable s | false | false |

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,8 @@ nodes
406406
| E.cs:404:9:404:18 | SSA def(i) |
407407
| E.cs:404:9:404:18 | SSA def(i) |
408408
| E.cs:405:16:405:16 | access to local variable i |
409+
| E.cs:417:24:417:40 | SSA capture def(i) |
410+
| E.cs:417:34:417:34 | access to parameter i |
409411
| Forwarding.cs:7:16:7:23 | SSA def(s) |
410412
| Forwarding.cs:9:13:9:30 | [false] !... |
411413
| Forwarding.cs:14:9:17:9 | if (...) ... |
@@ -795,6 +797,7 @@ edges
795797
| E.cs:384:27:384:28 | access to parameter e2 | E.cs:384:13:384:36 | [false] ... && ... |
796798
| E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
797799
| E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
800+
| E.cs:417:24:417:40 | SSA capture def(i) | E.cs:417:34:417:34 | access to parameter i |
798801
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:9:13:9:30 | [false] !... |
799802
| Forwarding.cs:9:13:9:30 | [false] !... | Forwarding.cs:14:9:17:9 | if (...) ... |
800803
| Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -907,6 +910,7 @@ edges
907910
| 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:28:382:37 | ... != ... | this |
908911
| 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 |
909912
| 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 |
913+
| E.cs:417:34:417:34 | access to parameter i | E.cs:417:24:417:40 | SSA capture def(i) | E.cs:417:34:417:34 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:415:27:415:27 | i | i | E.cs:415:27:415:27 | i | this |
910914
| 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 |
911915
| 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 |
912916
| 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)