diff --git a/python/ql/src/Resources/FileNotAlwaysClosed.qhelp b/python/ql/src/Resources/FileNotAlwaysClosed.qhelp index 5c3e0f98a0e..f37c5a4cbc4 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosed.qhelp +++ b/python/ql/src/Resources/FileNotAlwaysClosed.qhelp @@ -4,13 +4,16 @@ -

When a file is opened, it should always be closed. Failure to close files could result in loss of data or resource leaks.

- +

When a file is opened, it should always be closed. +

+

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. +

Ensure that opened files are always closed, including when an exception could be raised. -The best practice is to use a with statement to automatically clean up resources. +The best practice is often to use a with statement to automatically clean up resources. Otherwise, ensure that .close() is called in a try...except or try...finally block to handle any possible exceptions.

diff --git a/python/ql/src/Resources/FileNotAlwaysClosed.ql b/python/ql/src/Resources/FileNotAlwaysClosed.ql index 2f40ec3eb5f..4bfba62b213 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosed.ql +++ b/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 diff --git a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll index 1bfc40407d6..11473edde04 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll +++ b/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()) diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py index ddc89a8fb7b..1f30d309d6f 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py +++ b/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): diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql index 3abba197373..f176172d078 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql +++ b/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