Add case for exception flow

Author: joefarebrother

python/ql/src/Resources/FileNotAlwaysClosed.ql

@@ -13,63 +13,13 @@
  */
 
 import python
-// import FileOpen
 import FileNotAlwaysClosedQuery
 
-// /**
-//  * Whether resource is opened and closed in  in a matched pair of methods,
-//  * either `__enter__` and `__exit__` or `__init__` and `__del__`
-//  */
-// predicate opened_in_enter_closed_in_exit(ControlFlowNode open) {
-//   file_not_closed_at_scope_exit(open) and
-//   exists(FunctionValue entry, FunctionValue exit |
-//     open.getScope() = entry.getScope() and
-//     exists(ClassValue cls |
-//       cls.declaredAttribute("__enter__") = entry and cls.declaredAttribute("__exit__") = exit
-//       or
-//       cls.declaredAttribute("__init__") = entry and cls.declaredAttribute("__del__") = exit
-//     ) and
-//     exists(AttrNode attr_open, AttrNode attrclose |
-//       attr_open.getScope() = entry.getScope() and
-//       attrclose.getScope() = exit.getScope() and
-//       expr_is_open(attr_open.(DefinitionNode).getValue(), open) and
-//       attr_open.getName() = attrclose.getName() and
-//       close_method_call(_, attrclose)
-//     )
-//   )
-// }
-// predicate file_not_closed_at_scope_exit(ControlFlowNode open) {
-//   exists(EssaVariable v |
-//     BaseFlow::reaches_exit(v) and
-//     var_is_open(v, open) and
-//     not file_is_returned(v, open)
-//   )
-//   or
-//   call_to_open(open) and
-//   not exists(AssignmentDefinition def | def.getValue() = open) and
-//   not exists(Return r | r.getValue() = open.getNode())
-// }
-// predicate file_not_closed_at_exception_exit(ControlFlowNode open, ControlFlowNode exit) {
-//   exists(EssaVariable v |
-//     exit.(RaisingNode).viableExceptionalExit(_, _) and
-//     not closes_arg(exit, v.getSourceVariable()) and
-//     not close_method_call(exit, v.getAUse().(NameNode)) and
-//     var_is_open(v, open) and
-//     v.getAUse() = exit.getAChild*()
-//   )
-// }
-/* Check to see if a file is opened but not closed or returned */
-// from ControlFlowNode defn, string message
-// where
-//   not opened_in_enter_closed_in_exit(defn) and
-//   (
-//     file_not_closed_at_scope_exit(defn) and message = "File is opened but is not closed."
-//     or
-//     not file_not_closed_at_scope_exit(defn) and
-//     file_not_closed_at_exception_exit(defn, _) and
-//     message = "File may not be closed if an exception is raised."
-//   )
-// select defn.getNode(), message
-from FileOpen fo
-where fileNotAlwaysClosed(fo)
-select fo, "File is opened but is not closed."
+from FileOpen fo, string msg
+where
+  fileNotClosed(fo) and
+  msg = "File is opened but is not closed."
+  or
+  fileMayNotBeClosedOnException(fo, _) and
+  msg = "File may not be closed if an exception is raised"
+select fo.getLocalSource(), msg

python/ql/src/Resources/FileNotAlwaysClosedQuery.qll

@@ -4,50 +4,133 @@ import python
 import semmle.python.dataflow.new.internal.DataFlowDispatch
 import semmle.python.ApiGraphs
 
-abstract class FileOpen extends DataFlow::CfgNode { }
+/** A CFG node where a file is opened. */
+abstract class FileOpenSource extends DataFlow::CfgNode { }
 
