diff --git a/python/ql/src/Resources/FileNotAlwaysClosed.py b/python/ql/src/Resources/FileNotAlwaysClosed.py deleted file mode 100644 index 5f5f10345c7..00000000000 --- a/python/ql/src/Resources/FileNotAlwaysClosed.py +++ /dev/null @@ -1,15 +0,0 @@ -f = open("filename") - ... # Actions to perform on file -f.close() -# File only closed if actions are completed successfully - -with open("filename") as f: - ...# Actions to perform on file -# File always closed - -f = open("filename") -try: - ... # Actions to perform on file -finally: - f.close() -# File always closed diff --git a/python/ql/src/Resources/FileNotAlwaysClosed.qhelp b/python/ql/src/Resources/FileNotAlwaysClosed.qhelp index 71073caa47b..f37c5a4cbc4 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosed.qhelp +++ b/python/ql/src/Resources/FileNotAlwaysClosed.qhelp @@ -4,32 +4,30 @@ -

If a file is opened then it should always be closed again, even if an -exception is raised. -Failing to ensure that all files are closed may result in failure due to too -many open files.

- +

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 if you open a file it is always closed on exiting the method. -Wrap the code between the open() and close() -functions in a with statement or use a try...finally -statement. Using a with statement is preferred as it is shorter -and more readable.

+

Ensure that opened files are always closed, including when an exception could be raised. +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. +

 -

The following code shows examples of different ways of closing a file. In the first example, the -file is closed only if the method is exited successfully. In the other examples, the file is always -closed on exiting the method.

+

In the following examples, in the case marked BAD, the file may not be closed if an exception is raised. In the cases marked GOOD, the file is always closed.

