Skip to content

Commit 48dee29

Browse files
calumgrantcalumgrant
authored
Merge pull request github#2021 from hvitved/csharp/local-not-disposed
C#: Refactor `cs/local-not-disposed` using data flow library
2 parents dca39f0 + b66479c commit 48dee29

File tree

5 files changed

+91
-70
lines changed

5 files changed

+91
-70
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ The following changes in version 1.23 affect C# analysis in all applications.
1616
| **Query** | **Expected impact** | **Change** |
1717
|------------------------------|------------------------|-----------------------------------|
1818
| Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | Fewer false positive results | More `null` checks are now taken into account, including `null` checks for `dynamic` expressions and `null` checks such as `object alwaysNull = null; if (x != alwaysNull) ...`. |
19+
| Missing Dispose call on local IDisposable (`cs/local-not-disposed`) | Fewer false positive results | The query has been rewritten in order to identify more dispose patterns. For example, a local `IDisposable` that is disposed of by passing through a fluent API is no longer reported. |
1920

2021
## Removal of old queries
2122

‎csharp/ql/src/API Abuse/Dispose.qll

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -59,22 +59,4 @@ class LocalScopeDisposableCreation extends Call {
5959
)
6060
)
6161
}
62-
63-
/**
64-
* Gets an expression that, if it is disposed of, will imply that the object
65-
* created by this creation is disposed of as well.
66-
*/
67-
Expr getADisposeTarget() { result = getADisposeTarget0().asExpr() }
68-
69-
private DataFlow::Node getADisposeTarget0() {
70-
result = exprNode(this)
71-
or
72-
exists(DataFlow::Node mid | mid = this.getADisposeTarget0() |
73-
localFlowStep(mid, result)
74-
or
75-
result.asExpr() = any(LocalScopeDisposableCreation other |
76-
other.getAnArgument() = mid.asExpr()
77-
)
78-
)
79-
}
8062
}

‎csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql

Lines changed: 79 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -18,60 +18,90 @@ import Dispose
1818
import semmle.code.csharp.frameworks.System
1919
import semmle.code.csharp.commons.Disposal
2020

