C#: Refactor cs/local-not-disposed using data flow library

Author: hvitved

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()
-        )
-    )
-  }
 }

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

@@ -18,60 +18,91 @@ 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 mayNotBeDiposed(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 mayNotBeDiposed(disposable)
 select disposable, "Disposable '" + disposable.getType() + "' is created here but is not disposed."

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

@@ -66,7 +66,7 @@ public IDisposable Method()
         d = new GZipStream(fs, CompressionMode.Compress);
         dProp = new Timer(TimerProc);
         this[0] = new Timer(TimerProc);
-        d = new FileStream("", FileMode.CreateNew, FileAccess.Write).Fluent(); // FALSE POSITIVE
+        d = new FileStream("", FileMode.CreateNew, FileAccess.Write).Fluent();
 
         // GOOD: Passed to another IDisposable
         using (var reader = new StreamReader(new FileStream("", FileMode.Open)))

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

@@ -1,6 +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:54:9:54:64 | object creation of type FileStream | Disposable 'FileStream' is created here but is not disposed. |
-| NoDisposeCallOnLocalIDisposable.cs:69:13:69:68 | 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. |