Fix FP for py/file-not-closed

Author: owen-mc

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`. */

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 |

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

@@ -350,14 +350,14 @@ def close(self):
         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()