From bd3fa7fb21d5f2a4c61e7f75b09a3e7f9a4aeb48 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 5 Sep 2025 16:03:55 +0100 Subject: [PATCH 1/6] Switch to dataflow check for guards exceptions This reduces some confusing FPs, though appears to introduce another --- .../ql/src/Resources/FileNotAlwaysClosed.ql | 12 ++-- .../Resources/FileNotAlwaysClosedQuery.qll | 28 +++------ .../FileNotAlwaysClosed.expected | 10 +++ .../FileNotAlwaysClosed.qlref | 2 + .../FileNotAlwaysClosed/resources_test.py | 62 +++++++++++++++---- .../FileNotAlwaysClosed/test.expected | 0 .../Resources/FileNotAlwaysClosed/test.ql | 25 -------- 7 files changed, 76 insertions(+), 63 deletions(-) create mode 100644 python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected create mode 100644 python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.qlref delete mode 100644 python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.expected delete mode 100644 python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql diff --git a/python/ql/src/Resources/FileNotAlwaysClosed.ql b/python/ql/src/Resources/FileNotAlwaysClosed.ql index f639bc4aa912..4e0b897c9bb3 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosed.ql +++ b/python/ql/src/Resources/FileNotAlwaysClosed.ql @@ -15,12 +15,14 @@ import python import FileNotAlwaysClosedQuery +import codeql.util.Option -from FileOpen fo, string msg +from FileOpen fo, string msg, LocatableOption::Option exec where fileNotClosed(fo) and - msg = "File is opened but is not closed." + msg = "File is opened but is not closed." and + exec.isNone() or - fileMayNotBeClosedOnException(fo, _) and - msg = "File may not be closed if an exception is raised." -select fo, msg + fileMayNotBeClosedOnException(fo, exec.asSome()) and + msg = "File may not be closed if $@ raises an exception." +select fo, msg, exec, "this operation" diff --git a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll index 0122344d370e..94d34d9abc5e 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll +++ b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll @@ -34,6 +34,8 @@ class FileWrapperCall extends DataFlow::CallCfgNode { DataFlow::Node wrapped; FileWrapperCall() { + // Approximation: Treat any passing of a file object to a class constructor as potentially a wrapper + // This could be made more precise by checking that the constructor writes the file to a field. wrapped = this.getArg(_).getALocalSource() and this.getFunction() = classTracker(_) or @@ -51,21 +53,14 @@ class FileWrapperCall extends DataFlow::CallCfgNode { /** A node where a file is closed. */ abstract class FileClose extends DataFlow::CfgNode { /** Holds if this file close will occur if an exception is raised at `raises`. */ - predicate guardsExceptions(DataFlow::CfgNode raises) { + predicate guardsExceptions(DataFlow::CfgNode fileRaises) { // The close call occurs after an exception edge in the cfg (a catch or finally) - bbReachableRefl(raises.asCfgNode().getBasicBlock().getAnExceptionalSuccessor(), + bbReachableRefl(fileRaises.asCfgNode().getBasicBlock().getAnExceptionalSuccessor(), this.asCfgNode().getBasicBlock()) or // The exception is after the close call. - // A full cfg reachability check is not in general feasible for performance, so we approximate it with: - // - A basic block reachability check (here) that works if the expression and close call are in different basic blocks - // - A check (in the `WithStatement` override of `guardsExceptions`) for the case where the exception call - // is lexically contained in the body of a `with` statement that closes the file. - // This may cause FPs in a case such as: - // f.close() - // f.write("...") - // We presume this to not be very common. - bbReachableStrict(this.asCfgNode().getBasicBlock(), raises.asCfgNode().getBasicBlock()) + // A full cfg reachability check is not feasible for performance, instead we use local dataflow + fileLocalFlow(this, fileRaises) } } @@ -93,13 +88,6 @@ class WithStatement extends FileClose { With w; WithStatement() { this.asExpr() = w.getContextExpr() } - - override predicate guardsExceptions(DataFlow::CfgNode raises) { - super.guardsExceptions(raises) - or - // Check whether the exception is raised in the body of the with statement. - raises.asExpr().getParent*() = w.getBody().getAnItem() - } } /** Holds if an exception may be raised at `raises` if `file` is a file object. */ @@ -131,7 +119,7 @@ private predicate fileLocalFlowHelper1( /** Holds if data flows from `source` to `sink`, including file wrapper classes. */ pragma[inline] -private predicate fileLocalFlow(FileOpen source, DataFlow::Node sink) { +private predicate fileLocalFlow(DataFlow::Node source, DataFlow::Node sink) { exists(DataFlow::LocalSourceNode mid | fileLocalFlowHelper1(source, mid) and mid.flowsTo(sink)) } @@ -171,7 +159,7 @@ predicate fileMayNotBeClosedOnException(FileOpen fo, DataFlow::Node raises) { fileLocalFlow(fo, fileRaised) and not exists(FileClose fc | fileLocalFlow(fo, fc) and - fc.guardsExceptions(raises) + fc.guardsExceptions(fileRaised) ) ) } diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected new file mode 100644 index 000000000000..f5527589f807 --- /dev/null +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected @@ -0,0 +1,10 @@ +| resources_test.py:4:10:4:25 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:5:5:5:33 | ControlFlowNode for Attribute() | this operation | +| resources_test.py:9:10:9:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | +| resources_test.py:108:11:108:20 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | +| resources_test.py:112:11:112:28 | ControlFlowNode for opener_func2() | File may not be closed if $@ raises an exception. | resources_test.py:113:5:113:22 | ControlFlowNode for Attribute() | this operation | +| resources_test.py:123:11:123:24 | ControlFlowNode for opener_func2() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | +| resources_test.py:129:15:129:24 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:130:9:130:26 | ControlFlowNode for Attribute() | this operation | +| resources_test.py:248:11:248:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | +| resources_test.py:269:10:269:27 | ControlFlowNode for Attribute() | File may not be closed if $@ raises an exception. | resources_test.py:271:5:271:19 | ControlFlowNode for Attribute() | this operation | +| resources_test.py:285:11:285:20 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:287:5:287:31 | ControlFlowNode for Attribute() | this operation | +| resources_test.py:305:10:305:19 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:308:5:308:24 | ControlFlowNode for Attribute() | this operation | diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.qlref b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.qlref new file mode 100644 index 000000000000..57ffce32b8bc --- /dev/null +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.qlref @@ -0,0 +1,2 @@ +query: Resources/FileNotAlwaysClosed.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql \ No newline at end of file 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 244c6f73c133..1eb380013c3f 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/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") # $ notClosedOnException + f1 = open("filename") # $ Alert # not closed on exception f1.write("Error could occur") f1.close() def not_close2(): - f2 = open("filename") # $ notClosed + f2 = open("filename") # $ Alert def closed3(): f3 = open("filename") @@ -46,7 +46,7 @@ def closed7(): def not_closed8(): f8 = None try: - f8 = open("filename") # $ MISSING:notClosedOnException + f8 = open("filename") # $ MISSING:Alert # not closed on exception f8.write("Error could occur") finally: if f8 is None: # We don't precisely consider this condition, so this result is MISSING. However, this seems uncommon. @@ -105,11 +105,11 @@ def opener_func2(name): return t1 def not_closed13(name): - f13 = open(name) # $ notClosed + f13 = open(name) # $ Alert f13.write("Hello") def may_not_be_closed14(name): - f14 = opener_func2(name) # $ notClosedOnException + f14 = opener_func2(name) # $ Alert # not closed on exception f14.write("Hello") f14.close() @@ -120,13 +120,13 @@ def closer2(t3): closer1(t3) def closed15(): - f15 = opener_func2() # $ SPURIOUS:notClosed + f15 = opener_func2() # $ SPURIOUS:Alert 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) # $ notClosedOnException + f16 = open(name) # $ Alert # not closed on exception 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") # $ MISSING:notClosedOnException + f17 = open("filename") # $ MISSING:Alert # not closed on exception 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") # $ MISSING:notClosedOnException + f22 = open(path, "wb") # $ MISSING:Alert # not closed on exception try: f22.write(b"foo") may_raise() @@ -245,7 +245,7 @@ def not_closed22(path): f22.close() def not_closed23(path): - f23 = open(path, "w") # $ notClosed + f23 = open(path, "w") # $ Alert wr = FileWrapper(f23) def closed24(path): @@ -266,7 +266,7 @@ def closed26(path): os.close(fd) def not_closed27(path): - fd = os.open(path, "w") # $notClosedOnException + fd = os.open(path, "w") # $Alert # not closed on exception f27 = os.fdopen(fd, "w") f27.write("hi") f27.close() @@ -282,6 +282,42 @@ def closed28(path): def closed29(path): # Due to an approximation in CFG reachability for performance, it is not detected that the `write` call that may raise occurs after the file has already been closed. # We presume this case to be uncommon. - f28 = open(path) # $SPURIOUS:notClosedOnException + f28 = open(path) # $SPURIOUS:Alert # not closed on exception f28.close() - f28.write("already closed") \ No newline at end of file + f28.write("already closed") + +# False positive: + +class NotWrapper: + def __init__(self, fp): + self.data = fp.read() + fp.close() + + def do_something(): + pass + +def closed30(path): + # Combination of approximations resulting in this FP: + # - NotWrapper is treated as a wrapper class as a file handle is passed to it + # - thing.do_something() is treated as a call that can raise an exception while a file is open + # - this call is treated as occurring after the open but not as being guarded by the with statement, as it is in the same basic block + + with open(path) as fp: # $SPURIOUS:Alert # not closed on exception + thing = NotWrapper(fp) + + thing.do_something() + + +def closed31(path): + with open(path) as fp: + data = fp.readline() + data2 = fp.readline() + + +class FlowReader(): + def __init__(self, f): + pass +def test_cannot_convert(tdata): + with open(tdata, "rb") as f: + flow_reader = FlowReader(f) + list(flow_reader.stream()) \ 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 deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql deleted file mode 100644 index f176172d0782..000000000000 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/test.ql +++ /dev/null @@ -1,25 +0,0 @@ -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 From 0b293eaba515ef2da98aa1d976a6ed7166f8412a Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Fri, 5 Sep 2025 22:43:21 +0100 Subject: [PATCH 2/6] Update test output --- .../FileNotAlwaysClosed.expected | 2 +- .../FileNotAlwaysClosed/resources_test.py | 31 +++++++++++++------ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected index f5527589f807..ffa392b03f10 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected @@ -4,7 +4,7 @@ | resources_test.py:112:11:112:28 | ControlFlowNode for opener_func2() | File may not be closed if $@ raises an exception. | resources_test.py:113:5:113:22 | ControlFlowNode for Attribute() | this operation | | resources_test.py:123:11:123:24 | ControlFlowNode for opener_func2() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:129:15:129:24 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:130:9:130:26 | ControlFlowNode for Attribute() | this operation | +| resources_test.py:154:15:154:24 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:159:9:159:18 | ControlFlowNode for Attribute() | this operation | | resources_test.py:248:11:248:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:269:10:269:27 | ControlFlowNode for Attribute() | File may not be closed if $@ raises an exception. | resources_test.py:271:5:271:19 | ControlFlowNode for Attribute() | this operation | | resources_test.py:285:11:285:20 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:287:5:287:31 | ControlFlowNode for Attribute() | this operation | -| resources_test.py:305:10:305:19 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:308:5:308:24 | ControlFlowNode for Attribute() | this operation | 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 1eb380013c3f..d0aba731182a 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py @@ -151,11 +151,11 @@ def not_closed17(): #With statement will close the fp def closed18(path): try: - f18 = open(path) + f18 = open(path) # $SPURIOUS: Alert # Dataflow appears to not detect this with statement as guarding the exceptions produced by the `read()` call. except IOError as ex: print(ex) raise ex - with f18: + with f18: f18.read() class Closed19(object): @@ -286,7 +286,7 @@ def closed29(path): f28.close() f28.write("already closed") -# False positive: +# False positive in a previous implementation: class NotWrapper: def __init__(self, fp): @@ -297,12 +297,13 @@ def do_something(): pass def closed30(path): - # Combination of approximations resulting in this FP: + # Combination of approximations resulted in this FP: # - NotWrapper is treated as a wrapper class as a file handle is passed to it # - thing.do_something() is treated as a call that can raise an exception while a file is open # - this call is treated as occurring after the open but not as being guarded by the with statement, as it is in the same basic block + # - - this behaviour has been changed fixing the FP - with open(path) as fp: # $SPURIOUS:Alert # not closed on exception + with open(path) as fp: # No longer spurious alert here. thing = NotWrapper(fp) thing.do_something() @@ -314,10 +315,20 @@ def closed31(path): data2 = fp.readline() -class FlowReader(): +class Wrapper(): def __init__(self, f): + self.f = f + def read(self): + return self.f.read() + def __enter__(self): pass -def test_cannot_convert(tdata): - with open(tdata, "rb") as f: - flow_reader = FlowReader(f) - list(flow_reader.stream()) \ No newline at end of file + def __exit__(self): + self.f.close() + +def closed32(path): + with open(path, "rb") as f: # No longer spurious alert here. + wrap = Wrapper(f) + # This resulted in an FP in a previous implementation, + # due to a check that an operation is lexically contained within a `with` block (with `expr.getParent*()`) + # not detecting this case. + return list(wrap.read()) \ No newline at end of file From e382f7cd43fc5417e59cdda5ff9032f65109febb Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 9 Sep 2025 11:26:17 +0100 Subject: [PATCH 3/6] Improve check for containment in with statement --- python/ql/src/Resources/FileNotAlwaysClosedQuery.qll | 6 ++++++ .../Resources/FileNotAlwaysClosed/resources_test.py | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll index 94d34d9abc5e..36807ca39f1a 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll +++ b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll @@ -88,6 +88,12 @@ class WithStatement extends FileClose { With w; WithStatement() { this.asExpr() = w.getContextExpr() } + + override predicate guardsExceptions(DataFlow::CfgNode fileRaises) { + super.guardsExceptions(fileRaises) + or + w.getBody().contains(fileRaises.asExpr()) + } } /** Holds if an exception may be raised at `raises` if `file` is a file object. */ 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 d0aba731182a..0f9edc02803b 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py @@ -151,7 +151,7 @@ def not_closed17(): #With statement will close the fp def closed18(path): try: - f18 = open(path) # $SPURIOUS: Alert # Dataflow appears to not detect this with statement as guarding the exceptions produced by the `read()` call. + f18 = open(path) # $Alert except IOError as ex: print(ex) raise ex @@ -301,7 +301,7 @@ def closed30(path): # - NotWrapper is treated as a wrapper class as a file handle is passed to it # - thing.do_something() is treated as a call that can raise an exception while a file is open # - this call is treated as occurring after the open but not as being guarded by the with statement, as it is in the same basic block - # - - this behaviour has been changed fixing the FP + # - - this behavior has been changed fixing the FP with open(path) as fp: # No longer spurious alert here. thing = NotWrapper(fp) From b01b40b51b8b55a72c50a477d187fdd3e557550b Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 9 Sep 2025 13:44:03 +0100 Subject: [PATCH 4/6] Update test output --- .../FileNotAlwaysClosed/FileNotAlwaysClosed.expected | 4 +++- .../Resources/FileNotAlwaysClosed/resources_test.py | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected index ffa392b03f10..579424e9318f 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected @@ -1,10 +1,12 @@ +#select | resources_test.py:4:10:4:25 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:5:5:5:33 | ControlFlowNode for Attribute() | this operation | | resources_test.py:9:10:9:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:108:11:108:20 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:112:11:112:28 | ControlFlowNode for opener_func2() | File may not be closed if $@ raises an exception. | resources_test.py:113:5:113:22 | ControlFlowNode for Attribute() | this operation | | resources_test.py:123:11:123:24 | ControlFlowNode for opener_func2() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:129:15:129:24 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:130:9:130:26 | ControlFlowNode for Attribute() | this operation | -| resources_test.py:154:15:154:24 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:159:9:159:18 | ControlFlowNode for Attribute() | this operation | | resources_test.py:248:11:248:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:269:10:269:27 | ControlFlowNode for Attribute() | File may not be closed if $@ raises an exception. | resources_test.py:271:5:271:19 | ControlFlowNode for Attribute() | this operation | | resources_test.py:285:11:285:20 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:287:5:287:31 | ControlFlowNode for Attribute() | this operation | +testFailures +| resources_test.py:154:26:154:154 | Comment # $SPURIOUS: Alert # Dataflow appears to not detect this with statement as guarding the exceptions produced by the `read()` call. | Fixed spurious result: Alert | 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 0f9edc02803b..972cc3a7be29 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py @@ -293,7 +293,7 @@ def __init__(self, fp): self.data = fp.read() fp.close() - def do_something(): + def do_something(self): pass def closed30(path): @@ -322,7 +322,7 @@ def read(self): return self.f.read() def __enter__(self): pass - def __exit__(self): + def __exit__(self,exc_type, exc_value,traceback): self.f.close() def closed32(path): From ec40ea800d37886764f27b2af3f633f9675e3471 Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 9 Sep 2025 13:46:52 +0100 Subject: [PATCH 5/6] Update qldoc --- python/ql/src/Resources/FileNotAlwaysClosedQuery.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll index 36807ca39f1a..9d91e4f523c2 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll +++ b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll @@ -52,7 +52,7 @@ class FileWrapperCall extends DataFlow::CallCfgNode { /** A node where a file is closed. */ abstract class FileClose extends DataFlow::CfgNode { - /** Holds if this file close will occur if an exception is raised at `raises`. */ + /** Holds if this file close will occur if an exception is raised at `fileRaises`. */ predicate guardsExceptions(DataFlow::CfgNode fileRaises) { // The close call occurs after an exception edge in the cfg (a catch or finally) bbReachableRefl(fileRaises.asCfgNode().getBasicBlock().getAnExceptionalSuccessor(), From ea562de3e6fa57ea279729b8cfc9f7d68e9530fe Mon Sep 17 00:00:00 2001 From: Joe Farebrother Date: Tue, 9 Sep 2025 15:17:16 +0100 Subject: [PATCH 6/6] Fix tests --- .../Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected | 3 --- .../Resources/FileNotAlwaysClosed/resources_test.py | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected index 579424e9318f..7f48feb72eb9 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected @@ -1,4 +1,3 @@ -#select | resources_test.py:4:10:4:25 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:5:5:5:33 | ControlFlowNode for Attribute() | this operation | | resources_test.py:9:10:9:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:108:11:108:20 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | @@ -8,5 +7,3 @@ | resources_test.py:248:11:248:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:269:10:269:27 | ControlFlowNode for Attribute() | File may not be closed if $@ raises an exception. | resources_test.py:271:5:271:19 | ControlFlowNode for Attribute() | this operation | | resources_test.py:285:11:285:20 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:287:5:287:31 | ControlFlowNode for Attribute() | this operation | -testFailures -| resources_test.py:154:26:154:154 | Comment # $SPURIOUS: Alert # Dataflow appears to not detect this with statement as guarding the exceptions produced by the `read()` call. | Fixed spurious result: Alert | 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 972cc3a7be29..15ba9393715a 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py @@ -151,7 +151,7 @@ def not_closed17(): #With statement will close the fp def closed18(path): try: - f18 = open(path) # $Alert + f18 = open(path) except IOError as ex: print(ex) raise ex