From 199fd864add1babc4c5e8196b7f5b619d0f80fdd Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 17 Jun 2026 12:36:04 +0100 Subject: [PATCH] Fix FP for py/file-not-closed --- .../Resources/FileNotAlwaysClosedQuery.qll | 29 ++++++++++++++++++- .../FileNotAlwaysClosed.expected | 1 - .../FileNotAlwaysClosed/resources_test.py | 10 +++---- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll index 9d91e4f523c..bda0ee5dbdc 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll +++ b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll @@ -21,12 +21,39 @@ private DataFlow::TypeTrackingNode fileOpenInstance(DataFlow::TypeTracker t) { exists(DataFlow::TypeTracker t2 | result = fileOpenInstance(t2).track(t2, t)) } +/** + * Holds if `read` is an attribute read that re-exposes an already-open file held in an + * instance attribute, for example `FileIO.fileno` returning `self._fd`. + * + * Instance-attribute type tracking can launder an open file out of such an accessor, which + * would otherwise be mistaken for a fresh file open. The underlying open is tracked, and its + * lifetime handled, separately at its real creation site. + */ +private predicate launderedAttrRead(DataFlow::AttrRead read) { + fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(read) +} + +/** Type tracking forward from an attribute read that re-exposes a file held in a field. */ +private DataFlow::TypeTrackingNode launderedFileInstance(DataFlow::TypeTracker t) { + t.start() and + launderedAttrRead(result) + or + exists(DataFlow::TypeTracker t2 | result = launderedFileInstance(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) } + FileOpen() { + fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(this) and + // Don't treat an accessor that merely re-exposes a file held in an instance attribute + // (e.g. `FileIO.fileno` returning `self._fd`) as opening a new file. Such flow is + // introduced by instance-attribute type tracking; the underlying open is tracked at its + // real creation site. + not launderedFileInstance(DataFlow::TypeTracker::end()).flowsTo(this) + } } /** A call that may wrap a file object in a wrapper class or `os.fdopen`. */ diff --git a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected index 6f17eab01aa..b7d9d37785b 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/FileNotAlwaysClosed.expected @@ -7,4 +7,3 @@ | resources_test.py:250:11:250:25 | ControlFlowNode for open() | File is opened but is not closed. | file://:0:0:0:0 | (none) | this operation | | resources_test.py:271:10:271:27 | ControlFlowNode for Attribute() | File may not be closed if $@ raises an exception. | resources_test.py:273:5:273:19 | ControlFlowNode for Attribute() | this operation | | resources_test.py:287:11:287:20 | ControlFlowNode for open() | File may not be closed if $@ raises an exception. | resources_test.py:289:5:289:31 | ControlFlowNode for Attribute() | this operation | -| resources_test.py:361:13:361:27 | ControlFlowNode for Attribute() | File is opened but is not closed. | file://:0:0:0:0 | (none) | 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 4f9b067d773..57568a6eb11 100644 --- a/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py +++ b/python/ql/test/query-tests/Resources/FileNotAlwaysClosed/resources_test.py @@ -350,14 +350,14 @@ class FdHolder33(): os.close(self._fd) def closed33(path): - # False positive mirroring CPython's `_pyio.open`. + # Regression test mirroring CPython's `_pyio.open`. # `holder.fileno()` merely returns the existing file descriptor; it does not # open a new resource. With instance-attribute type tracking, the `os.open` - # source flows through `self._fd` and back out of `fileno()`, so the call - # `holder.fileno()` is wrongly treated as a fresh file-open whose result is - # never closed. The descriptor is in fact owned and closed by `holder.close()`. + # source flows through `self._fd` and back out of `fileno()`. The query must + # not treat that re-exposed descriptor as a fresh file-open whose result is + # never closed. The descriptor is owned and closed by `holder.close()`. holder = FdHolder33(path) try: - n = holder.fileno() # $ SPURIOUS: Alert + n = holder.fileno() # No alert: this re-exposes an existing descriptor, not a new open. finally: holder.close()