-class FileOpenCall extends FileOpen {
-  FileOpenCall() { this = [API::builtin("open").getACall()] }
+/** A call to the builtin `open` or `os.open`. */
+class FileOpenCall extends FileOpenSource {
+  FileOpenCall() {
+    this = [API::builtin("open").getACall(), API::moduleImport("os").getMember("open").getACall()]
+  }
+}
+
+private DataFlow::TypeTrackingNode fileOpenInstance(DataFlow::TypeTracker t) {
+  t.start() and
+  result instanceof FileOpenSource
+  or
+  exists(DataFlow::TypeTracker t2 | result = fileOpenInstance(t2).track(t2, 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
+  }
 }
 
-class FileWrapperClassCall extends FileOpen, DataFlow::CallCfgNode {
+/** A call that may wrap a file object in a wrapper class or `os.fdopen`. */
+class FileWrapperCall extends FileOpenSource, DataFlow::CallCfgNode {
   FileOpen wrapped;
 
-  FileWrapperClassCall() {
+  FileWrapperCall() {
     wrapped = this.getArg(_).getALocalSource() and
     this.getFunction() = classTracker(_)
+    or
+    wrapped = this.getArg(0) and
+    this = API::moduleImport("os").getMember("fdopen").getACall()
   }
 
+  /** Gets the file that this call wraps. */
   FileOpen getWrapped() { result = wrapped }
 }
 
-abstract class FileClose extends DataFlow::CfgNode { }
+/** A node where a file is closed. */
+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*()
+      )
+    )
+  }
+}
 
+/** A call to the `.close()` method of a file object. */
 class FileCloseCall extends FileClose {
   FileCloseCall() { exists(DataFlow::MethodCallNode mc | mc.calls(this, "close")) }
 }
 
+/** A call to `os.close`. */
 class OsCloseCall extends FileClose {
   OsCloseCall() { this = API::moduleImport("os").getMember("close").getACall().getArg(0) }
 }
 
+/** A `with` statement. */
 class WithStatement extends FileClose {
-  WithStatement() { exists(With w | this.asExpr() = w.getContextExpr()) }
+  With w;
+
+  WithStatement() { this.asExpr() = w.getContextExpr() }
+
+  override predicate guardsExceptions(Expr e) {
+    super.guardsExceptions(e)
+    or
+    e = w.getAStmt().getAChildNode*()
+  }
 }
 