- + - +
  • Python Documentation: Reading and writing files.
  • Python Language Reference: The with statement, The try statement.
  • Python PEP 343: The "with" Statement.
    • diff --git a/python/ql/src/Resources/FileNotAlwaysClosed.ql b/python/ql/src/Resources/FileNotAlwaysClosed.ql index 5b5a869e62a..c3950eda805 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosed.ql +++ b/python/ql/src/Resources/FileNotAlwaysClosed.ql @@ -1,74 +1,26 @@ /** * @name File is not always closed - * @description Opening a file without ensuring that it is always closed may cause resource leaks. + * @description Opening a file without ensuring that it is always closed may lead to data loss or resource leaks. * @kind problem * @tags efficiency * correctness * resources + * quality * external/cwe/cwe-772 * @problem.severity warning * @sub-severity high - * @precision medium + * @precision high * @id py/file-not-closed */ 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 +from FileOpen fo, string msg 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 + 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, msg diff --git a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll new file mode 100644 index 00000000000..fe1d6578e11 --- /dev/null +++ b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll @@ -0,0 +1,150 @@ +/** Definitions for reasoning about whether files are closed. */ + +import python +import semmle.python.dataflow.new.internal.DataFlowDispatch +import semmle.python.ApiGraphs + +/** A CFG node where a file is opened. */ +abstract class FileOpenSource extends DataFlow::CfgNode { } + +/** 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. + * This includes calls to methods that transitively call `open` or similar. + */ +class FileOpen extends DataFlow::CallCfgNode { + FileOpen() { fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(this) } +} + +/** A call that may wrap a file object in a wrapper class or `os.fdopen`. */ +class FileWrapperCall extends DataFlow::CallCfgNode { + DataFlow::Node wrapped; + + FileWrapperCall() { + wrapped = this.getArg(_).getALocalSource() and + this.getFunction() = classTracker(_) + or + wrapped = this.getArg(0) and + this = API::moduleImport("os").getMember("fdopen").getACall() + or + wrapped = this.getArg(0) and + this = API::moduleImport("django").getMember("http").getMember("FileResponse").getACall() + } + + /** Gets the file that this call wraps. */ + DataFlow::Node getWrapped() { result = wrapped } +} + +/** 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 `raises`. */ + predicate guardsExceptions(DataFlow::CfgNode raises) { + this.asCfgNode() = raises.asCfgNode().getAnExceptionalSuccessor().getASuccessor*() + or + // The expression is after the close call. + // This also covers the body of a `with` statement. + raises.asCfgNode() = this.asCfgNode().getASuccessor*() + } +} + +/** 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() { this.asExpr() = any(With w).getContextExpr() } +} + +/** Holds if an exception may be raised at `raises` if `file` is a file object. */ +private predicate mayRaiseWithFile(DataFlow::CfgNode file, DataFlow::CfgNode raises) { + // Currently just consider any method called on `file`; e.g. `file.write()`; as potentially raising an exception + raises.(DataFlow::MethodCallNode).getObject() = file and + not file instanceof FileOpen and + not file instanceof FileClose +} + +/** Holds if data flows from `nodeFrom` to `nodeTo` in one step that also includes file wrapper classes. */ +private predicate fileAdditionalLocalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) { + exists(FileWrapperCall fw | nodeFrom = fw.getWrapped() and nodeTo = fw) +} + +private predicate fileLocalFlowHelper0( + DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo +) { + exists(DataFlow::Node nodeMid | + nodeFrom.flowsTo(nodeMid) and fileAdditionalLocalFlowStep(nodeMid, nodeTo) + ) +} + +private predicate fileLocalFlowHelper1( + DataFlow::LocalSourceNode nodeFrom, DataFlow::LocalSourceNode nodeTo +) { + fileLocalFlowHelper0*(nodeFrom, nodeTo) +} + +/** Holds if data flows from `source` to `sink`, including file wrapper classes. */ +pragma[inline] +private predicate fileLocalFlow(FileOpen source, DataFlow::Node sink) { + exists(DataFlow::LocalSourceNode mid | fileLocalFlowHelper1(source, mid) and mid.flowsTo(sink)) +} + +/** Holds if the file opened at `fo` is closed. */ +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) { + exists(Return ret, Expr retVal | + ( + retVal = ret.getValue() + or + retVal = ret.getValue().(List).getAnElt() + or + retVal = ret.getValue().(Tuple).getAnElt() + ) and + 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 | 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) +} + +predicate fileMayNotBeClosedOnException(FileOpen fo, DataFlow::Node raises) { + fileIsClosed(fo) and + exists(DataFlow::CfgNode fileRaised | + mayRaiseWithFile(fileRaised, raises) and + fileLocalFlow(fo, fileRaised) and + not exists(FileClose fc | + fileLocalFlow(fo, fc) and + fc.guardsExceptions(raises) + ) + ) +} diff --git a/python/ql/src/Resources/FileOpen.qll b/python/ql/src/Resources/FileOpen.qll index 8fc45306353..dd952e732d4 100644 --- a/python/ql/src/Resources/FileOpen.qll +++ b/python/ql/src/Resources/FileOpen.qll @@ -1,4 +1,8 @@ -/** Contains predicates concerning when and where files are opened and closed. */ +/** + * DEPRECATED: Use FileNotAlwaysClosedQuery instead. + * Contains predicates concerning when and where files are opened and closed. + */ +deprecated module; import python import semmle.python.pointsto.Filters diff --git a/python/ql/src/Resources/examples/FileNotAlwaysClosed.py b/python/ql/src/Resources/examples/FileNotAlwaysClosed.py new file mode 100644 index 00000000000..cd5bdb2118a --- /dev/null +++ b/python/ql/src/Resources/examples/FileNotAlwaysClosed.py @@ -0,0 +1,17 @@ +def bad(): + f = open("filename", "w") + f.write("could raise exception") # BAD: This call could raise an exception, leading to the file not being closed. + f.close() + + +def good1(): + with open("filename", "w") as f: + f.write("always closed") # GOOD: The `with` statement ensures the file is always closed. + +def good2(): + f = open("filename", "w") + try: + f.write("always closed") + finally: + f.close() # GOOD: The `finally` block always ensures the file is closed. + diff --git a/python/ql/test/query-tests/Resources/Dataflow.expected b/python/ql/test/query-tests/Resources/Dataflow.expected deleted file mode 100644 index 18ac2157458..00000000000 --- a/python/ql/test/query-tests/Resources/Dataflow.expected +++ /dev/null @@ -1,119 +0,0 @@ -| f1_0 = open() | open | | -| f1_1 = MethodCallsiteRefinement(f1_0) | open | | -| f1_2 = MethodCallsiteRefinement(f1_1) | closed | exit | -| f2_0 = open() | open | exit | -| f3_0 = open() | open | | -| f3_1 = MethodCallsiteRefinement(f3_0) | closed | exit | -| f4_0 = with | closed | | -| f4_1 = MethodCallsiteRefinement(f4_0) | closed | exit | -| f5_0 = open() | open | | -| f5_1 = MethodCallsiteRefinement(f5_0) | open | | -| f5_2 = MethodCallsiteRefinement(f5_1) | closed | exit | -| f5_3 = phi(f5_0, f5_1) | open | | -| f6_0 = None | closed | | -| f6_1 = open() | open | | -| f6_2 = MethodCallsiteRefinement(f6_1) | open | | -| f6_3 = phi(f6_0, f6_1, f6_2) | open | | -| f6_4 = Pi(f6_2) [true] | open | | -| f6_5 = MethodCallsiteRefinement(f6_4) | closed | | -| f6_6 = Pi(f6_3) [true] | open | | -| f6_7 = Pi(f6_2) [false] | closed | | -| f6_8 = phi(f6_5, f6_7) | closed | exit | -| f7_0 = None | closed | | -| f7_1 = open() | open | | -| f7_2 = MethodCallsiteRefinement(f7_1) | open | | -| f7_3 = phi(f7_0, f7_1, f7_2) | open | | -| f7_4 = Pi(f7_2) [true] | open | | -| f7_5 = MethodCallsiteRefinement(f7_4) | closed | | -| f7_6 = Pi(f7_3) [true] | open | | -| f7_7 = Pi(f7_2) [false] | closed | | -| f7_8 = phi(f7_5, f7_7) | closed | exit | -| f8_0 = None | closed | | -| f8_1 = open() | open | | -| f8_2 = MethodCallsiteRefinement(f8_1) | open | | -| f8_3 = phi(f8_0, f8_1, f8_2) | open | | -| f8_4 = Pi(f8_2) [true] | closed | | -| f8_5 = MethodCallsiteRefinement(f8_4) | closed | | -| f8_6 = Pi(f8_3) [true] | closed | | -| f8_7 = Pi(f8_2) [false] | open | | -| f8_8 = phi(f8_5, f8_7) | open | exit | -| f9_0 = None | closed | | -| f9_1 = open() | open | | -| f9_2 = MethodCallsiteRefinement(f9_1) | open | | -| f9_3 = phi(f9_0, f9_1, f9_2) | open | | -| f9_4 = Pi(f9_2) [true] | closed | | -| f9_5 = MethodCallsiteRefinement(f9_4) | closed | | -| f9_6 = Pi(f9_3) [true] | closed | | -| f9_7 = Pi(f9_2) [false] | open | | -| f9_8 = phi(f9_5, f9_7) | open | exit | -| f10_0 = open() | open | | -| f10_1 = MethodCallsiteRefinement(f10_0) | open | | -| f10_2 = MethodCallsiteRefinement(f10_1) | open | | -| f10_3 = MethodCallsiteRefinement(f10_2) | closed | | -| f10_4 = phi(f10_0, f10_1, f10_2, f10_3) | open | | -| f10_5 = MethodCallsiteRefinement(f10_4) | closed | | -| f10_6 = phi(f10_3, f10_5) | closed | exit | -| f11_0 = open() | open | | -| f11_1 = MethodCallsiteRefinement(f11_0) | open | | -| f11_2 = MethodCallsiteRefinement(f11_1) | open | | -| f11_3 = MethodCallsiteRefinement(f11_2) | closed | | -| f11_4 = phi(f11_0, f11_1, f11_2, f11_3) | open | | -| f11_5 = MethodCallsiteRefinement(f11_4) | closed | | -| f11_6 = phi(f11_3, f11_5) | closed | exit | -| f12_0 = open() | open | | -| f12_1 = MethodCallsiteRefinement(f12_0) | open | | -| f12_2 = MethodCallsiteRefinement(f12_1) | open | | -| f12_3 = MethodCallsiteRefinement(f12_2) | closed | | -| f12_4 = phi(f12_0, f12_1, f12_2, f12_3) | open | | -| f12_5 = MethodCallsiteRefinement(f12_4) | closed | | -| f12_6 = phi(f12_3, f12_5) | closed | exit | -| f13_0 = open() | open | | -| f13_1 = MethodCallsiteRefinement(f13_0) | open | exit | -| f14_0 = opener_func2() | open | | -| f14_1 = MethodCallsiteRefinement(f14_0) | open | | -| f14_2 = MethodCallsiteRefinement(f14_1) | closed | exit | -| f15_0 = opener_func2() | open | | -| f15_1 = ArgumentRefinement(f15_0) | closed | exit | -| f16_0 = ScopeEntryDefinition | closed | | -| f16_1 = open() | open | | -| f16_2 = MethodCallsiteRefinement(f16_1) | open | | -| f16_3 = MethodCallsiteRefinement(f16_2) | closed | | -| f16_4 = phi(f16_0, f16_1, f16_2, f16_3) | open | | -| f16_5 = phi(f16_3, f16_4) | open | exit | -| f17_0 = open() | open | | -| f17_1 = MethodCallsiteRefinement(f17_0) | open | | -| f17_2 = MethodCallsiteRefinement(f17_1) | open | | -| f17_3 = MethodCallsiteRefinement(f17_2) | closed | | -| f17_4 = phi(f17_0, f17_1, f17_2, f17_3) | open | | -| f17_5 = MethodCallsiteRefinement(f17_4) | closed | | -| f17_6 = phi(f17_3, f17_5) | closed | exit | -| f18_0 = open() | closed | | -| f18_1 = MethodCallsiteRefinement(f18_0) | closed | exit | -| f20_0 = open() | open | | -| f20_1 = ArgumentRefinement(f20_0) | closed | exit | -| f21_0 = open() | open | | -| f21_1 = MethodCallsiteRefinement(f21_0) | open | | -| f21_2 = MethodCallsiteRefinement(f21_1) | closed | | -| f21_3 = phi(f21_1, f21_2) | open | | -| f21_4 = phi(f21_0, f21_1, f21_2) | open | | -| f21_5 = Pi(f21_3) [true] | open | | -| f21_6 = MethodCallsiteRefinement(f21_5) | closed | | -| f21_7 = Pi(f21_4) [true] | open | | -| f21_8 = Pi(f21_3) [false] | closed | | -| f21_9 = phi(f21_6, f21_8) | closed | exit | -| f22_0 = open() | open | | -| f22_1 = MethodCallsiteRefinement(f22_0) | open | | -| f22_2 = MethodCallsiteRefinement(f22_1) | closed | | -| f22_3 = phi(f22_1, f22_2) | open | | -| f22_4 = phi(f22_0, f22_1, f22_2) | open | | -| f22_5 = Pi(f22_3) [true] | closed | | -| f22_6 = MethodCallsiteRefinement(f22_5) | closed | | -| f22_7 = Pi(f22_4) [true] | closed | | -| f22_8 = Pi(f22_3) [false] | open | | -| f22_9 = phi(f22_6, f22_8) | open | exit | -| f_0 = FunctionExpr | closed | exit | -| file_0 = open() | open | | -| file_1 = open() | open | | -| file_2 = None | closed | | -| file_3 = phi(file_0, file_1, file_2) | open | exit | -| fp_0 = ParameterDefinition | closed | exit | diff --git a/python/ql/test/query-tests/Resources/Dataflow.ql b/python/ql/test/query-tests/Resources/Dataflow.ql deleted file mode 100644 index fad31d80ec1..00000000000 --- a/python/ql/test/query-tests/Resources/Dataflow.ql +++ /dev/null @@ -1,14 +0,0 @@ -import python -import Resources.FileOpen - -from EssaVariable v, EssaDefinition def, string open, string exit -where - def = v.getDefinition() and - v.getSourceVariable().getName().charAt(0) = "f" and - ( - var_is_open(v, _) and open = "open" - or - not var_is_open(v, _) and open = "closed" - ) and - if BaseFlow::reaches_exit(v) then exit = "exit" else exit = "" -select v.getRepresentation() + " = " + v.getDefinition().getRepresentation(), open, exit diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed.expected deleted file mode 100644 index c0a6c413333..00000000000 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed.expected +++ /dev/null @@ -1,9 +0,0 @@ -| resources_test.py:4:10:4:25 | open() | File may not be closed if an exception is raised. | -| resources_test.py:9:10:9:25 | open() | File is opened but is not closed. | -| resources_test.py:49:14:49:29 | open() | File is opened but is not closed. | -| resources_test.py:58:14:58:29 | open() | File is opened but is not closed. | -| resources_test.py:79:11:79:26 | open() | File may not be closed if an exception is raised. | -| resources_test.py:108:11:108:20 | open() | File is opened but is not closed. | -| resources_test.py:112:11:112:28 | opener_func2() | File may not be closed if an exception is raised. | -| resources_test.py:129:15:129:24 | open() | File is opened but is not closed. | -| resources_test.py:237:11:237:26 | open() | File is opened but is not closed. | diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed.qlref b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed.qlref deleted file mode 100644 index 37e9a076680..00000000000 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed.qlref +++ /dev/null @@ -1 +0,0 @@ -Resources/FileNotAlwaysClosed.ql \ No newline at end of file diff --git a/python/ql/test/query-tests/Resources/resources_test.py b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py similarity index 68% rename from python/ql/test/query-tests/Resources/resources_test.py rename to python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py index 73fa88b4401..598d54c892c 100644 --- a/python/ql/test/query-tests/Resources/resources_test.py +++ b/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() + f1.close() def not_close2(): - f2 = open("filename") + f2 = open("filename") # $ notClosed def closed3(): f3 = open("filename") @@ -46,19 +46,19 @@ def closed7(): def not_closed8(): f8 = None try: - f8 = open("filename") + 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(): f9 = None try: - f9 = open("filename") + 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(): @@ -76,19 +76,19 @@ def closed10(): #Not closed by handling the wrong exception def not_closed11(): - f11 = open("filename") + f11 = open("filename") # $ MISSING:notAlwaysClosed try: 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(): +def doesnt_raise(*args): pass def mostly_closed12(): - f12 = open("filename") + 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) + f13 = open(name) # $ notClosed f13.write("Hello") def may_not_be_closed14(name): - f14 = opener_func2(name) + f14 = opener_func2(name) # $ notClosedOnException f14.write("Hello") f14.close() @@ -120,13 +120,13 @@ def closer2(t3): closer1(t3) def closed15(): - f15 = opener_func2() - closer2(f15) + f15 = opener_func2() # $ SPURIOUS:notClosed + closer2(f15) # We don't detect that this call closes the file, so this result is SPURIOUS. def may_not_be_closed16(name): try: - f16 = open(name) + f16 = open(name) # $ notClosedOnException f16.write("Hello") f16.close() except IOError: @@ -138,13 +138,13 @@ def may_raise(): #Not handling all exceptions, but we'll tolerate the false negative def not_closed17(): - f17 = open("filename") + f17 = open("filename") # $ MISSING:notClosedOnException try: f17.write("IOError could occur") 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 @@ -234,13 +234,47 @@ def closed21(path): def not_closed22(path): - f22 = open(path, "wb") + f22 = open(path, "wb") # $ MISSING:notClosedOnException try: f22.write(b"foo") may_raise() 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): + f23 = open(path, "w") # $ notClosed + wr = FileWrapper(f23) + +def closed24(path): + f24 = open(path, "w") + try: + f24.write("hi") + except: + pass + f24.close() + +def closed25(path): + from django.http import FileResponse + return FileResponse(open(path)) + +import os +def closed26(path): + fd = os.open(path) + os.close(fd) + +def not_closed27(path): + fd = os.open(path, "w") # $notClosedOnException + f27 = os.fdopen(fd, "w") + f27.write("hi") + f27.close() + +def closed28(path): + fd = os.open(path, os.O_WRONLY) + f28 = os.fdopen(fd, "w") + try: + f28.write("hi") + finally: + f28.close() \ No newline at end of file diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql new file mode 100644 index 00000000000..f176172d078 --- /dev/null +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql @@ -0,0 +1,25 @@ +import python +import Resources.FileNotAlwaysClosedQuery +import utils.test.InlineExpectationsTest + +module MethodArgTest implements TestSig { + string getARelevantTag() { result = ["notClosed", "notClosedOnException"] } + + predicate hasActualResult(Location location, string element, string tag, string value) { + exists(DataFlow::CfgNode el, FileOpen fo | + el = fo and + element = el.toString() and + location = el.getLocation() and + value = "" and + ( + fileNotClosed(fo) and + tag = "notClosed" + or + fileMayNotBeClosedOnException(fo, _) and + tag = "notClosedOnException" + ) + ) + } +} + +import MakeTest