21-
/** Holds if expression `e` escapes the local method scope. */
22-
predicate escapes(Expr e) {
23-
// Things that return escape
24-
exists(Callable c | c.canReturn(e) or c.canYieldReturn(e))
25-
or
26-
// Things that are assigned to fields, properties, or indexers escape the scope
27-
exists(AssignableDefinition def, Assignable a |
28-
def.getSource() = e and
29-
a = def.getTarget()
30-
|
31-
a instanceof Field or
32-
a instanceof Property or
33-
a instanceof Indexer
34-
)
35-
or
36-
// Things that are added to a collection of some kind are likely to escape the scope
37-
exists(MethodCall mc | mc.getTarget().hasName("Add") | mc.getAnArgument() = e)
21+
private class ReturnNode extends DataFlow::ExprNode {
22+
ReturnNode() {
23+
exists(Callable c, Expr e | e = this.getExpr() | c.canReturn(e) or c.canYieldReturn(e))
24+
}
3825
}
3926

40-
/** Holds if the `disposable` is a whitelisted result. */
41-
predicate isWhitelisted(LocalScopeDisposableCreation disposable) {
42-
exists(MethodCall mc |
43-
// Close can often be used instead of Dispose
44-
mc.getTarget().hasName("Close")
27+
private class Conf extends DataFlow::Configuration {
28+
Conf() { this = "NoDisposeCallOnLocalIDisposable" }
29+
30+
override predicate isSource(DataFlow::Node node) {
31+
node.asExpr() = any(LocalScopeDisposableCreation disposable |
32+
// Only care about library types - user types often have spurious IDisposable declarations
33+
disposable.getType().fromLibrary() and
34+
// WebControls are usually disposed automatically
35+
not disposable.getType() instanceof WebControl
36+
)
37+
}
38+
39+
override predicate isSink(DataFlow::Node node) {
40+
// Things that return may be disposed elsewhere
41+
node instanceof ReturnNode
4542
or
46-
// Used as an alias for Dispose
47-
mc.getTarget().hasName("Clear")
48-
|
49-
mc.getQualifier() = disposable.getADisposeTarget()
43+
exists(Expr e | e = node.asExpr() |
44+
// Disposables constructed in the initializer of a `using` are safe
45+
exists(UsingStmt us | us.getAnExpr() = e)
46+
or
47+
// Foreach calls Dispose
48+
exists(ForeachStmt fs | fs.getIterableExpr() = e)
49+
or
50+
// As are disposables on which the Dispose method is called explicitly
51+
exists(MethodCall mc |
52+
mc.getTarget() instanceof DisposeMethod and
53+
mc.getQualifier() = e
54+
)
55+
or
56+
// A disposing method
57+
exists(Call c, Parameter p | e = c.getArgumentForParameter(p) | mayBeDisposed(p))
58+
or
59+
// Things that are assigned to fields, properties, or indexers may be disposed
60+
exists(AssignableDefinition def, Assignable a |
61+
def.getSource() = e and
62+
a = def.getTarget()
63+
|
64+
a instanceof Field or
65+
a instanceof Property or
66+
a instanceof Indexer
67+
)
68+
or
69+
// Things that are added to a collection of some kind are likely to escape the scope
70+
exists(MethodCall mc | mc.getAnArgument() = e | mc.getTarget().hasName("Add"))
71+
or
72+
exists(MethodCall mc | mc.getQualifier() = e |
73+
// Close can often be used instead of Dispose
74+
mc.getTarget().hasName("Close")
75+
or
76+
// Used as an alias for Dispose
77+
mc.getTarget().hasName("Clear")
78+
)
79+
)
80+
}
81+
82+
override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
83+
node2.asExpr() = any(LocalScopeDisposableCreation other | other.getAnArgument() = node1.asExpr())
84+
}
85+
86+
override predicate isBarrierOut(DataFlow::Node node) {
87+
this.isSink(node) and
88+
not node instanceof ReturnNode
89+
}
90+
}
91+
92+
/** Holds if `disposable` may not be disposed. */
93+
predicate mayNotBeDisposed(LocalScopeDisposableCreation disposable) {
94+
exists(Conf conf, DataFlow::ExprNode e |
95+
e.getExpr() = disposable and
96+
conf.isSource(e) and
97+
not exists(DataFlow::Node sink | conf.hasFlow(DataFlow::exprNode(disposable), sink) |
98+
sink instanceof ReturnNode
99+
implies
100+
sink.getEnclosingCallable() = disposable.getEnclosingCallable()
101+
)
50102
)
51-
or
52-
// WebControls are usually disposed automatically
53-
disposable.getType() instanceof WebControl
54103
}
55104

56105
from LocalScopeDisposableCreation disposable
57-
// The disposable is local scope - the lifetime is the execution of this method
58-
where
59-
not escapes(disposable.getADisposeTarget()) and
60-
// Only care about library types - user types often have spurious IDisposable declarations
61-
disposable.getType().fromLibrary() and
62-
// Disposables constructed in the initializer of a `using` are safe
63-
not exists(UsingStmt us | us.getAnExpr() = disposable.getADisposeTarget()) and
64-
// Foreach calls Dispose
65-
not exists(ForeachStmt fs | fs.getIterableExpr() = disposable.getADisposeTarget()) and
66-
// As are disposables on which the Dispose method is called explicitly
67-
not exists(MethodCall mc |
68-
mc.getTarget() instanceof DisposeMethod and
69-
mc.getQualifier() = disposable.getADisposeTarget()
70-
) and
71-
// Ignore whitelisted results
72-
not isWhitelisted(disposable) and
73-
// Not passed to a disposing method
74-
not exists(Call c, Parameter p | disposable.getADisposeTarget() = c.getArgumentForParameter(p) |
75-
mayBeDisposed(p)
76-
)
106+
where mayNotBeDisposed(disposable)
77107
select disposable, "Disposable '" + disposable.getType() + "' is created here but is not disposed."

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// semmle-extractor-options: --cil /r:System.Xml.dll /r:System.Xml.ReaderWriter.dll /r:System.Private.Xml.dll /r:System.ComponentModel.Primitives.dll /r:System.IO.Compression.dll /r:System.Runtime.Extensions.dll
1+
// semmle-extractor-options: --cil /langversion:8.0 /r:System.Xml.dll /r:System.Xml.ReaderWriter.dll /r:System.Private.Xml.dll /r:System.ComponentModel.Primitives.dll /r:System.IO.Compression.dll /r:System.Runtime.Extensions.dll
22

33
using System;
44
using System.Text;
@@ -51,6 +51,7 @@ public IDisposable Method()
5151
// BAD: No Dispose call
5252
var c1d = new Timer(TimerProc);
5353
var fs = new FileStream("", FileMode.CreateNew, FileAccess.Write);
54+
new FileStream("", FileMode.CreateNew, FileAccess.Write).Fluent();
5455

5556
// GOOD: Disposed via wrapper
5657
fs = new FileStream("", FileMode.CreateNew, FileAccess.Write);
@@ -65,6 +66,7 @@ public IDisposable Method()
6566
d = new GZipStream(fs, CompressionMode.Compress);
6667
dProp = new Timer(TimerProc);
6768
this[0] = new Timer(TimerProc);
69+
d = new FileStream("", FileMode.CreateNew, FileAccess.Write).Fluent();
6870

6971
// GOOD: Passed to another IDisposable
7072
using (var reader = new StreamReader(new FileStream("", FileMode.Open)))
@@ -90,6 +92,11 @@ public IDisposable Method()
9092
void TimerProc(object obj)
9193
{
9294
}
95+
96+
public void Dispose() { }
9397
}
9498

95-
// semmle-extractor-options: /langversion:8.0
99+
static class Extensions
100+
{
101+
public static FileStream Fluent(this FileStream fs) => fs;
102+
}

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
| NoDisposeCallOnLocalIDisposable.cs:52:19:52:38 | object creation of type Timer | Disposable 'Timer' is created here but is not disposed. |
22
| NoDisposeCallOnLocalIDisposable.cs:53:18:53:73 | object creation of type FileStream | Disposable 'FileStream' is created here but is not disposed. |
3-
| NoDisposeCallOnLocalIDisposable.cs:74:25:74:71 | call to method Create | Disposable 'XmlReader' is created here but is not disposed. |
3+
| NoDisposeCallOnLocalIDisposable.cs:54:9:54:64 | object creation of type FileStream | Disposable 'FileStream' is created here but is not disposed. |
4+
| NoDisposeCallOnLocalIDisposable.cs:76:25:76:71 | call to method Create | Disposable 'XmlReader' is created here but is not disposed. |
45
| NoDisposeCallOnLocalIDisposableBad.cs:8:22:8:56 | object creation of type FileStream | Disposable 'FileStream' is created here but is not disposed. |

0 commit comments

Comments
 (0)