Skip to content

Commit f1749b3

Browse files
jf205jf205
authored
Merge pull request #2654 from calumgrant/cs/null-dereference
C#: Improvements to cs/dereferenced-value-may-be-null
2 parents 007b079 + a868456 commit f1749b3

File tree

5 files changed

+35
-14
lines changed

5 files changed

+35
-14
lines changed

‎change-notes/1.24/analysis-csharp.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
1414
| **Query** | **Expected impact** | **Change** |
1515
|------------------------------|------------------------|-----------------------------------|
1616
| Useless assignment to local variable (`cs/useless-assignment-to-local`) | Fewer false positive results | Results have been removed when the variable is named `_` in a `foreach` statement. |
17+
| Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | More results | Results are reported from parameters with a default value of `null`. |
1718

1819
## Removal of old queries
1920

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,17 @@ private predicate isMaybeNullArgument(Ssa::ExplicitDefinition def, MaybeNullExpr
176176
pdef = def.getADefinition()
177177
|
178178
p = pdef.getParameter().getSourceDeclaration() and
179-
p.getAnAssignedArgument() = arg and
179+
arg = p.getAnAssignedArgument() and
180+
not arg.getEnclosingCallable().getEnclosingCallable*() instanceof TestMethod
181+
)
182+
}
183+
184+
private predicate isNullDefaultArgument(Ssa::ExplicitDefinition def, AlwaysNullExpr arg) {
185+
exists(AssignableDefinitions::ImplicitParameterDefinition pdef, Parameter p |
186+
pdef = def.getADefinition()
187+
|
188+
p = pdef.getParameter().getSourceDeclaration() and
189+
arg = p.getDefaultValue() and
180190
not arg.getEnclosingCallable().getEnclosingCallable*() instanceof TestMethod
181191
)
182192
}
@@ -204,6 +214,8 @@ private predicate defMaybeNull(Ssa::Definition def, string msg, Element reason)
204214
else msg = "because of $@ potential null argument"
205215
)
206216
or
217+
isNullDefaultArgument(def, reason) and msg = "because the parameter has a null default value"
218+
or
207219
// If the source of a variable is `null` then the variable may be `null`
208220
exists(AssignableDefinition adef | adef = def.(Ssa::ExplicitDefinition).getADefinition() |
209221
adef.getSource() instanceof MaybeNullExpr and

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,15 +198,15 @@ public void Ex13(bool b)
198198
var o = b ? null : "";
199199
o.M1(); // GOOD
200200
if (b)
201-
o.M2(); // BAD (maybe)
201+
o.M2(); // BAD (maybe)
202202
else
203-
o.Select(x => x); // BAD (maybe)
203+
o.Select(x => x); // BAD (maybe)
204204
}
205205

206206
public int Ex14(string s)
207207
{
208208
if (s is string)
209-
return s.Length;
209+
return s.Length;
210210
return s.GetHashCode(); // BAD (always)
211211
}
212212

@@ -362,6 +362,10 @@ static void Ex33(string s, object o)
362362
if (x != (string)null)
363363
x.ToString(); // GOOD
364364
}
365+
366+
static int Ex34(string s = null) => s.Length; // BAD (maybe)
367+
368+
static int Ex35(string s = "null") => s.Length; // GOOD
365369
}
366370

