From ea7510bf724d45688ebb7471696622703e653b6c Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Wed, 17 Jun 2026 13:10:47 +0100 Subject: [PATCH] Refactor ReExposedInstance logic into one place --- .../new/internal/ReExposedInstance.qll | 46 +++++++++++++++++++ .../Resources/FileNotAlwaysClosedQuery.qll | 22 +++------ .../src/Statements/ShouldUseWithStatement.ql | 25 ++-------- 3 files changed, 57 insertions(+), 36 deletions(-) create mode 100644 python/ql/lib/semmle/python/dataflow/new/internal/ReExposedInstance.qll diff --git a/python/ql/lib/semmle/python/dataflow/new/internal/ReExposedInstance.qll b/python/ql/lib/semmle/python/dataflow/new/internal/ReExposedInstance.qll new file mode 100644 index 00000000000..b080610f512 --- /dev/null +++ b/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 { + /** + * 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) + } +} diff --git a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll b/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll index bda0ee5dbdc..2dfda7044d9 100644 --- a/python/ql/src/Resources/FileNotAlwaysClosedQuery.qll +++ b/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; /** * 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) } } diff --git a/python/ql/src/Statements/ShouldUseWithStatement.ql b/python/ql/src/Statements/ShouldUseWithStatement.ql index 3536a017d2b..8ead51fa6b3 100644 --- a/python/ql/src/Statements/ShouldUseWithStatement.ql +++ b/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; 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.",