Skip to content

Commit cc7c30d

Browse files
hvitvedhvitved
authored
Merge pull request #2179 from calumgrant/cs/local-disposal
C#: Fix a FP in cs/local-not-disposed
2 parents 3eea045 + b9ba534 commit cc7c30d

File tree

9 files changed

+65
-15
lines changed

9 files changed

+65
-15
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,5 +43,6 @@ The following changes in version 1.23 affect C# analysis in all applications.
4343
* There is now a `DataFlow::localExprFlow` predicate and a
4444
`TaintTracking::localExprTaint` predicate to make it easy to use the most
4545
common case of local data flow and taint: from one `Expr` to another.
46+
* Data is now tracked through null-coalescing expressions (`??`).
4647

4748
## Changes to autobuilder

‎csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,10 @@ module LocalFlow {
8686
scope = e2 and
8787
isSuccessor = true
8888
or
89+
e1 = e2.(NullCoalescingExpr).getAnOperand() and
90+
scope = e2 and
91+
isSuccessor = false
92+
or
8993
e1 = e2.(SuppressNullableWarningExpr).getExpr() and
9094
scope = e2 and
9195
isSuccessor = true

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
| dataflow.cs:46:35:46:39 | "t1b" | dataflow.cs:46:18:46:40 | call to method Taint1 |
1313
| dataflow.cs:49:35:49:38 | "t6" | dataflow.cs:49:18:49:45 | call to method TaintIndirect |
1414
| dataflow.cs:49:41:49:44 | "t6" | dataflow.cs:49:18:49:45 | call to method TaintIndirect |
15+
| dataflow.cs:102:30:102:33 | null | dataflow.cs:74:21:74:52 | ... ?? ... |
1516
| dataflow.cs:102:30:102:33 | null | dataflow.cs:89:24:89:51 | ... ? ... : ... |
1617
| dataflow.cs:102:30:102:33 | null | dataflow.cs:108:20:108:33 | call to method IndirectNull |
18+
| dataflow.cs:109:23:109:26 | null | dataflow.cs:74:21:74:52 | ... ?? ... |
1719
| dataflow.cs:109:23:109:26 | null | dataflow.cs:89:24:89:51 | ... ? ... : ... |

‎csharp/ql/test/library-tests/dataflow/local/DataFlow.expected‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,9 @@
88
| LocalDataFlow.cs:412:15:412:20 | access to local variable sink70 |
99
| LocalDataFlow.cs:420:19:420:24 | access to local variable sink71 |
1010
| LocalDataFlow.cs:430:23:430:28 | access to local variable sink72 |
11-
| LocalDataFlow.cs:466:15:466:21 | access to parameter tainted |
11+
| LocalDataFlow.cs:445:15:445:20 | access to local variable sink73 |
12+
| LocalDataFlow.cs:446:15:446:20 | access to local variable sink74 |
13+
| LocalDataFlow.cs:472:15:472:21 | access to parameter tainted |
1214
| SSA.cs:9:15:9:22 | access to local variable ssaSink0 |
1315
| SSA.cs:25:15:25:22 | access to local variable ssaSink1 |
1416
| SSA.cs:43:15:43:22 | access to local variable ssaSink2 |

‎csharp/ql/test/library-tests/dataflow/local/DataFlowStep.expected‎

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,10 @@
545545
| LocalDataFlow.cs:408:15:408:22 | access to local variable nonSink0 | LocalDataFlow.cs:415:31:415:38 | access to local variable nonSink0 |
546546
| LocalDataFlow.cs:411:13:411:34 | SSA def(sink70) | LocalDataFlow.cs:412:15:412:20 | access to local variable sink70 |
547547
| LocalDataFlow.cs:411:22:411:34 | ... = ... | LocalDataFlow.cs:411:13:411:34 | SSA def(sink70) |
548+
| LocalDataFlow.cs:411:22:411:34 | SSA def(sink0) | LocalDataFlow.cs:443:34:443:38 | access to local variable sink0 |
549+
| LocalDataFlow.cs:411:22:411:34 | SSA def(sink0) | LocalDataFlow.cs:444:22:444:26 | access to local variable sink0 |
548550
| LocalDataFlow.cs:411:30:411:34 | access to local variable sink0 | LocalDataFlow.cs:411:22:411:34 | ... = ... |
551+
| LocalDataFlow.cs:411:30:411:34 | access to local variable sink0 | LocalDataFlow.cs:411:22:411:34 | SSA def(sink0) |
549552
| LocalDataFlow.cs:412:15:412:20 | [post] access to local variable sink70 | LocalDataFlow.cs:419:13:419:18 | access to local variable sink70 |
550553
| LocalDataFlow.cs:412:15:412:20 | access to local variable sink70 | LocalDataFlow.cs:419:13:419:18 | access to local variable sink70 |
551554
| LocalDataFlow.cs:415:9:415:38 | SSA def(nonSink0) | LocalDataFlow.cs:416:15:416:22 | access to local variable nonSink0 |
@@ -562,12 +565,23 @@
562565
| LocalDataFlow.cs:427:17:427:22 | access to local variable sink70 | LocalDataFlow.cs:429:18:429:30 | SSA def(sink72) |
563566
| LocalDataFlow.cs:429:18:429:30 | SSA def(sink72) | LocalDataFlow.cs:430:23:430:28 | access to local variable sink72 |
564567
| LocalDataFlow.cs:435:17:435:24 | access to local variable nonSink0 | LocalDataFlow.cs:437:18:437:33 | SSA def(nonSink17) |
568+
| LocalDataFlow.cs:435:17:435:24 | access to local variable nonSink0 | LocalDataFlow.cs:443:22:443:29 | access to local variable nonSink0 |
565569
| LocalDataFlow.cs:437:18:437:33 | SSA def(nonSink17) | LocalDataFlow.cs:438:23:438:31 | access to local variable nonSink17 |
566-
| LocalDataFlow.cs:458:28:458:30 | this | LocalDataFlow.cs:458:41:458:45 | this access |
567-
| LocalDataFlow.cs:458:50:458:52 | this | LocalDataFlow.cs:458:56:458:60 | this access |
568-
| LocalDataFlow.cs:458:50:458:52 | value | LocalDataFlow.cs:458:64:458:68 | access to parameter value |
569-
| LocalDataFlow.cs:464:41:464:47 | tainted | LocalDataFlow.cs:466:15:466:21 | access to parameter tainted |
570-
| LocalDataFlow.cs:469:44:469:53 | nonTainted | LocalDataFlow.cs:471:15:471:24 | access to parameter nonTainted |
570+
| LocalDataFlow.cs:443:13:443:38 | SSA def(sink73) | LocalDataFlow.cs:445:15:445:20 | access to local variable sink73 |
571+
| LocalDataFlow.cs:443:22:443:29 | access to local variable nonSink0 | LocalDataFlow.cs:443:22:443:38 | ... ?? ... |
572+
| LocalDataFlow.cs:443:22:443:29 | access to local variable nonSink0 | LocalDataFlow.cs:444:31:444:38 | access to local variable nonSink0 |
573+
| LocalDataFlow.cs:443:22:443:38 | ... ?? ... | LocalDataFlow.cs:443:13:443:38 | SSA def(sink73) |
574+
| LocalDataFlow.cs:443:34:443:38 | access to local variable sink0 | LocalDataFlow.cs:443:22:443:38 | ... ?? ... |
575+
| LocalDataFlow.cs:443:34:443:38 | access to local variable sink0 | LocalDataFlow.cs:444:22:444:26 | access to local variable sink0 |
576+
| LocalDataFlow.cs:444:13:444:38 | SSA def(sink74) | LocalDataFlow.cs:446:15:446:20 | access to local variable sink74 |
577+
| LocalDataFlow.cs:444:22:444:26 | access to local variable sink0 | LocalDataFlow.cs:444:22:444:38 | ... ?? ... |
578+
| LocalDataFlow.cs:444:22:444:38 | ... ?? ... | LocalDataFlow.cs:444:13:444:38 | SSA def(sink74) |
579+
| LocalDataFlow.cs:444:31:444:38 | access to local variable nonSink0 | LocalDataFlow.cs:444:22:444:38 | ... ?? ... |
580+
| LocalDataFlow.cs:464:28:464:30 | this | LocalDataFlow.cs:464:41:464:45 | this access |
581+
| LocalDataFlow.cs:464:50:464:52 | this | LocalDataFlow.cs:464:56:464:60 | this access |
582+
| LocalDataFlow.cs:464:50:464:52 | value | LocalDataFlow.cs:464:64:464:68 | access to parameter value |
583+
| LocalDataFlow.cs:470:41:470:47 | tainted | LocalDataFlow.cs:472:15:472:21 | access to parameter tainted |
584+
| LocalDataFlow.cs:475:44:475:53 | nonTainted | LocalDataFlow.cs:477:15:477:24 | access to parameter nonTainted |
571585
| SSA.cs:5:17:5:17 | SSA entry def(this.S) | SSA.cs:67:9:67:14 | access to field S |
572586
| SSA.cs:5:17:5:17 | this | SSA.cs:67:9:67:12 | this access |
573587
| SSA.cs:5:26:5:32 | tainted | SSA.cs:8:24:8:30 | access to parameter tainted |

‎csharp/ql/test/library-tests/dataflow/local/LocalDataFlow.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,12 @@ public async void M(bool b)
438438
Check(nonSink17);
439439
break;
440440
}
441+
442+
// Null-coalescing expressions
443+
var sink73 = nonSink0 ?? sink0;
444+
var sink74 = sink0 ?? nonSink0;
445+
Check(sink73);
446+
Check(sink74);
441447
}
442448

