Skip to content

Commit d39a336

Browse files
hvitvedhvitved
committed
C#: Fix false-positives in cs/dereferenced-value-may-be-null
Dereferencing an expression of a nullable type should only be reported when the expression is not clearly non-null.
1 parent ce2368d commit d39a336

File tree

7 files changed

+13
-10
lines changed

7 files changed

+13
-10
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: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,7 @@ private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason)
231231
exists(Dereference d | dereferenceAt(_, _, def, d) |
232232
d.hasNullableType() and
233233
not def instanceof Ssa::PseudoDefinition and
234+
not nonNullDef(def) and
234235
reason = def.getSourceVariable().getAssignable() and
235236
msg = "because it has a nullable type"
236237
)

‎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: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -389,13 +389,13 @@ static bool Ex37(E e1, E e2)
389389
int Ex38(int? i)
390390
{
391391
i ??= 0;
392-
return i.Value; // GOOD (false positive)
392+
return i.Value; // GOOD
393393
}
394394

395395
System.Drawing.Color Ex39(System.Drawing.Color? color)
396396
{
397397
color ??= System.Drawing.Color.White;
398-
return color.Value; // GOOD (false positive)
398+
return color.Value; // GOOD
399399
}
400400
}
401401

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

Lines changed: 2 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 |

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

Lines changed: 2 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 |

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -370,10 +370,6 @@ 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:391:9:391:15 | SSA def(i) |
374-
| E.cs:392:16:392:16 | access to parameter i |
375-
| E.cs:397:9:397:44 | SSA def(color) |
376-
| E.cs:398:16:398:20 | access to parameter color |
377373
| Forwarding.cs:7:16:7:23 | SSA def(s) |
378374
| Forwarding.cs:14:9:17:9 | if (...) ... |
379375
| Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -723,8 +719,6 @@ edges
723719
| E.cs:384:9:385:24 | if (...) ... | E.cs:384:27:384:28 | access to parameter e2 |
724720
| E.cs:384:9:385:24 | if (...) ... | E.cs:386:27:386:28 | access to parameter e2 |
725721
| E.cs:384:27:384:28 | access to parameter e2 | E.cs:386:16:386:17 | access to parameter e1 |
726-
| E.cs:391:9:391:15 | SSA def(i) | E.cs:392:16:392:16 | access to parameter i |
727-
| E.cs:397:9:397:44 | SSA def(color) | E.cs:398:16:398:20 | access to parameter color |
728722
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:14:9:17:9 | if (...) ... |
729723
| Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
730724
| Forwarding.cs:19:9:22:9 | if (...) ... | Forwarding.cs:24:9:27:9 | if (...) ... |
@@ -833,8 +827,6 @@ edges
833827
| 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 |
834828
| 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 |
835829
| 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 |
836-
| E.cs:392:16:392:16 | access to parameter i | E.cs:391:9:391:15 | SSA def(i) | E.cs:392:16:392:16 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:389:19:389:19 | i | i | E.cs:389:19:389:19 | i | this |
837-
| E.cs:398:16:398:20 | access to parameter color | E.cs:397:9:397:44 | SSA def(color) | E.cs:398:16:398:20 | access to parameter color | Variable $@ may be null here because it has a nullable type. | E.cs:395:53:395:57 | color | color | E.cs:395:53:395:57 | color | this |
838830
| 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 |
839831
| 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 |
840832
| 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)