diff --git a/change-notes/1.23/analysis-csharp.md b/change-notes/1.23/analysis-csharp.md index 243a1efec8a..90d4bf06d37 100644 --- a/change-notes/1.23/analysis-csharp.md +++ b/change-notes/1.23/analysis-csharp.md @@ -16,6 +16,7 @@ The following changes in version 1.23 affect C# analysis in all applications. | **Query** | **Expected impact** | **Change** | |------------------------------|------------------------|-----------------------------------| | 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) ...`. | +| 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. | ## Removal of old queries diff --git a/csharp/ql/src/API Abuse/Dispose.qll b/csharp/ql/src/API Abuse/Dispose.qll index 657ecbd8033..e8036ad36cd 100644 --- a/csharp/ql/src/API Abuse/Dispose.qll +++ b/csharp/ql/src/API Abuse/Dispose.qll @@ -59,22 +59,4 @@ class LocalScopeDisposableCreation extends Call { ) ) } - - /** - * Gets an expression that, if it is disposed of, will imply that the object - * created by this creation is disposed of as well. - */ - Expr getADisposeTarget() { result = getADisposeTarget0().asExpr() } - - private DataFlow::Node getADisposeTarget0() { - result = exprNode(this) - or - exists(DataFlow::Node mid | mid = this.getADisposeTarget0() | - localFlowStep(mid, result) - or - result.asExpr() = any(LocalScopeDisposableCreation other | - other.getAnArgument() = mid.asExpr() - ) - ) - } } diff --git a/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql b/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql index 27849480052..646ab04ac1f 100644 --- a/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql +++ b/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql @@ -18,60 +18,90 @@ import Dispose import semmle.code.csharp.frameworks.System import semmle.code.csharp.commons.Disposal -/** Holds if expression `e` escapes the local method scope. */ -predicate escapes(Expr e) { - // Things that return escape - exists(Callable c | c.canReturn(e) or c.canYieldReturn(e)) - or - // Things that are assigned to fields, properties, or indexers escape the scope - exists(AssignableDefinition def, Assignable a | - def.getSource() = e and - a = def.getTarget() - | - a instanceof Field or - a instanceof Property or - a instanceof Indexer - ) - or - // Things that are added to a collection of some kind are likely to escape the scope - exists(MethodCall mc | mc.getTarget().hasName("Add") | mc.getAnArgument() = e) +private class ReturnNode extends DataFlow::ExprNode { + ReturnNode() { + exists(Callable c, Expr e | e = this.getExpr() | c.canReturn(e) or c.canYieldReturn(e)) + } } -/** Holds if the `disposable` is a whitelisted result. */ -predicate isWhitelisted(LocalScopeDisposableCreation disposable) { - exists(MethodCall mc | - // Close can often be used instead of Dispose - mc.getTarget().hasName("Close") +private class Conf extends DataFlow::Configuration { + Conf() { this = "NoDisposeCallOnLocalIDisposable" } + + override predicate isSource(DataFlow::Node node) { + node.asExpr() = any(LocalScopeDisposableCreation disposable | + // Only care about library types - user types often have spurious IDisposable declarations + disposable.getType().fromLibrary() and + // WebControls are usually disposed automatically + not disposable.getType() instanceof WebControl + ) + } + + override predicate isSink(DataFlow::Node node) { + // Things that return may be disposed elsewhere + node instanceof ReturnNode or - // Used as an alias for Dispose - mc.getTarget().hasName("Clear") - | - mc.getQualifier() = disposable.getADisposeTarget() + exists(Expr e | e = node.asExpr() | + // Disposables constructed in the initializer of a `using` are safe + exists(UsingStmt us | us.getAnExpr() = e) + or + // Foreach calls Dispose + exists(ForeachStmt fs | fs.getIterableExpr() = e) + or + // As are disposables on which the Dispose method is called explicitly + exists(MethodCall mc | + mc.getTarget() instanceof DisposeMethod and + mc.getQualifier() = e + ) + or + // A disposing method + exists(Call c, Parameter p | e = c.getArgumentForParameter(p) | mayBeDisposed(p)) + or + // Things that are assigned to fields, properties, or indexers may be disposed + exists(AssignableDefinition def, Assignable a | + def.getSource() = e and + a = def.getTarget() + | + a instanceof Field or + a instanceof Property or + a instanceof Indexer + ) + or + // Things that are added to a collection of some kind are likely to escape the scope + exists(MethodCall mc | mc.getAnArgument() = e | mc.getTarget().hasName("Add")) + or + exists(MethodCall mc | mc.getQualifier() = e | + // Close can often be used instead of Dispose + mc.getTarget().hasName("Close") + or + // Used as an alias for Dispose + mc.getTarget().hasName("Clear") + ) + ) + } + + override predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) { + node2.asExpr() = any(LocalScopeDisposableCreation other | other.getAnArgument() = node1.asExpr()) + } + + override predicate isBarrierOut(DataFlow::Node node) { + this.isSink(node) and + not node instanceof ReturnNode + } +} + +/** Holds if `disposable` may not be disposed. */ +predicate mayNotBeDisposed(LocalScopeDisposableCreation disposable) { + exists(Conf conf, DataFlow::ExprNode e | + e.getExpr() = disposable and + conf.isSource(e) and + not exists(DataFlow::Node sink | conf.hasFlow(DataFlow::exprNode(disposable), sink) | + sink instanceof ReturnNode + implies + sink.getEnclosingCallable() = disposable.getEnclosingCallable() + ) ) - or - // WebControls are usually disposed automatically - disposable.getType() instanceof WebControl } from LocalScopeDisposableCreation disposable -// The disposable is local scope - the lifetime is the execution of this method -where - not escapes(disposable.getADisposeTarget()) and - // Only care about library types - user types often have spurious IDisposable declarations - disposable.getType().fromLibrary() and - // Disposables constructed in the initializer of a `using` are safe - not exists(UsingStmt us | us.getAnExpr() = disposable.getADisposeTarget()) and - // Foreach calls Dispose - not exists(ForeachStmt fs | fs.getIterableExpr() = disposable.getADisposeTarget()) and - // As are disposables on which the Dispose method is called explicitly - not exists(MethodCall mc | - mc.getTarget() instanceof DisposeMethod and - mc.getQualifier() = disposable.getADisposeTarget() - ) and - // Ignore whitelisted results - not isWhitelisted(disposable) and - // Not passed to a disposing method - not exists(Call c, Parameter p | disposable.getADisposeTarget() = c.getArgumentForParameter(p) | - mayBeDisposed(p) - ) +where mayNotBeDisposed(disposable) select disposable, "Disposable '" + disposable.getType() + "' is created here but is not disposed." diff --git a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs index aa325c33bdc..45caec0022d 100644 --- a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs +++ b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs @@ -1,4 +1,4 @@ -// 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 +// 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 using System; using System.Text; @@ -51,6 +51,7 @@ class Test // BAD: No Dispose call var c1d = new Timer(TimerProc); var fs = new FileStream("", FileMode.CreateNew, FileAccess.Write); + new FileStream("", FileMode.CreateNew, FileAccess.Write).Fluent(); // GOOD: Disposed via wrapper fs = new FileStream("", FileMode.CreateNew, FileAccess.Write); @@ -65,6 +66,7 @@ class Test d = new GZipStream(fs, CompressionMode.Compress); dProp = new Timer(TimerProc); this[0] = new Timer(TimerProc); + d = new FileStream("", FileMode.CreateNew, FileAccess.Write).Fluent(); // GOOD: Passed to another IDisposable using (var reader = new StreamReader(new FileStream("", FileMode.Open))) @@ -90,6 +92,11 @@ class Test void TimerProc(object obj) { } + + public void Dispose() { } } -// semmle-extractor-options: /langversion:8.0 +static class Extensions +{ + public static FileStream Fluent(this FileStream fs) => fs; +} diff --git a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected index 2b9899736cc..aa70ad5a1cd 100644 --- a/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected +++ b/csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.expected @@ -1,4 +1,5 @@ | NoDisposeCallOnLocalIDisposable.cs:52:19:52:38 | object creation of type Timer | Disposable 'Timer' is created here but is not disposed. | | NoDisposeCallOnLocalIDisposable.cs:53:18:53:73 | object creation of type FileStream | Disposable 'FileStream' is created here but is not disposed. | -| NoDisposeCallOnLocalIDisposable.cs:74:25:74:71 | call to method Create | Disposable 'XmlReader' is created here but is not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:54:9:54:64 | object creation of type FileStream | Disposable 'FileStream' is created here but is not disposed. | +| NoDisposeCallOnLocalIDisposable.cs:76:25:76:71 | call to method Create | Disposable 'XmlReader' is created here but is not disposed. | | NoDisposeCallOnLocalIDisposableBad.cs:8:22:8:56 | object creation of type FileStream | Disposable 'FileStream' is created here but is not disposed. |