Skip to content

Commit 632713c

Browse files
hvitvedhvitved
authored
Merge pull request github#3986 from hvitved/csharp/null-maybe-null-coalescing-assignment
C#: Fix false-positives in `cs/dereferenced-value-may-be-null`
2 parents ddbec50 + 05307b8 commit 632713c

File tree

8 files changed

+108
-36
lines changed

8 files changed

+108
-36
lines changed

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -876,6 +876,8 @@ module Internal {
876876
not e.(QualifiableExpr).isConditional()
877877
or
878878
e instanceof SuppressNullableWarningExpr
879+
or
880+
e.stripCasts().getType() = any(ValueType t | not t instanceof NullableType)
879881
}
880882

881883
/** Holds if expression `e2` is a non-`null` value whenever `e1` is. */

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

Lines changed: 37 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -197,42 +197,44 @@ private predicate isNullDefaultArgument(Ssa::ExplicitDefinition def, AlwaysNullE
197197

198198
/** Holds if `def` is an SSA definition that may be `null`. */
199199
private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason) {
200-
// A variable compared to `null` might be `null`
201-
exists(G::DereferenceableExpr de | de = def.getARead() |
202-
reason = de.getANullCheck(_, true) and
203-
msg = "as suggested by $@ null check" and
204-
not de = any(Ssa::PseudoDefinition pdef).getARead() and
205-
strictcount(Element e | e = any(Ssa::Definition def0 | de = def0.getARead()).getElement()) = 1 and
206-
not nonNullDef(def) and
207-
// Don't use a check as reason if there is a `null` assignment
208-
// or argument
209-
not def.(Ssa::ExplicitDefinition).getADefinition().getSource() instanceof MaybeNullExpr and
210-
not isMaybeNullArgument(def, _)
211-
)
212-
or
213-
// A parameter might be `null` if there is a `null` argument somewhere
214-
isMaybeNullArgument(def, reason) and
200+
not nonNullDef(def) and
215201
(
216-
if reason instanceof AlwaysNullExpr
217-
then msg = "because of $@ null argument"
218-
else msg = "because of $@ potential null argument"
219-
)
220-
or
221-
isNullDefaultArgument(def, reason) and msg = "because the parameter has a null default value"
222-
or
223-
// If the source of a variable is `null` then the variable may be `null`
224-
exists(AssignableDefinition adef | adef = def.(Ssa::ExplicitDefinition).getADefinition() |
225-
adef.getSource() instanceof MaybeNullExpr and
226-
reason = adef.getExpr() and
227-
msg = "because of $@ assignment"
228-
)
229-
or
230-
// A variable of nullable type may be null
231-
exists(Dereference d | dereferenceAt(_, _, def, d) |
232-
d.hasNullableType() and
233-
not def instanceof Ssa::PseudoDefinition and
234-
reason = def.getSourceVariable().getAssignable() and
235-
msg = "because it has a nullable type"
202+
// A variable compared to `null` might be `null`
203+
exists(G::DereferenceableExpr de | de = def.getARead() |
204+
reason = de.getANullCheck(_, true) and
205+
msg = "as suggested by $@ null check" and
206+
not de = any(Ssa::PseudoDefinition pdef).getARead() and
207+
strictcount(Element e | e = any(Ssa::Definition def0 | de = def0.getARead()).getElement()) = 1 and
208+
// Don't use a check as reason if there is a `null` assignment
209+
// or argument
210+
not def.(Ssa::ExplicitDefinition).getADefinition().getSource() instanceof MaybeNullExpr and
211+
not isMaybeNullArgument(def, _)
212+
)
213+
or
214+
// A parameter might be `null` if there is a `null` argument somewhere
215+
isMaybeNullArgument(def, reason) and
216+
(
217+
if reason instanceof AlwaysNullExpr
218+
then msg = "because of $@ null argument"
219+
else msg = "because of $@ potential null argument"
220+
)
221+
or
222+
isNullDefaultArgument(def, reason) and msg = "because the parameter has a null default value"
223+
or
224+
// If the source of a variable is `null` then the variable may be `null`
225+
exists(AssignableDefinition adef | adef = def.(Ssa::ExplicitDefinition).getADefinition() |
226+
adef.getSource() instanceof MaybeNullExpr and
227+
reason = adef.getExpr() and
228+
msg = "because of $@ assignment"
229+
)
230+
or
231+
// A variable of nullable type may be null
232+
exists(Dereference d | dereferenceAt(_, _, def, d) |
233+
d.hasNullableType() and
234+
not def instanceof Ssa::PseudoDefinition and
235+
reason = def.getSourceVariable().getAssignable() and
236+
msg = "because it has a nullable type"
237+
)
236238
)
237239
}
238240

