Skip to content

Commit 984e01e

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 7948d96 commit 984e01e

File tree

4 files changed

+41
-27
lines changed

4 files changed

+41
-27
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: 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
int Ex38(int? i)
@@ -420,14 +420,14 @@ static bool Ex42(int? i, IEnumerable<int> @is)
420420
static bool Ex43(int? i, IEnumerable<int> @is)
421421
{
422422
if (i.HasValue)
423-
return @is.Any(j => j == i.Value); // GOOD (FALSE POSITIVE)
423+
return @is.Any(j => j == i.Value); // GOOD
424424
return false;
425425
}
426426

427427
static bool Ex44(int? i, IEnumerable<int> @is)
428428
{
429429
if (i.HasValue)
430-
@is = @is.Where(j => j == i.Value); // BAD (always)
430+
@is = @is.Where(j => j == i.Value); // BAD (always) (FALSE NEGATIVE)
431431
i = null;
432432
return @is.Any();
433433
}

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -408,10 +408,6 @@ nodes
408408
| E.cs:405:16:405:16 | access to local variable i |
409409
| E.cs:417:24:417:40 | SSA capture def(i) |
410410
| E.cs:417:34:417:34 | access to parameter i |
411-
| E.cs:423:28:423:44 | SSA capture def(i) |
412-
| E.cs:423:38:423:38 | access to parameter i |
413-
| E.cs:430:29:430:45 | SSA capture def(i) |
414-
| E.cs:430:39:430:39 | access to parameter i |
415411
| Forwarding.cs:7:16:7:23 | SSA def(s) |
416412
| Forwarding.cs:9:13:9:30 | [false] !... |
417413
| Forwarding.cs:14:9:17:9 | if (...) ... |
@@ -802,8 +798,6 @@ edges
802798
| E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
803799
| E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
804800
| E.cs:417:24:417:40 | SSA capture def(i) | E.cs:417:34:417:34 | access to parameter i |
805-
| E.cs:423:28:423:44 | SSA capture def(i) | E.cs:423:38:423:38 | access to parameter i |
806-
| E.cs:430:29:430:45 | SSA capture def(i) | E.cs:430:39:430:39 | access to parameter i |
807801
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:9:13:9:30 | [false] !... |
808802
| Forwarding.cs:9:13:9:30 | [false] !... | Forwarding.cs:14:9:17:9 | if (...) ... |
809803
| Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -917,8 +911,6 @@ edges
917911
| 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 |
918912
| 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 |
919913
| 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 |
920-
| E.cs:423:38:423:38 | access to parameter i | E.cs:423:28:423:44 | SSA capture def(i) | E.cs:423:38:423:38 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:420:27:420:27 | i | i | E.cs:420:27:420:27 | i | this |
921-
| E.cs:430:39:430:39 | access to parameter i | E.cs:430:29:430:45 | SSA capture def(i) | E.cs:430:39:430:39 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:427:27:427:27 | i | i | E.cs:427:27:427:27 | i | this |
922914
| 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 |
923915
| 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 |
924916
| 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)