Simplify handling of wrapper classes and exception flow + improve qldoc and annotate tests.

python/ql/src/Resources/FileNotAlwaysClosed.qhelp

@@ -4,13 +4,16 @@
 <qhelp>
 
 <overview>
-<p>When a file is opened, it should always be closed. Failure to close files could result in loss of data or resource leaks.</p>
-
+<p>When a file is opened, it should always be closed. 
+</p>
+<p>A file opened for writing that is not closed when the application exits may result in data loss, where not all of the data written may be saved to the file.
+A file opened for reading or writing that is not closed may also use up file descriptors, which is a resource leak that in long running applications could lead to a failure to open additional files. 
+</p>
 </overview>
 <recommendation>
 
 <p>Ensure that opened files are always closed, including when an exception could be raised.
-The best practice is to use a <code>with</code> statement to automatically clean up resources.
+The best practice is often to use a <code>with</code> statement to automatically clean up resources.
 Otherwise, ensure that <code>.close()</code> is called in a <code>try...except</code> or <code>try...finally</code> 
 block to handle any possible exceptions.
 </p>

python/ql/src/Resources/FileNotAlwaysClosed.ql

@@ -22,4 +22,4 @@ where
   or
   fileMayNotBeClosedOnException(fo, _) and
   msg = "File may not be closed if an exception is raised."
-select fo.getLocalSource(), msg
+select fo, msg

python/ql/src/Resources/FileNotAlwaysClosedQuery.qll

@@ -24,17 +24,10 @@ private DataFlow::TypeTrackingNode fileOpenInstance(DataFlow::TypeTracker t) {
 /** A call that returns an instance of an open file object. */
 class FileOpen extends DataFlow::CallCfgNode {
   FileOpen() { fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(this) }
-
-  /** Gets the local source of this file object, through any wrapper calls. */
-  FileOpen getLocalSource() {
-    if this instanceof FileWrapperCall
-    then result = this.(FileWrapperCall).getWrapped().getLocalSource()
-    else result = this
-  }
 }
 
 /** A call that may wrap a file object in a wrapper class or `os.fdopen`. */
-class FileWrapperCall extends FileOpenSource, DataFlow::CallCfgNode {
+class FileWrapperCall extends DataFlow::CallCfgNode {
   FileOpen wrapped;
 
   FileWrapperCall() {
@@ -53,14 +46,11 @@ class FileWrapperCall extends FileOpenSource, DataFlow::CallCfgNode {
 abstract class FileClose extends DataFlow::CfgNode {
   /** Holds if this file close will occur if an exception is thrown at `e`. */
   predicate guardsExceptions(Expr e) {
-    exists(Try try |
-      e = try.getAStmt().getAChildNode*() and
-      (
-        this.asExpr() = try.getAHandler().getAChildNode*()
-        or
-        this.asExpr() = try.getAFinalstmt().getAChildNode*()
-      )
-    )
+    this.asCfgNode() =
+      DataFlow::exprNode(e).asCfgNode().getAnExceptionalSuccessor().getASuccessor*()
+    or
+    // the expression is after the close call
+    DataFlow::exprNode(e).asCfgNode() = this.asCfgNode().getASuccessor*()
   }
 }
 
@@ -95,8 +85,20 @@ private predicate mayRaiseWithFile(DataFlow::CfgNode node) {
   not node instanceof FileClose
 }
 
+/** Holds if data flows from `nodeFrom` to `nodeTo` in one step that also includes file wrapper classes. */
+private predicate fileLocalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+  DataFlow::localFlowStep(nodeFrom, nodeTo)
+  or
+  exists(FileWrapperCall fw | nodeFrom = fw.getWrapped() and nodeTo = fw)
+}
+
+/** Holds if data flows from `source` to `sink`, including file wrapper classes. */
+private predicate fileLocalFlow(DataFlow::Node source, DataFlow::Node sink) {
+  fileLocalFlowStep*(source, sink)
+}
+
 /** Holds if the file opened at `fo` is closed. */
-predicate fileIsClosed(FileOpen fo) { exists(FileClose fc | DataFlow::localFlow(fo, fc)) }
+predicate fileIsClosed(FileOpen fo) { exists(FileClose fc | fileLocalFlow(fo, fc)) }
 
 /** Holds if the file opened at `fo` is returned to the caller. This makes the caller responsible for closing the file. */
 predicate fileIsReturned(FileOpen fo) {
@@ -108,27 +110,26 @@ predicate fileIsReturned(FileOpen fo) {
       or
       retVal = ret.getValue().(Tuple).getAnElt()
     ) and
-    DataFlow::localFlow(fo, DataFlow::exprNode(retVal))
+    fileLocalFlow(fo, DataFlow::exprNode(retVal))
   )
 }
 
 /** Holds if the file opened at `fo` is stored in a field. We assume that another method is then responsible for closing the file. */
 predicate fileIsStoredInField(FileOpen fo) {
-  exists(DataFlow::AttrWrite aw | DataFlow::localFlow(fo, aw.getValue()))
+  exists(DataFlow::AttrWrite aw | fileLocalFlow(fo, aw.getValue()))
 }
 
 /** Holds if the file opened at `fo` is not closed, and is expected to be closed. */
 predicate fileNotClosed(FileOpen fo) {
   not fileIsClosed(fo) and
   not fileIsReturned(fo) and
-  not fileIsStoredInField(fo) and
-  not exists(FileWrapperCall fwc | fo = fwc.getWrapped())
+  not fileIsStoredInField(fo)
 }
 
 predicate fileMayNotBeClosedOnException(FileOpen fo, DataFlow::Node raises) {
   fileIsClosed(fo) and
   mayRaiseWithFile(raises) and
-  DataFlow::localFlow(fo, raises) and
+  fileLocalFlow(fo, raises) and
   not exists(FileClose fc |
     DataFlow::localFlow(fo, fc) and
     fc.guardsExceptions(raises.asExpr())