367371
public static class Extensions

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1130,10 +1130,10 @@
11301130
| E.cs:199:9:199:9 | access to local variable o | non-empty | E.cs:198:17:198:29 | ... ? ... : ... | non-empty |
11311131
| E.cs:199:9:199:9 | access to local variable o | non-null | E.cs:198:17:198:29 | ... ? ... : ... | non-null |
11321132
| E.cs:199:9:199:9 | access to local variable o | null | E.cs:198:17:198:29 | ... ? ... : ... | null |
1133-
| E.cs:201:11:201:11 | access to local variable o | non-null | E.cs:198:17:198:29 | ... ? ... : ... | non-null |
1134-
| E.cs:201:11:201:11 | access to local variable o | null | E.cs:198:17:198:29 | ... ? ... : ... | null |
1135-
| E.cs:203:11:203:11 | access to local variable o | non-null | E.cs:198:17:198:29 | ... ? ... : ... | non-null |
1136-
| E.cs:203:11:203:11 | access to local variable o | null | E.cs:198:17:198:29 | ... ? ... : ... | null |
1133+
| E.cs:201:13:201:13 | access to local variable o | non-null | E.cs:198:17:198:29 | ... ? ... : ... | non-null |
1134+
| E.cs:201:13:201:13 | access to local variable o | null | E.cs:198:17:198:29 | ... ? ... : ... | null |
1135+
| E.cs:203:13:203:13 | access to local variable o | non-null | E.cs:198:17:198:29 | ... ? ... : ... | non-null |
1136+
| E.cs:203:13:203:13 | access to local variable o | null | E.cs:198:17:198:29 | ... ? ... : ... | null |
11371137
| E.cs:208:13:208:23 | ... is ... | false | E.cs:208:13:208:13 | access to parameter s | null |
11381138
| E.cs:208:13:208:23 | ... is ... | true | E.cs:208:13:208:13 | access to parameter s | non-null |
11391139
| E.cs:220:13:220:13 | access to local variable x | non-null | E.cs:217:17:217:20 | null | non-null |

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

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -321,8 +321,8 @@ nodes
321321
| E.cs:192:17:192:17 | access to parameter o |
322322
| E.cs:198:13:198:29 | [b (line 196): false] SSA def(o) |
323323
| E.cs:198:13:198:29 | [b (line 196): true] SSA def(o) |
324-
| E.cs:201:11:201:11 | access to local variable o |
325-
| E.cs:203:11:203:11 | access to local variable o |
324+
| E.cs:201:13:201:13 | access to local variable o |
325+
| E.cs:203:13:203:13 | access to local variable o |
326326
| E.cs:206:28:206:28 | SSA param(s) |
327327
| E.cs:210:16:210:16 | access to parameter s |
328328
| E.cs:217:13:217:20 | [b (line 213): true] SSA def(x) |
@@ -351,6 +351,8 @@ nodes
351351
| E.cs:343:9:343:9 | access to local variable x |
352352
| E.cs:348:17:348:36 | SSA def(x) |
353353
| E.cs:349:9:349:9 | access to local variable x |
354+
| E.cs:366:28:366:28 | SSA param(s) |
355+
| E.cs:366:41:366:41 | access to parameter s |
354356
| Forwarding.cs:7:16:7:23 | SSA def(s) |
355357
| Forwarding.cs:14:9:17:9 | if (...) ... |
356358
| Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -664,8 +666,8 @@ edges
664666
| E.cs:181:9:183:9 | {...} | E.cs:184:9:187:9 | if (...) ... |
665667
| E.cs:184:9:187:9 | if (...) ... | E.cs:186:13:186:15 | access to parameter obj |
666668
| E.cs:190:29:190:29 | SSA param(o) | E.cs:192:17:192:17 | access to parameter o |
667-
| E.cs:198:13:198:29 | [b (line 196): false] SSA def(o) | E.cs:203:11:203:11 | access to local variable o |
668-
| E.cs:198:13:198:29 | [b (line 196): true] SSA def(o) | E.cs:201:11:201:11 | access to local variable o |
669+
| E.cs:198:13:198:29 | [b (line 196): false] SSA def(o) | E.cs:203:13:203:13 | access to local variable o |
670+
| E.cs:198:13:198:29 | [b (line 196): true] SSA def(o) | E.cs:201:13:201:13 | access to local variable o |
669671
| E.cs:206:28:206:28 | SSA param(s) | E.cs:210:16:210:16 | access to parameter s |
670672
| E.cs:217:13:217:20 | [b (line 213): true] SSA def(x) | E.cs:218:9:218:9 | access to local variable x |
671673
| E.cs:217:13:217:20 | [b (line 213): true] SSA def(x) | E.cs:220:13:220:13 | access to local variable x |
@@ -682,6 +684,7 @@ edges
682684
| E.cs:330:13:330:36 | SSA def(x) | E.cs:331:9:331:9 | access to local variable x |
683685
| E.cs:342:13:342:32 | SSA def(x) | E.cs:343:9:343:9 | access to local variable x |
684686
| E.cs:348:17:348:36 | SSA def(x) | E.cs:349:9:349:9 | access to local variable x |
687+
| E.cs:366:28:366:28 | SSA param(s) | E.cs:366:41:366:41 | access to parameter s |
685688
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:14:9:17:9 | if (...) ... |
686689
| Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
687690
| Forwarding.cs:19:9:22:9 | if (...) ... | Forwarding.cs:24:9:27:9 | if (...) ... |
@@ -771,8 +774,8 @@ edges
771774
| E.cs:186:13:186:15 | access to parameter obj | E.cs:173:29:173:31 | SSA param(obj) | E.cs:186:13:186:15 | access to parameter obj | Variable $@ may be null here as suggested by $@ null check. | E.cs:173:29:173:31 | obj | obj | E.cs:175:19:175:29 | ... == ... | this |
772775
| E.cs:186:13:186:15 | access to parameter obj | E.cs:173:29:173:31 | SSA param(obj) | E.cs:186:13:186:15 | access to parameter obj | Variable $@ may be null here as suggested by $@ null check. | E.cs:173:29:173:31 | obj | obj | E.cs:180:13:180:23 | ... == ... | this |
773776
| E.cs:192:17:192:17 | access to parameter o | E.cs:190:29:190:29 | SSA param(o) | E.cs:192:17:192:17 | access to parameter o | Variable $@ may be null here as suggested by $@ null check. | E.cs:190:29:190:29 | o | o | E.cs:193:17:193:17 | access to parameter o | this |
774-
| E.cs:201:11:201:11 | access to local variable o | E.cs:198:13:198:29 | [b (line 196): true] SSA def(o) | E.cs:201:11:201:11 | access to local variable o | Variable $@ may be null here because of $@ assignment. | E.cs:198:13:198:13 | o | o | E.cs:198:13:198:29 | String o = ... | this |
775-
| E.cs:203:11:203:11 | access to local variable o | E.cs:198:13:198:29 | [b (line 196): false] SSA def(o) | E.cs:203:11:203:11 | access to local variable o | Variable $@ may be null here because of $@ assignment. | E.cs:198:13:198:13 | o | o | E.cs:198:13:198:29 | String o = ... | this |
777+
| E.cs:201:13:201:13 | access to local variable o | E.cs:198:13:198:29 | [b (line 196): true] SSA def(o) | E.cs:201:13:201:13 | access to local variable o | Variable $@ may be null here because of $@ assignment. | E.cs:198:13:198:13 | o | o | E.cs:198:13:198:29 | String o = ... | this |
778+
| E.cs:203:13:203:13 | access to local variable o | E.cs:198:13:198:29 | [b (line 196): false] SSA def(o) | E.cs:203:13:203:13 | access to local variable o | Variable $@ may be null here because of $@ assignment. | E.cs:198:13:198:13 | o | o | E.cs:198:13:198:29 | String o = ... | this |
776779
| E.cs:218:9:218:9 | access to local variable x | E.cs:217:13:217:20 | [b (line 213): true] SSA def(x) | E.cs:218:9:218:9 | access to local variable x | Variable $@ may be null here because of $@ assignment. | E.cs:215:13:215:13 | x | x | E.cs:217:13:217:20 | ... = ... | this |
777780
| E.cs:230:9:230:9 | access to local variable x | E.cs:227:13:227:20 | [b (line 223): true] SSA def(x) | E.cs:230:9:230:9 | access to local variable x | Variable $@ may be null here because of $@ assignment. | E.cs:225:13:225:13 | x | x | E.cs:227:13:227:20 | ... = ... | this |
778781
| E.cs:235:16:235:16 | access to parameter i | E.cs:233:26:233:26 | SSA param(i) | E.cs:235:16:235:16 | access to parameter i | Variable $@ may be null here because it has a nullable type. | E.cs:233:26:233:26 | i | i | E.cs:233:26:233:26 | i | this |
@@ -782,6 +785,7 @@ edges
782785
| E.cs:302:9:302:9 | access to local variable s | E.cs:301:13:301:27 | SSA def(s) | E.cs:302:9:302:9 | access to local variable s | Variable $@ may be null here because of $@ assignment. | E.cs:301:13:301:13 | s | s | E.cs:301:13:301:27 | String s = ... | this |
783786
| E.cs:343:9:343:9 | access to local variable x | E.cs:342:13:342:32 | SSA def(x) | E.cs:343:9:343:9 | access to local variable x | Variable $@ may be null here because of $@ assignment. | E.cs:342:13:342:13 | x | x | E.cs:342:13:342:32 | String x = ... | this |
784787
| E.cs:349:9:349:9 | access to local variable x | E.cs:348:17:348:36 | SSA def(x) | E.cs:349:9:349:9 | access to local variable x | Variable $@ may be null here because of $@ assignment. | E.cs:348:17:348:17 | x | x | E.cs:348:17:348:36 | dynamic x = ... | this |
788+
| E.cs:366:41:366:41 | access to parameter s | E.cs:366:28:366:28 | SSA param(s) | E.cs:366:41:366:41 | access to parameter s | Variable $@ may be null here because the parameter has a null default value. | E.cs:366:28:366:28 | s | s | E.cs:366:32:366:35 | null | this |
785789
| 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 |
786790
| 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 |
787791
| 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)