Skip to content

Commit 91637d4

Browse files
committed
Fix null dereference false positive
1 parent a354ca3 commit 91637d4

File tree

2 files changed

+42
-5
lines changed

2 files changed

+42
-5
lines changed

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

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,48 @@ private predicate isMaybeNullArgument(Ssa::ExplicitDefinition def, MaybeNullExpr
163163
|
164164
p = pdef.getParameter().getUnboundDeclaration() and
165165
arg = p.getAnAssignedArgument() and
166-
not arg.getEnclosingCallable().getEnclosingCallable*() instanceof TestMethod
166+
not arg.getEnclosingCallable().getEnclosingCallable*() instanceof TestMethod and
167+
(
168+
p.isParams()
169+
implies
170+
(
171+
isValidExplicitParamsType(p, arg.getType()) and
172+
not exists(Call c | c.getAnArgument() = arg and hasMultipleParamsArguments(c))
173+
)
174+
)
175+
)
176+
}
177+
178+
/**
179+
* Holds if the type `t` is a valid argument type for passing an explicit array
180+
* to the `params` parameter `p`. For example, the types `object[]` and `string[]`
181+
* of the arguments on lines 4 and 5, respectively, are valid for the parameter
182+
* `args` on line 1 in
183+
*
184+
* ```csharp
185+
* void M(params object[] args) { ... }
186+
*
187+
* void CallM(object[] os, string[] ss, string s) {
188+
* M(os);
189+
* M(ss);
190+
* M(s);
191+
* }
192+
* ```
193+
*/
194+
pragma[nomagic]
195+
private predicate isValidExplicitParamsType(Parameter p, Type t) {
196+
p.isParams() and
197+
t.isImplicitlyConvertibleTo(p.getType())
198+
}
199+
200+
/**
201+
* Holds if call `c` has multiple arguments for a `params` parameter
202+
* of the targeted callable.
203+
*/
204+
private predicate hasMultipleParamsArguments(Call c) {
205+
exists(Parameter p | p = c.getTarget().getAParameter() |
206+
p.isParams() and
207+
exists(c.getArgument(any(int i | i > p.getPosition())))
167208
)
168209
}
169210

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

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -443,9 +443,7 @@ nodes
443443
| NullAlwaysBad.cs:9:30:9:30 | access to parameter s |
444444
| NullMaybeBad.cs:7:27:7:27 | access to parameter o |
445445
| NullMaybeBad.cs:13:17:13:20 | null |
446-
| Params.cs:9:17:9:20 | access to parameter args |
447446
| Params.cs:14:17:14:20 | access to parameter args |
448-
| Params.cs:19:27:19:30 | null |
449447
| Params.cs:20:12:20:15 | null |
450448
| StringConcatenation.cs:14:16:14:23 | SSA def(s) |
451449
| StringConcatenation.cs:15:16:15:16 | access to local variable s |
@@ -835,7 +833,6 @@ edges
835833
| GuardedString.cs:34:26:34:26 | 0 | GuardedString.cs:35:31:35:31 | access to local variable s |
836834
| NullAlwaysBad.cs:7:29:7:29 | SSA param(s) | NullAlwaysBad.cs:9:30:9:30 | access to parameter s |
837835
| NullMaybeBad.cs:13:17:13:20 | null | NullMaybeBad.cs:7:27:7:27 | access to parameter o |
838-
| Params.cs:19:27:19:30 | null | Params.cs:9:17:9:20 | access to parameter args |
839836
| Params.cs:20:12:20:15 | null | Params.cs:14:17:14:20 | access to parameter args |
840837
| StringConcatenation.cs:14:16:14:23 | SSA def(s) | StringConcatenation.cs:15:16:15:16 | access to local variable s |
841838
| StringConcatenation.cs:15:16:15:16 | access to local variable s | StringConcatenation.cs:16:17:16:17 | access to local variable s |
@@ -924,6 +921,5 @@ edges
924921
| 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 at this access because it has a nullable type. | E.cs:415:27:415:27 | i | i | E.cs:415:27:415:27 | i | this |
925922
| 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 at this access because of $@ assignment. | GuardedString.cs:7:16:7:16 | s | s | GuardedString.cs:7:16:7:32 | String s = ... | this |
926923
| 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 at this access because of $@ null argument. | NullMaybeBad.cs:5:25:5:25 | o | o | NullMaybeBad.cs:13:17:13:20 | null | this |
927-
| Params.cs:9:17:9:20 | access to parameter args | Params.cs:19:27:19:30 | null | Params.cs:9:17:9:20 | access to parameter args | Variable $@ may be null at this access because of $@ null argument. | Params.cs:7:36:7:39 | args | args | Params.cs:19:27:19:30 | null | this |
928924
| Params.cs:14:17:14:20 | access to parameter args | Params.cs:20:12:20:15 | null | Params.cs:14:17:14:20 | access to parameter args | Variable $@ may be null at this access because of $@ null argument. | Params.cs:12:36:12:39 | args | args | Params.cs:20:12:20:15 | null | this |
929925
| 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 at this access 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)