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

Author: joefarebrother

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

python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py

@@ -46,10 +46,10 @@ def closed7():
 def not_closed8():
     f8 = None
     try:
-        f8 = open("filename") # $ MISSING:notClosedOnException
+        f8 = open("filename") # $ MISSING:notClosedOnException 
         f8.write("Error could occur")
     finally:
-        if f8 is None:
+        if f8 is None: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon.
             f8.close()
 
 def not_closed9():
@@ -58,7 +58,7 @@ def not_closed9():
         f9 = open("filename") # $ MISSING:notAlwaysClosed
         f9.write("Error could occur")
     finally:
-        if not f9:
+        if not f9: # We don't precisely consider this condition, so this result is MISSING.However, this seems uncommon.
             f9.close()
 
 def not_closed_but_cant_tell_locally():
@@ -81,7 +81,7 @@ def not_closed11():
         f11.write("IOError could occur")
         f11.write("IOError could occur")
         f11.close()
-    except AttributeError:
+    except AttributeError: # We don't consider the type of exception handled here, so this result is MISSING.
         f11.close()
 
 def doesnt_raise(*args):
@@ -121,7 +121,7 @@ def closer2(t3):
 
 def closed15():
     f15 = opener_func2() # $ SPURIOUS:notClosed
-    closer2(f15)
+    closer2(f15) # We don't detect that this call closes the file, so this result is SPURIOUS.
 
 
 def may_not_be_closed16(name):
@@ -144,7 +144,7 @@ def not_closed17():
         f17.write("IOError could occur")
         may_raise("ValueError could occur") # FN here.
         f17.close()
-    except IOError:
+    except IOError: # We don't detect that a ValueErrror could be raised that isn't handled here, so this result is MISSING.
         f17.close()
 
 #ODASA-3779
@@ -241,7 +241,7 @@ def not_closed22(path):
         if foo:
             f22.close()
     finally:
-        if f22.closed: # Wrong sense
+        if f22.closed: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon.
             f22.close()
 
 def not_closed23(path):

python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql

@@ -7,7 +7,7 @@ module MethodArgTest implements TestSig {
 
   predicate hasActualResult(Location location, string element, string tag, string value) {
     exists(DataFlow::CfgNode el, FileOpen fo |
-      el = fo.getLocalSource() and
+      el = fo and
       element = el.toString() and
       location = el.getLocation() and
       value = "" and