Refactor ReExposedInstance logic into one place

Author: owen-mc

python/ql/lib/semmle/python/dataflow/new/internal/ReExposedInstance.qll

@@ -0,0 +1,46 @@
+/**
+ * Provides a parameterized module for identifying values that instance-attribute type tracking
+ * has re-exposed (laundered) out of an instance attribute, rather than freshly created.
+ */
+
+private import python
+private import semmle.python.dataflow.new.DataFlow
+
+/** Holds if `node` should be treated as an instance source. */
+signature predicate instanceNodeSig(DataFlow::Node node);
+
+/**
+ * Provides a predicate for identifying values that instance-attribute type tracking has
+ * re-exposed (laundered) out of an instance attribute, rather than freshly created.
+ *
+ * Instance-attribute type tracking can flow a value into an instance attribute and back out at
+ * a later attribute read, for example `BufferedRWPair.reader` or `FileIO.fileno` returning
+ * `self._fd`. Such a re-exposed value is owned by the enclosing instance and is not a fresh
+ * resource; queries that reason about resource creation or lifetime should not treat it as one.
+ *
+ * The parameter `isInstance` defines which nodes count as instance sources (typically the result
+ * of a class- or resource-instance type tracker).
+ */
+module ReExposedInstance<instanceNodeSig/1 isInstance> {
+  /**
+   * Holds if `read` is an attribute read that re-exposes an instance held in an instance
+   * attribute.
+   */
+  private predicate launderedAttrRead(DataFlow::AttrRead read) { isInstance(read) }
+
+  /** Type tracking forward from an attribute read that re-exposes an instance held in a field. */
+  private DataFlow::TypeTrackingNode launderedInstance(DataFlow::TypeTracker t) {
+    t.start() and
+    launderedAttrRead(result)
+    or
+    exists(DataFlow::TypeTracker t2 | result = launderedInstance(t2).track(t2, t))
+  }
+
+  /**
+   * Holds if `node` is a value that has been re-exposed (laundered) out of an instance attribute,
+   * rather than being a freshly created instance.
+   */
+  predicate isReExposed(DataFlow::Node node) {
+    launderedInstance(DataFlow::TypeTracker::end()).flowsTo(node)
+  }
+}

python/ql/src/Resources/FileNotAlwaysClosedQuery.qll

@@ -3,6 +3,7 @@
 import python
 import semmle.python.dataflow.new.internal.DataFlowDispatch
 import semmle.python.ApiGraphs
+private import semmle.python.dataflow.new.internal.ReExposedInstance
 
 /** A CFG node where a file is opened. */
 abstract class FileOpenSource extends DataFlow::CfgNode { }
@@ -22,24 +23,13 @@ private DataFlow::TypeTrackingNode fileOpenInstance(DataFlow::TypeTracker 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.
+ * Holds if `node` is tracked to be an instance of an open file object.
  */
-private predicate launderedAttrRead(DataFlow::AttrRead read) {
-  fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(read)
+private predicate fileInstanceNode(DataFlow::Node node) {
+  fileOpenInstance(DataFlow::TypeTracker::end()).flowsTo(node)
 }
 
-/** 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))
-}
+private module FileReExposed = ReExposedInstance<fileInstanceNode/1>;
 
 /**
  * A call that returns an instance of an open file object.
@@ -52,7 +42,7 @@ class FileOpen extends DataFlow::CallCfgNode {
     // (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)
+    not FileReExposed::isReExposed(this)
   }
 }
 

python/ql/src/Statements/ShouldUseWithStatement.ql

@@ -15,6 +15,7 @@
 import python
 private import semmle.python.dataflow.new.DataFlow
 private import semmle.python.dataflow.new.internal.DataFlowDispatch
+private import semmle.python.dataflow.new.internal.ReExposedInstance
 
 predicate calls_close(Call c) { exists(Attribute a | c.getFunc() = a and a.getName() = "close") }
 
@@ -24,26 +25,10 @@ predicate only_stmt_in_finally(Try t, Call c) {
   )
 }
 
-/**
- * Holds if `read` is an attribute read that re-exposes an instance of `cls` held in an
- * instance attribute, for example `BufferedRWPair.reader`.
- *
- * Instance-attribute type tracking can launder such an instance out of a field. The object
- * is owned by the enclosing instance, so its lifetime spans that instance and cannot be
- * expressed with a `with` statement; closing it in a `finally` block is therefore not a
- * candidate for refactoring.
- */
-private predicate launderedAttrRead(Class cls, DataFlow::AttrRead read) {
-  read = classInstanceTracker(cls)
-}
+/** Holds if `node` is tracked to be an instance of some class. */
+private predicate classInstanceNode(DataFlow::Node node) { node = classInstanceTracker(_) }
 
-/** Type tracking forward from an attribute read that re-exposes an instance held in a field. */
-private DataFlow::TypeTrackingNode launderedInstance(Class cls, DataFlow::TypeTracker t) {
-  t.start() and
-  launderedAttrRead(cls, result)
-  or
-  exists(DataFlow::TypeTracker t2 | result = launderedInstance(cls, t2).track(t2, t))
-}
+private module ClassReExposed = ReExposedInstance<classInstanceNode/1>;
 
 from Call close, Try t, Class cls, DataFlow::Node closeTarget
 where
@@ -54,7 +39,7 @@ where
   // Don't report closing a resource that is held in an instance attribute (e.g. `self.reader`).
   // Such flow is introduced by instance-attribute type tracking; the object's lifetime is tied
   // to the enclosing instance and cannot be expressed with a `with` statement.
-  not launderedInstance(cls, DataFlow::TypeTracker::end()).flowsTo(closeTarget) and
+  not ClassReExposed::isReExposed(closeTarget) and
   DuckTyping::isContextManager(cls)
 select close,
   "Instance of context-manager class $@ is closed in a finally block. Consider using 'with' statement.",