+/** Holds if an exception may be raised at `node` if it is a file object. */
+private predicate mayRaiseWithFile(DataFlow::CfgNode node) {
+  // Currently just consider any method called on `node`; e.g. `file.write()`; as potentially raising an exception
+  exists(DataFlow::MethodCallNode mc | node = mc.getObject()) and
+  not node instanceof FileOpen and
+  not node instanceof FileClose
+}
+
+/** Holds if the file opened at `fo` is closed. */
 predicate fileIsClosed(FileOpen fo) { exists(FileClose fc | DataFlow::localFlow(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) {
-  exists(Return ret | DataFlow::localFlow(fo, DataFlow::exprNode(ret.getValue())))
+  exists(Return ret, Expr retVal |
+    (
+      retVal = ret.getValue()
+      or
+      retVal = ret.getValue().(List).getAnElt()
+      or
+      retVal = ret.getValue().(Tuple).getAnElt()
+    ) and
+    DataFlow::localFlow(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()))
 }
 
-predicate fileNotAlwaysClosed(FileOpen fo) {
+/** 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(FileWrapperClassCall fwc | fo = fwc.getWrapped())
+  not exists(FileWrapperCall fwc | fo = fwc.getWrapped())
+}
+
+predicate fileMayNotBeClosedOnException(FileOpen fo, DataFlow::Node raises) {
+  fileIsClosed(fo) and
+  mayRaiseWithFile(raises) and
+  DataFlow::localFlow(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

@@ -1,12 +1,12 @@
 #File not always closed
 
 def not_close1():
-    f1 = open("filename")
+    f1 = open("filename") # $ notClosedOnException
     f1.write("Error could occur")
-    f1.close() # $ notAlwaysClosed
+    f1.close() 
 
 def not_close2():
-    f2 = open("filename") # $ notAlwaysClosed
+    f2 = open("filename") # $ notClosed
 
 def closed3():
     f3 = open("filename")
@@ -46,7 +46,7 @@ def closed7():
 def not_closed8():
     f8 = None
     try:
-        f8 = open("filename") # $ notAlwaysClosed
+        f8 = open("filename") # $ MISSING:notClosedOnException
         f8.write("Error could occur")
     finally:
         if f8 is None:
@@ -55,7 +55,7 @@ def not_closed8():
 def not_closed9():
     f9 = None
     try:
-        f9 = open("filename") # $ notAlwaysClosed
+        f9 = open("filename") # $ MISSING:notAlwaysClosed
         f9.write("Error could occur")
     finally:
         if not f9:
@@ -76,7 +76,7 @@ def closed10():
 
 #Not closed by handling the wrong exception
 def not_closed11():
-    f11 = open("filename") # $ notAlwaysClosed
+    f11 = open("filename") # $ MISSING:notAlwaysClosed
     try:
         f11.write("IOError could occur")
         f11.write("IOError could occur")
@@ -88,7 +88,7 @@ def doesnt_raise(*args):
     pass
 
 def mostly_closed12():
-    f12 = open("filename") # $ SPURIOUS:notAlwaysClosed
+    f12 = open("filename") 
     try:
         f12.write("IOError could occur")
         f12.write("IOError could occur")
@@ -105,11 +105,11 @@ def opener_func2(name):
     return t1
 
 def not_closed13(name):
-    f13 = open(name) # $ notAlwaysClosed
+    f13 = open(name) # $ notClosed
     f13.write("Hello")
 
 def may_not_be_closed14(name):
-    f14 = opener_func2(name) # $ notAlwaysClosed
+    f14 = opener_func2(name) # $ notClosedOnException
     f14.write("Hello")
     f14.close()
 
@@ -120,13 +120,13 @@ def closer2(t3):
     closer1(t3)
 
 def closed15():
-    f15 = opener_func2()
+    f15 = opener_func2() # $ SPURIOUS:notClosed
     closer2(f15)
 
 
 def may_not_be_closed16(name):
     try:
-        f16 = open(name) # $ notAlwaysClosed
+        f16 = open(name) # $ notClosedOnException
         f16.write("Hello")
         f16.close()
     except IOError:
@@ -138,7 +138,7 @@ def may_raise():
 
 #Not handling all exceptions, but we'll tolerate the false negative
 def not_closed17():
-    f17 = open("filename") # $ notAlwaysClosed
+    f17 = open("filename") # $ MISSING:notClosedOnException
     try:
         f17.write("IOError could occur")
         f17.write("IOError could occur")
@@ -234,7 +234,7 @@ def closed21(path):
 
 
 def not_closed22(path):
-    f22 = open(path, "wb") # $ notAlwaysClosed
+    f22 = open(path, "wb") # $ MISSING:notClosedOnException
     try:
         f22.write(b"foo")
         may_raise()
@@ -244,3 +244,6 @@ def not_closed22(path):
         if f22.closed: # Wrong sense
             f22.close()
 
+def not_closed23(path):
+    f23 = open(path, "w") # $ notClosed
+    wr = FileWrapper(f23)

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

@@ -1,21 +1,25 @@
-import python 
+import python
 import Resources.FileNotAlwaysClosedQuery
 import utils.test.InlineExpectationsTest
 
 module MethodArgTest implements TestSig {
-  string getARelevantTag() { result = "notAlwaysClosed" }
+  string getARelevantTag() { result = ["notClosed", "notClosedOnException"] }
 
   predicate hasActualResult(Location location, string element, string tag, string value) {
-    exists(DataFlow::CfgNode f |
-      element = f.toString() and
-      location = f.getLocation() and
+    exists(DataFlow::CfgNode el, FileOpen fo |
+      el = fo.getLocalSource() and
+      element = el.toString() and
+      location = el.getLocation() and
       value = "" and
       (
-        fileNotAlwaysClosed(f) and
-        tag = "notAlwaysClosed"
+        fileNotClosed(fo) and
+        tag = "notClosed"
+        or
+        fileMayNotBeClosedOnException(fo, _) and
+        tag = "notClosedOnException"
       )
     )
   }
 }
 
-import MakeTest<MethodArgTest>
+import MakeTest<MethodArgTest>