443449
static void Check<T>(T x) { }

‎csharp/ql/test/library-tests/dataflow/local/TaintTracking.expected‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,9 @@
6262
| LocalDataFlow.cs:412:15:412:20 | access to local variable sink70 |
6363
| LocalDataFlow.cs:420:19:420:24 | access to local variable sink71 |
6464
| LocalDataFlow.cs:430:23:430:28 | access to local variable sink72 |
65-
| LocalDataFlow.cs:466:15:466:21 | access to parameter tainted |
65+
| LocalDataFlow.cs:445:15:445:20 | access to local variable sink73 |
66+
| LocalDataFlow.cs:446:15:446:20 | access to local variable sink74 |
67+
| LocalDataFlow.cs:472:15:472:21 | access to parameter tainted |
6668
| SSA.cs:9:15:9:22 | access to local variable ssaSink0 |
6769
| SSA.cs:25:15:25:22 | access to local variable ssaSink1 |
6870
| SSA.cs:43:15:43:22 | access to local variable ssaSink2 |

‎csharp/ql/test/library-tests/dataflow/local/TaintTrackingStep.expected‎

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -690,7 +690,10 @@
690690
| LocalDataFlow.cs:408:15:408:22 | access to local variable nonSink0 | LocalDataFlow.cs:415:31:415:38 | access to local variable nonSink0 |
691691
| LocalDataFlow.cs:411:13:411:34 | SSA def(sink70) | LocalDataFlow.cs:412:15:412:20 | access to local variable sink70 |
692692
| LocalDataFlow.cs:411:22:411:34 | ... = ... | LocalDataFlow.cs:411:13:411:34 | SSA def(sink70) |
693+
| LocalDataFlow.cs:411:22:411:34 | SSA def(sink0) | LocalDataFlow.cs:443:34:443:38 | access to local variable sink0 |
694+
| LocalDataFlow.cs:411:22:411:34 | SSA def(sink0) | LocalDataFlow.cs:444:22:444:26 | access to local variable sink0 |
693695
| LocalDataFlow.cs:411:30:411:34 | access to local variable sink0 | LocalDataFlow.cs:411:22:411:34 | ... = ... |
696+
| LocalDataFlow.cs:411:30:411:34 | access to local variable sink0 | LocalDataFlow.cs:411:22:411:34 | SSA def(sink0) |
694697
| LocalDataFlow.cs:412:15:412:20 | [post] access to local variable sink70 | LocalDataFlow.cs:419:13:419:18 | access to local variable sink70 |
695698
| LocalDataFlow.cs:412:15:412:20 | access to local variable sink70 | LocalDataFlow.cs:419:13:419:18 | access to local variable sink70 |
696699
| LocalDataFlow.cs:415:9:415:38 | SSA def(nonSink0) | LocalDataFlow.cs:416:15:416:22 | access to local variable nonSink0 |
@@ -707,15 +710,26 @@
707710
| LocalDataFlow.cs:427:17:427:22 | access to local variable sink70 | LocalDataFlow.cs:429:18:429:30 | SSA def(sink72) |
708711
| LocalDataFlow.cs:429:18:429:30 | SSA def(sink72) | LocalDataFlow.cs:430:23:430:28 | access to local variable sink72 |
709712
| LocalDataFlow.cs:435:17:435:24 | access to local variable nonSink0 | LocalDataFlow.cs:437:18:437:33 | SSA def(nonSink17) |
713+
| LocalDataFlow.cs:435:17:435:24 | access to local variable nonSink0 | LocalDataFlow.cs:443:22:443:29 | access to local variable nonSink0 |
710714
| LocalDataFlow.cs:437:18:437:33 | SSA def(nonSink17) | LocalDataFlow.cs:438:23:438:31 | access to local variable nonSink17 |
711-
| LocalDataFlow.cs:458:28:458:30 | this | LocalDataFlow.cs:458:41:458:45 | this access |
712-
| LocalDataFlow.cs:458:50:458:52 | this | LocalDataFlow.cs:458:56:458:60 | this access |
713-
| LocalDataFlow.cs:458:50:458:52 | value | LocalDataFlow.cs:458:50:458:52 | value |
714-
| LocalDataFlow.cs:458:50:458:52 | value | LocalDataFlow.cs:458:64:458:68 | access to parameter value |
715-
| LocalDataFlow.cs:464:41:464:47 | tainted | LocalDataFlow.cs:464:41:464:47 | tainted |
716-
| LocalDataFlow.cs:464:41:464:47 | tainted | LocalDataFlow.cs:466:15:466:21 | access to parameter tainted |
717-
| LocalDataFlow.cs:469:44:469:53 | nonTainted | LocalDataFlow.cs:469:44:469:53 | nonTainted |
718-
| LocalDataFlow.cs:469:44:469:53 | nonTainted | LocalDataFlow.cs:471:15:471:24 | access to parameter nonTainted |
715+
| LocalDataFlow.cs:443:13:443:38 | SSA def(sink73) | LocalDataFlow.cs:445:15:445:20 | access to local variable sink73 |
716+
| LocalDataFlow.cs:443:22:443:29 | access to local variable nonSink0 | LocalDataFlow.cs:443:22:443:38 | ... ?? ... |
717+
| LocalDataFlow.cs:443:22:443:29 | access to local variable nonSink0 | LocalDataFlow.cs:444:31:444:38 | access to local variable nonSink0 |
718+
| LocalDataFlow.cs:443:22:443:38 | ... ?? ... | LocalDataFlow.cs:443:13:443:38 | SSA def(sink73) |
719+
| LocalDataFlow.cs:443:34:443:38 | access to local variable sink0 | LocalDataFlow.cs:443:22:443:38 | ... ?? ... |
720+
| LocalDataFlow.cs:443:34:443:38 | access to local variable sink0 | LocalDataFlow.cs:444:22:444:26 | access to local variable sink0 |
721+
| LocalDataFlow.cs:444:13:444:38 | SSA def(sink74) | LocalDataFlow.cs:446:15:446:20 | access to local variable sink74 |
722+
| LocalDataFlow.cs:444:22:444:26 | access to local variable sink0 | LocalDataFlow.cs:444:22:444:38 | ... ?? ... |
723+
| LocalDataFlow.cs:444:22:444:38 | ... ?? ... | LocalDataFlow.cs:444:13:444:38 | SSA def(sink74) |
724+
| LocalDataFlow.cs:444:31:444:38 | access to local variable nonSink0 | LocalDataFlow.cs:444:22:444:38 | ... ?? ... |
725+
| LocalDataFlow.cs:464:28:464:30 | this | LocalDataFlow.cs:464:41:464:45 | this access |
726+
| LocalDataFlow.cs:464:50:464:52 | this | LocalDataFlow.cs:464:56:464:60 | this access |
727+
| LocalDataFlow.cs:464:50:464:52 | value | LocalDataFlow.cs:464:50:464:52 | value |
728+
| LocalDataFlow.cs:464:50:464:52 | value | LocalDataFlow.cs:464:64:464:68 | access to parameter value |
729+
| LocalDataFlow.cs:470:41:470:47 | tainted | LocalDataFlow.cs:470:41:470:47 | tainted |
730+
| LocalDataFlow.cs:470:41:470:47 | tainted | LocalDataFlow.cs:472:15:472:21 | access to parameter tainted |
731+
| LocalDataFlow.cs:475:44:475:53 | nonTainted | LocalDataFlow.cs:475:44:475:53 | nonTainted |
732+
| LocalDataFlow.cs:475:44:475:53 | nonTainted | LocalDataFlow.cs:477:15:477:24 | access to parameter nonTainted |
719733
| SSA.cs:5:17:5:17 | SSA entry def(this.S) | SSA.cs:67:9:67:14 | access to field S |
720734
| SSA.cs:5:17:5:17 | this | SSA.cs:67:9:67:12 | this access |
721735
| SSA.cs:5:26:5:32 | tainted | SSA.cs:5:26:5:32 | tainted |

‎csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,11 @@ public IDisposable Method()
8383
// GOOD: Disposed automatically.
8484
using var c2 = new Timer(TimerProc);
8585

86+
// GOOD: ownership taken via ??
87+
StringReader source = null;
88+
using(XmlReader.Create(source ?? new StringReader("xml"), null))
89+
;
90+
8691
return null;
8792
}
8893

0 commit comments

Comments
 (0)