Fix FP for py/should-use-with

Author: owen-mc

python/ql/src/Statements/ShouldUseWithStatement.ql

@@ -13,6 +13,7 @@
  */
 
 import python
+private import semmle.python.dataflow.new.DataFlow
 private import semmle.python.dataflow.new.internal.DataFlowDispatch
 
 predicate calls_close(Call c) { exists(Attribute a | c.getFunc() = a and a.getName() = "close") }
@@ -23,11 +24,37 @@ predicate only_stmt_in_finally(Try t, Call c) {
   )
 }
 
-from Call close, Try t, Class cls
+/**
+ * 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)
+}
+
+/** 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))
+}
+
+from Call close, Try t, Class cls, DataFlow::Node closeTarget
 where
   only_stmt_in_finally(t, close) and
   calls_close(close) and
-  classInstanceTracker(cls).asExpr() = close.getFunc().(Attribute).getObject() and
+  closeTarget.asExpr() = close.getFunc().(Attribute).getObject() and
+  closeTarget = classInstanceTracker(cls) and
+  // 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
   DuckTyping::isContextManager(cls)
 select close,
   "Instance of context-manager class $@ is closed in a finally block. Consider using 'with' statement.",

python/ql/test/query-tests/Statements/general/ShouldUseWithStatement.expected

@@ -1,2 +1 @@
 | test.py:168:9:168:17 | Attribute() | Instance of context-manager class $@ is closed in a finally block. Consider using 'with' statement. | test.py:151:1:151:17 | Class CM | CM |
-| test.py:182:13:182:26 | Attribute() | Instance of context-manager class $@ is closed in a finally block. Consider using 'with' statement. | test.py:151:1:151:17 | Class CM | CM |

python/ql/test/query-tests/Statements/general/test.py

@@ -167,8 +167,10 @@ def no_with():
     finally:
         f.close()
 
-# Should use context manager, with the resource held in an instance attribute
-# (caught via instance-attribute type tracking).
+# Should not use a 'with' statement here: the resource is held in an instance
+# attribute, so its lifetime spans the enclosing instance and cannot be expressed
+# with a 'with' statement. Instance-attribute type tracking can launder the
+# instance out of the field, but this must not be reported.
 class HoldsCM(object):
 
     def __init__(self):
@@ -179,7 +181,7 @@ def no_with_attribute(self):
             self.f.write("Hello ")
             self.f.write(" World\n")
         finally:
-            self.f.close()
+            self.f.close()  # No alert: re-exposes a field, not a local resource.
 
 #Assert without side-effect
 def assert_ok(seq):