Model instance-attribute flow as a Ruby/JS-style field level step (any method)

Author: Copilot

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

@@ -172,6 +172,8 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
   /** Holds if there is a level step from `nodeFrom` to `nodeTo`, which does not depend on the call graph. */
   predicate levelStepNoCall(Node nodeFrom, LocalSourceNode nodeTo) {
     TypeTrackerSummaryFlow::levelStepNoCall(nodeFrom, nodeTo)
+    or
+    localFieldStep(nodeFrom, nodeTo)
   }
 
   /**
@@ -318,36 +320,47 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
   }
 
   /**
-   * Holds if `nodeFrom` is the `self` parameter of an `__init__` method and `nodeTo` is
-   * the `self` parameter of another method on the same class.
+   * Holds if `ref` accesses attribute `attr` of `self`, where `self` is the first
+   * parameter of an instance method of `cls` (i.e. an access of the form `self.attr`).
    *
-   * This models flow through instance attributes (`self.foo`): a value stored into
-   * `self.foo` inside `__init__` can be read from `self.foo` in another method.
-   * Type-tracking handles the store and read steps via `AttrWrite`/`AttrRead`, but on its
-   * own it cannot relate the `self` parameter of `__init__` (where the store happens) to
-   * the `self` parameter of the reading method.
+   * Static methods and class methods are excluded, since their first parameter is not a
+   * `self` instance reference.
+   */
+  private predicate selfAttrRef(Class cls, string attr, DataFlowPublic::AttrRef ref) {
+    exists(Function method, Name selfUse |
+      method = cls.getAMethod() and
+      not DataFlowDispatch::isStaticmethod(method) and
+      not DataFlowDispatch::isClassmethod(method) and
+      selfUse.getVariable() = method.getArg(0).(Name).getVariable() and
+      ref.getObject().asCfgNode().getNode() = selfUse and
+      ref.mayHaveAttributeName(attr)
+    )
+  }
+
+  /**
+   * Holds if `nodeFrom` is written to attribute `self.attr` in some instance method of a
+   * class, and `nodeTo` reads attribute `self.attr` in some (possibly different) instance
+   * method of the same class.
    *
-   * We restrict the source to `self` in `__init__` (rather than allowing flow between the
-   * `self` parameters of arbitrary method pairs) because `__init__` is guaranteed to run
-   * before any other method of the instance is used. This keeps the over-approximation
-   * directional -- attributes assigned during construction flow to later method bodies,
-   * but we do not pretend that an attribute assigned in one ordinary method is visible in
-   * another (where there is no such ordering guarantee).
+   * This models flow through instance attributes (`self.foo`): a value stored into
+   * `self.foo` in one method can be read from `self.foo` in another method. Type-tracking
+   * handles the store and read steps via `AttrWrite`/`AttrRead`, but on its own it cannot
+   * relate the `self` of the writing method to the `self` of the reading method. Following
+   * the approach used for Ruby and JavaScript, we model this directly as a level step from
+   * the written value to the read reference, for any pair of methods on the class (not
+   * just from `__init__`).
    *
-   * This is still instance-insensitive (it does not distinguish between different
-   * instances of the same class), matching the precision of instance-attribute handling
-   * elsewhere.
+   * This is an over-approximation: it is instance-insensitive (it does not distinguish
+   * between different instances of the same class) and order-insensitive (it does not
+   * require the write to happen before the read), matching the precision of
+   * instance-attribute handling for Ruby and JavaScript.
    */
-  private predicate initSelfJumpStep(Node nodeFrom, LocalSourceNode nodeTo) {
-    exists(Class cls, Function initMethod, Function otherMethod |
-      initMethod = cls.getAMethod() and
-      initMethod.getName() = "__init__" and
-      otherMethod = cls.getAMethod() and
-      otherMethod != initMethod and
-      not DataFlowDispatch::isStaticmethod(otherMethod) and
-      not DataFlowDispatch::isClassmethod(otherMethod) and
-      nodeFrom.asExpr() = initMethod.getArg(0) and
-      nodeTo.asExpr() = otherMethod.getArg(0)
+  private predicate localFieldStep(Node nodeFrom, LocalSourceNode nodeTo) {
+    exists(Class cls, string attr, DataFlowPublic::AttrWrite write, DataFlowPublic::AttrRead read |
+      selfAttrRef(cls, attr, write) and
+      nodeFrom = write.getValue() and
+      selfAttrRef(cls, attr, read) and
+      nodeTo = read
     )
   }
 
@@ -358,8 +371,6 @@ module TypeTrackingInput implements Shared::TypeTrackingInput<Location> {
     DataFlowPrivate::jumpStepSharedWithTypeTracker(nodeFrom, nodeTo)
     or
     capturedJumpStep(nodeFrom, nodeTo)
-    or
-    initSelfJumpStep(nodeFrom, nodeTo)
   }
 
   predicate hasFeatureBacktrackStoreTarget() { any() }

python/ql/test/library-tests/dataflow/typetracking/attribute_tests.py

@@ -150,11 +150,11 @@ class MyClass2(object):
     def __init__(self): # $ tracked=foo
         self.foo = tracked # $ tracked=foo tracked
 
-    def print_foo(self): # $ tracked=foo
-        print(self.foo) # $ tracked=foo tracked
+    def print_foo(self): # $ MISSING: tracked=foo
+        print(self.foo) # $ tracked MISSING: tracked=foo
 
-    def possibly_uncalled_method(self): # $ tracked=foo
-        print(self.foo) # $ tracked=foo tracked
+    def possibly_uncalled_method(self): # $ MISSING: tracked=foo
+        print(self.foo) # $ tracked MISSING: tracked=foo
 
 instance = MyClass2()
 print(instance.foo) # $ MISSING: tracked=foo tracked

python/ql/test/library-tests/frameworks/hdbcli/pep249.py

@@ -11,10 +11,10 @@
 
 # Connection stored in a class attribute (`self._conn`) and used in another method.
 #
-# This is detected because type tracking includes a jump step from the `self` parameter of
-# `__init__` to the `self` parameter of the other methods on the class, which lets the
-# connection stored into `self._conn` during construction be read back from `self._conn`
-# (directly or via a getter) in the other methods.
+# This is detected because type tracking includes a level step modelling flow through
+# instance attributes: a value written to `self._conn` in one method (here `__init__`) can
+# be read back from `self._conn` (directly or via a getter) in any other method on the same
+# class. This follows the same approach used for instance fields in Ruby and JavaScript.
 class Database:
     def __init__(self):
         self._conn = dbapi.connect(address="hostname", port=300, user="username")