‎csharp/ql/test/library-tests/cil/dataflow/Nullness.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,10 @@ alwaysNull
88
| dataflow.cs:80:21:80:44 | access to property NullProperty |
99
| dataflow.cs:89:31:89:44 | call to method NullFunction |
1010
alwaysNotNull
11+
| dataflow.cs:71:13:71:20 | access to local variable nonNull1 |
12+
| dataflow.cs:71:13:71:35 | Int32 nonNull1 = ... |
1113
| dataflow.cs:71:24:71:35 | default(...) |
14+
| dataflow.cs:71:32:71:34 | access to type Int32 |
1215
| dataflow.cs:72:27:72:30 | this access |
1316
| dataflow.cs:72:27:72:40 | call to method GetType |
1417
| dataflow.cs:73:30:73:33 | true |
@@ -25,6 +28,7 @@ alwaysNotNull
2528
| dataflow.cs:85:24:85:30 | access to local variable nonNull |
2629
| dataflow.cs:85:24:85:55 | call to method ReturnsNonNullIndirect |
2730
| dataflow.cs:86:24:86:30 | access to local variable nonNull |
31+
| dataflow.cs:89:24:89:27 | access to field cond |
2832
| dataflow.cs:89:24:89:27 | this access |
2933
| dataflow.cs:89:31:89:44 | this access |
3034
| dataflow.cs:89:48:89:51 | this access |

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

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,32 @@ static bool Ex37(E e1, E e2)
385385
return true;
386386
return e1.Long == e2.Long; // GOOD (false positive)
387387
}
388+
389+
int Ex38(int? i)
390+
{
391+
i ??= 0;
392+
return i.Value; // GOOD
393+
}
394+
395+
System.Drawing.Color Ex39(System.Drawing.Color? color)
396+
{
397+
color ??= System.Drawing.Color.White;
398+
return color.Value; // GOOD
399+
}
400+
401+
int Ex40()
402+
{
403+
int? i = null;
404+
i ??= null;
405+
return i.Value; // BAD (always)
406+
}
407+
408+
int Ex41()
409+
{
410+
int? i = 1;
411+
i ??= null;
412+
return i.Value; // GOOD
413+
}
388414
}
389415

390416
public static class Extensions
@@ -393,4 +419,4 @@ public static void M1(this string s) { }
393419
public static int M2(this string s) => s.Length;
394420
}
395421

396-
// semmle-extractor-options: /r:System.Linq.dll
422+
// semmle-extractor-options: /r:System.Linq.dll /r:System.Drawing.Primitives.dll

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1115,12 +1115,14 @@
11151115
| E.cs:176:13:176:14 | access to local variable b2 | true | E.cs:175:19:175:42 | ... ? ... : ... | true |
11161116
| E.cs:176:13:176:22 | ... == ... | false | E.cs:176:13:176:14 | (...) ... | non-null |
11171117
| E.cs:176:13:176:22 | ... == ... | true | E.cs:176:13:176:14 | (...) ... | null |
1118+
| E.cs:176:13:176:22 | ... == ... | true | E.cs:176:19:176:22 | null | non-null |
11181119
| E.cs:180:13:180:23 | ... == ... | false | E.cs:180:13:180:15 | access to parameter obj | non-null |
11191120
| E.cs:180:13:180:23 | ... == ... | true | E.cs:180:13:180:15 | access to parameter obj | null |
11201121
| E.cs:184:13:184:14 | (...) ... | non-null | E.cs:184:13:184:14 | access to parameter b1 | non-null |
11211122
| E.cs:184:13:184:14 | (...) ... | null | E.cs:184:13:184:14 | access to parameter b1 | null |
11221123
| E.cs:184:13:184:22 | ... == ... | false | E.cs:184:13:184:14 | (...) ... | non-null |
11231124
| E.cs:184:13:184:22 | ... == ... | true | E.cs:184:13:184:14 | (...) ... | null |
1125+
| E.cs:184:13:184:22 | ... == ... | true | E.cs:184:19:184:22 | null | non-null |
11241126
| E.cs:193:19:193:29 | call to method ToString | non-null | E.cs:193:17:193:17 | access to parameter o | non-null |
11251127
| E.cs:198:17:198:29 | ... ? ... : ... | non-null | E.cs:198:17:198:17 | access to parameter b | false |
11261128
| E.cs:198:17:198:29 | ... ? ... : ... | non-null | E.cs:198:28:198:29 | "" | non-null |
@@ -1256,6 +1258,26 @@
12561258
| E.cs:384:13:384:36 | ... && ... | true | E.cs:384:27:384:36 | ... == ... | true |
12571259
| E.cs:384:27:384:36 | ... == ... | false | E.cs:384:27:384:28 | access to parameter e2 | non-null |
12581260
| E.cs:384:27:384:36 | ... == ... | true | E.cs:384:27:384:28 | access to parameter e2 | null |
1261+
| E.cs:404:9:404:9 | access to local variable i | non-null | E.cs:403:18:403:21 | null | non-null |
1262+
| E.cs:404:9:404:9 | access to local variable i | null | E.cs:403:18:403:21 | null | null |
1263+
| E.cs:404:9:404:18 | ... = ... | non-null | E.cs:404:9:404:9 | access to local variable i | non-null |
1264+
| E.cs:404:9:404:18 | ... = ... | non-null | E.cs:404:9:404:18 | ... ?? ... | non-null |
1265+
| E.cs:404:9:404:18 | ... = ... | null | E.cs:404:9:404:9 | access to local variable i | null |
1266+
| E.cs:404:9:404:18 | ... = ... | null | E.cs:404:9:404:18 | ... ?? ... | null |
1267+
| E.cs:404:9:404:18 | ... ?? ... | null | E.cs:404:9:404:9 | access to local variable i | null |
1268+
| E.cs:404:9:404:18 | ... ?? ... | null | E.cs:404:15:404:18 | null | null |
1269+
| E.cs:405:16:405:16 | access to local variable i | non-null | E.cs:404:9:404:18 | ... ?? ... | non-null |
1270+
| E.cs:405:16:405:16 | access to local variable i | null | E.cs:404:9:404:18 | ... ?? ... | null |
1271+
| E.cs:411:9:411:9 | access to local variable i | non-null | E.cs:410:18:410:18 | (...) ... | non-null |
1272+
| E.cs:411:9:411:9 | access to local variable i | null | E.cs:410:18:410:18 | (...) ... | null |
1273+
| E.cs:411:9:411:18 | ... = ... | non-null | E.cs:411:9:411:9 | access to local variable i | non-null |
1274+
| E.cs:411:9:411:18 | ... = ... | non-null | E.cs:411:9:411:18 | ... ?? ... | non-null |
1275+
| E.cs:411:9:411:18 | ... = ... | null | E.cs:411:9:411:9 | access to local variable i | null |
1276+
| E.cs:411:9:411:18 | ... = ... | null | E.cs:411:9:411:18 | ... ?? ... | null |
1277+
| E.cs:411:9:411:18 | ... ?? ... | null | E.cs:411:9:411:9 | access to local variable i | null |
1278+
| E.cs:411:9:411:18 | ... ?? ... | null | E.cs:411:15:411:18 | null | null |
1279+
| E.cs:412:16:412:16 | access to local variable i | non-null | E.cs:411:9:411:18 | ... ?? ... | non-null |
1280+
| E.cs:412:16:412:16 | access to local variable i | null | E.cs:411:9:411:18 | ... ?? ... | null |
12591281
| Forwarding.cs:9:13:9:30 | !... | false | Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | true |
12601282
| Forwarding.cs:9:13:9:30 | !... | true | Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | false |
12611283
| 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/NullAlways.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
| E.cs:323:13:323:14 | access to parameter s1 | Variable $@ is always null here. | E.cs:319:29:319:30 | s1 | s1 |
3737
| E.cs:324:13:324:14 | access to parameter s2 | Variable $@ is always null here. | E.cs:319:40:319:41 | s2 | s2 |
3838
| E.cs:331:9:331:9 | access to local variable x | Variable $@ is always null here. | E.cs:330:13:330:13 | x | x |
39+
| E.cs:405:16:405:16 | access to local variable i | Variable $@ is always null here. | E.cs:403:14:403:14 | i | i |
3940
| Forwarding.cs:36:31:36:31 | access to local variable s | Variable $@ is always null here. | Forwarding.cs:7:16:7:16 | s | s |
4041
| Forwarding.cs:40:27:40:27 | access to local variable s | Variable $@ is always null here. | Forwarding.cs:7:16:7:16 | s | s |
4142
| NullAlwaysBad.cs:9:30:9:30 | access to parameter s | Variable $@ is always null here. | NullAlwaysBad.cs:7:29:7:29 | s | s |

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,10 +219,12 @@
219219
| E.cs:175:19:175:29 | ... == ... | E.cs:175:19:175:21 | access to parameter obj | true | true |
220220
| E.cs:176:13:176:22 | ... == ... | E.cs:176:13:176:14 | (...) ... | false | false |
221221
| E.cs:176:13:176:22 | ... == ... | E.cs:176:13:176:14 | (...) ... | true | true |
222+
| E.cs:176:13:176:22 | ... == ... | E.cs:176:19:176:22 | null | true | false |
222223
| E.cs:180:13:180:23 | ... == ... | E.cs:180:13:180:15 | access to parameter obj | false | false |
223224
| E.cs:180:13:180:23 | ... == ... | E.cs:180:13:180:15 | access to parameter obj | true | true |
224225
| E.cs:184:13:184:22 | ... == ... | E.cs:184:13:184:14 | (...) ... | false | false |
225226
| E.cs:184:13:184:22 | ... == ... | E.cs:184:13:184:14 | (...) ... | true | true |
227+
| E.cs:184:13:184:22 | ... == ... | E.cs:184:19:184:22 | null | true | false |
226228
| E.cs:193:17:193:17 | access to parameter o | E.cs:193:17:193:17 | access to parameter o | non-null | false |
227229
| E.cs:193:17:193:17 | access to parameter o | E.cs:193:17:193:17 | access to parameter o | null | true |
228230
| E.cs:208:13:208:23 | ... is ... | E.cs:208:13:208:13 | access to parameter s | false | true |
@@ -276,6 +278,14 @@
276278
| E.cs:384:13:384:22 | ... == ... | E.cs:384:13:384:14 | access to parameter e1 | true | true |
277279
| E.cs:384:27:384:36 | ... == ... | E.cs:384:27:384:28 | access to parameter e2 | false | false |
278280
| E.cs:384:27:384:36 | ... == ... | E.cs:384:27:384:28 | access to parameter e2 | true | true |
281+
| E.cs:391:9:391:9 | access to parameter i | E.cs:391:9:391:9 | access to parameter i | non-null | false |
282+
| E.cs:391:9:391:9 | access to parameter i | E.cs:391:9:391:9 | access to parameter i | null | true |
283+
| E.cs:397:9:397:13 | access to parameter color | E.cs:397:9:397:13 | access to parameter color | non-null | false |
284+
| E.cs:397:9:397:13 | access to parameter color | E.cs:397:9:397:13 | access to parameter color | null | true |
285+
| E.cs:404:9:404:9 | access to local variable i | E.cs:404:9:404:9 | access to local variable i | non-null | false |
286+
| E.cs:404:9:404:9 | access to local variable i | E.cs:404:9:404:9 | access to local variable i | null | true |
287+
| 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 |
288+
| E.cs:411:9:411:9 | access to local variable i | E.cs:411:9:411:9 | access to local variable i | null | true |
279289
| Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | Forwarding.cs:9:14:9:14 | access to local variable s | false | false |
280290
| Forwarding.cs:14:13:14:32 | call to method IsNotNullOrEmpty | Forwarding.cs:14:13:14:13 | access to local variable s | true | false |
281291
| 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: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,9 @@ nodes
370370
| E.cs:384:27:384:28 | access to parameter e2 |
371371
| E.cs:386:16:386:17 | access to parameter e1 |
372372
| E.cs:386:27:386:28 | access to parameter e2 |
373+
| E.cs:404:9:404:18 | SSA def(i) |
374+
| E.cs:404:9:404:18 | SSA def(i) |
375+
| E.cs:405:16:405:16 | access to local variable i |
373376
| Forwarding.cs:7:16:7:23 | SSA def(s) |
374377
| Forwarding.cs:14:9:17:9 | if (...) ... |
375378
| Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -719,6 +722,8 @@ edges
719722
| E.cs:384:9:385:24 | if (...) ... | E.cs:384:27:384:28 | access to parameter e2 |
720723
| E.cs:384:9:385:24 | if (...) ... | E.cs:386:27:386:28 | access to parameter e2 |
721724
| E.cs:384:27:384:28 | access to parameter e2 | E.cs:386:16:386:17 | access to parameter e1 |
725+
| E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
726+
| E.cs:404:9:404:18 | SSA def(i) | E.cs:405:16:405:16 | access to local variable i |
722727
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:14:9:17:9 | if (...) ... |
723728
| Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
724729
| Forwarding.cs:19:9:22:9 | if (...) ... | Forwarding.cs:24:9:27:9 | if (...) ... |

0 commit comments

Comments
 (0)