gh-151613: Fix remote debugging frame cache ABA

pablogsal · pablogsal · commit a548b2404097 · 2026-06-17T22:46:28.000+01:00
Fixes #151613. The remote debugging frame cache previously used only the last_profiled_frame address as its cache anchor. If a frame returned and a later frame reused the same _PyInterpreterFrame address, the profiler could accept a stale cache entry and splice parent frames from a different call chain into the current stack. This adds a last_profiled_frame_seq counter next to last_profiled_frame, increments it when the anchor advances, stores it in frame cache entries, and validates cache hits against both the frame address and the sequence. Cache miss walks now copy stack chunks before storing new cache entries so stored continuations come from a stable snapshot. The new regression test exercises alternating call chains and checks that cached stacks never contain frames from both branches.
diff --git a/Include/cpython/pystate.h b/Include/cpython/pystate.h
@@ -143,6 +143,7 @@ struct _ts {
     struct _PyInterpreterFrame *base_frame;
 
     struct _PyInterpreterFrame *last_profiled_frame;
+    uintptr_t last_profiled_frame_seq;
 
     Py_tracefunc c_profilefunc;
     Py_tracefunc c_tracefunc;
diff --git a/Include/internal/pycore_debug_offsets.h b/Include/internal/pycore_debug_offsets.h
@@ -104,6 +104,7 @@ typedef struct _Py_DebugOffsets {
         uint64_t current_frame;
         uint64_t base_frame;
         uint64_t last_profiled_frame;
+        uint64_t last_profiled_frame_seq;
         uint64_t thread_id;
         uint64_t native_thread_id;
         uint64_t datastack_chunk;
@@ -294,6 +295,7 @@ typedef struct _Py_DebugOffsets {
         .current_frame = offsetof(PyThreadState, current_frame), \
         .base_frame = offsetof(PyThreadState, base_frame), \
         .last_profiled_frame = offsetof(PyThreadState, last_profiled_frame), \
+        .last_profiled_frame_seq = offsetof(PyThreadState, last_profiled_frame_seq), \
         .thread_id = offsetof(PyThreadState, thread_id), \
         .native_thread_id = offsetof(PyThreadState, native_thread_id), \
         .datastack_chunk = offsetof(PyThreadState, datastack_chunk), \
diff --git a/Include/internal/pycore_interpframe.h b/Include/internal/pycore_interpframe.h
@@ -292,12 +292,15 @@ _PyThreadState_GetFrame(PyThreadState *tstate)
 // This avoids corrupting the cache when transient frames (called and returned
 // between profiler samples) update last_profiled_frame to addresses the
 // profiler never saw.
+// The sequence distinguishes this anchor from a later frame that reuses the
+// same _PyInterpreterFrame address.
 #define _PyThreadState_UpdateLastProfiledFrame(tstate, frame, previous) \
     do { \
         PyThreadState *tstate_ = (tstate); \
         _PyInterpreterFrame *frame_ = (frame); \
         if (tstate_->last_profiled_frame == frame_) { \
             tstate_->last_profiled_frame = (previous); \
+            tstate_->last_profiled_frame_seq++; \
         } \
     } while (0)
 
diff --git a/Lib/test/test_external_inspection.py b/Lib/test/test_external_inspection.py
@@ -3451,6 +3451,88 @@ def level1():
                 # Parent frames: must match exactly
                 self.assertEqual(loc_cached, loc_no_cache)
 
+    @skip_if_not_supported
+    @unittest.skipIf(
+        sys.platform != "linux" or not PROCESS_VM_READV_SUPPORTED,
+        "Test only runs on Linux with process_vm_readv support",
+    )
+    def test_cache_rejects_reused_frame_address_aba(self):
+        """Test that frame address reuse cannot splice cached parent frames."""
+        script_body = """\
+            sock.sendall(b"ready")
+
+            def burn_a():
+                total = 0
+                for i in range(20000):
+                    total += i
+                return total
+
+            def burn_b():
+                total = 0
+                for i in range(20000):
+                    total += i
+                return total
+
+            def a_leaf():
+                return burn_a()
+
+            def b_leaf():
+                return burn_b()
+
+            def a_parent():
+                return a_leaf()
+
+            def b_parent():
+                return b_leaf()
+
+            while True:
+                a_parent()
+                b_parent()
+            """
+
+        with self._target_process(script_body) as (p, client_socket, _):
+            _wait_for_signal(client_socket, b"ready")
+            unwinder = RemoteUnwinder(
+                p.pid, all_threads=True, cache_frames=True, stats=True
+            )
+            bad_stacks = []
+            seen = 0
+            branch_a = {"a_parent", "a_leaf", "burn_a"}
+            branch_b = {"b_parent", "b_leaf", "burn_b"}
+
+            for _ in range(8000):
+                with contextlib.suppress(*TRANSIENT_ERRORS):
+                    traces = unwinder.get_stack_trace()
+                    for interp in traces:
+                        for thread in interp.threads:
+                            in_a = False
+                            in_b = False
+                            for frame in thread.frame_info:
+                                name = frame.funcname
+                                if name in branch_a:
+                                    in_a = True
+                                elif name in branch_b:
+                                    in_b = True
+                                if in_a and in_b:
+                                    break
+                            if not (in_a or in_b):
+                                continue
+                            seen += 1
+                            if in_a and in_b:
+                                funcs = [f.funcname for f in thread.frame_info]
+                                bad_stacks.append(funcs)
+                    if len(bad_stacks) >= 3:
+                        break
+
+            stats = unwinder.get_stats()
+
+        self.assertGreater(seen, 0)
+        self.assertGreater(
+            stats["frame_cache_hits"] + stats["frame_cache_partial_hits"], 0
+        )
+        self.assertGreater(stats["frames_read_from_cache"], 0)
+        self.assertEqual(bad_stacks, [])
+
     @skip_if_not_supported
     @unittest.skipIf(
         sys.platform == "linux" and not PROCESS_VM_READV_SUPPORTED,
diff --git a/Misc/NEWS.d/next/Library/2026-06-13-11-57-48.gh-issue-151436.UEDowO.rst b/Misc/NEWS.d/next/Library/2026-06-13-11-57-48.gh-issue-151436.UEDowO.rst
@@ -1,4 +1,4 @@
-Fix skewed stack trackes in the Tachyon profiler when caching is enabled and
+Fix skewed stack traces in the Tachyon profiler when caching is enabled and
 when generators and coroutines are profiled, by updating
 ``tstate->last_profiled_frame`` at every frame-removal site. The issue resulted
 in total erasure of some callers. Patch by Maurycy Pawłowski-Wieroński.
diff --git a/Misc/NEWS.d/next/Library/2026-06-17-22-31-57.gh-issue-151613.n0nua1.rst b/Misc/NEWS.d/next/Library/2026-06-17-22-31-57.gh-issue-151613.n0nua1.rst
@@ -0,0 +1,3 @@
+Fix another way the Tachyon profiler frame cache could produce impossible
+mixed stack traces when ``_PyInterpreterFrame`` addresses are reused, by
+validating cached frame anchors with a sequence counter.
diff --git a/Modules/_remote_debugging/_remote_debugging.h b/Modules/_remote_debugging/_remote_debugging.h
@@ -228,6 +228,7 @@ typedef struct {
 typedef struct {
     uint64_t thread_id;                      // 0 = empty slot
     uintptr_t thread_state_addr;
+    uintptr_t last_profiled_frame_seq;
     uintptr_t addrs[FRAME_CACHE_MAX_FRAMES];
     Py_ssize_t num_addrs;
     PyObject *thread_id_obj;                 // owned reference, NULL if empty
@@ -435,6 +436,7 @@ typedef struct {
     uintptr_t base_frame_addr;      // Sentinel at bottom (for validation)
     uintptr_t gc_frame;             // GC frame address (0 if not tracking)
     uintptr_t last_profiled_frame;  // Last cached frame (0 if no cache)
+    uintptr_t last_profiled_frame_seq;
     StackChunkList *chunks;         // Pre-copied stack chunks
     int skip_first_frame;           // Skip frame_addr itself (continue from its caller)
     RemoteReadPrefetch prefetch;     // Optional already-read thread/frame buffers
@@ -626,11 +628,18 @@ extern void frame_cache_invalidate_stale(RemoteUnwinderObject *unwinder, PyObjec
 extern int frame_cache_lookup_and_extend(
     RemoteUnwinderObject *unwinder,
     uint64_t thread_id,
+    uintptr_t thread_state_addr,
     uintptr_t last_profiled_frame,
+    uintptr_t last_profiled_frame_seq,
     PyObject *frame_info,
     uintptr_t *frame_addrs,
     Py_ssize_t *num_addrs,
     Py_ssize_t max_addrs);
+extern int frame_cache_anchor_matches(
+    RemoteUnwinderObject *unwinder,
+    uintptr_t thread_state_addr,
+    uintptr_t last_profiled_frame,
+    uintptr_t last_profiled_frame_seq);
 // Returns: 1 = stored, 0 = not stored (graceful), -1 = error
 // Only stores complete stacks that reach base_frame_addr
 extern int frame_cache_store(
@@ -640,6 +649,7 @@ extern int frame_cache_store(
     const uintptr_t *addrs,
     Py_ssize_t num_addrs,
     uintptr_t thread_state_addr,
+    uintptr_t last_profiled_frame_seq,
     uintptr_t base_frame_addr,
     uintptr_t last_frame_visited);
 
diff --git a/Modules/_remote_debugging/debug_offsets_validation.h b/Modules/_remote_debugging/debug_offsets_validation.h
@@ -31,7 +31,7 @@
 #define FIELD_SIZE(type, member) sizeof(((type *)0)->member)
 
 enum {
-    PY_REMOTE_DEBUG_OFFSETS_TOTAL_SIZE = 880,
+    PY_REMOTE_DEBUG_OFFSETS_TOTAL_SIZE = 888,
     PY_REMOTE_ASYNC_DEBUG_OFFSETS_TOTAL_SIZE = 104,
 };
 
@@ -261,7 +261,8 @@ validate_fixed_field(
     APPLY(thread_state, next, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
     APPLY(thread_state, current_frame, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
     APPLY(thread_state, base_frame, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
-    APPLY(thread_state, last_profiled_frame, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size)
+    APPLY(thread_state, last_profiled_frame, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size); \
+    APPLY(thread_state, last_profiled_frame_seq, sizeof(uintptr_t), _Alignof(uintptr_t), buffer_size)
 
 #define PY_REMOTE_DEBUG_INTERPRETER_STATE_FIELDS(APPLY, buffer_size) \
     APPLY(interpreter_state, id, sizeof(int64_t), _Alignof(int64_t), buffer_size); \
diff --git a/Modules/_remote_debugging/frame_cache.c b/Modules/_remote_debugging/frame_cache.c
@@ -147,19 +147,59 @@ frame_cache_invalidate_stale(RemoteUnwinderObject *unwinder, PyObject *result)
             Py_CLEAR(unwinder->frame_cache[i].frame_list);
             unwinder->frame_cache[i].thread_id = 0;
             unwinder->frame_cache[i].thread_state_addr = 0;
+            unwinder->frame_cache[i].last_profiled_frame_seq = 0;
             unwinder->frame_cache[i].num_addrs = 0;
             STATS_INC(unwinder, stale_cache_invalidations);
         }
     }
 }
 
+int
+frame_cache_anchor_matches(
+    RemoteUnwinderObject *unwinder,
+    uintptr_t thread_state_addr,
+    uintptr_t last_profiled_frame,
+    uintptr_t last_profiled_frame_seq)
+{
+    uintptr_t live_frame = 0;
+    uintptr_t live_seq = 0;
+    uintptr_t frame_offset = (uintptr_t)unwinder->debug_offsets.thread_state.last_profiled_frame;
+    uintptr_t seq_offset = (uintptr_t)unwinder->debug_offsets.thread_state.last_profiled_frame_seq;
+
+    if (seq_offset == frame_offset + sizeof(uintptr_t)) {
+        uintptr_t live_anchor[2];
+        if (_Py_RemoteDebug_PagedReadRemoteMemory(&unwinder->handle,
+                                                  thread_state_addr + frame_offset,
+                                                  sizeof(live_anchor),
+                                                  live_anchor) < 0) {
+            PyErr_Clear();
+            return 0;
+        }
+        live_frame = live_anchor[0];
+        live_seq = live_anchor[1];
+    }
+    else {
+        if (read_ptr(unwinder, thread_state_addr + frame_offset, &live_frame) < 0) {
+            PyErr_Clear();
+            return 0;
+        }
+        if (read_ptr(unwinder, thread_state_addr + seq_offset, &live_seq) < 0) {
+            PyErr_Clear();
+            return 0;
+        }
+    }
+    return live_frame == last_profiled_frame && live_seq == last_profiled_frame_seq;
+}
+
 // Find last_profiled_frame in cache and extend frame_info with cached continuation
 // If frame_addrs is provided (not NULL), also extends it with cached addresses
 int
 frame_cache_lookup_and_extend(
     RemoteUnwinderObject *unwinder,
     uint64_t thread_id,
+    uintptr_t thread_state_addr,
     uintptr_t last_profiled_frame,
+    uintptr_t last_profiled_frame_seq,
     PyObject *frame_info,
     uintptr_t *frame_addrs,
     Py_ssize_t *num_addrs,
@@ -173,6 +213,9 @@ frame_cache_lookup_and_extend(
     if (!entry || !entry->frame_list) {
         return 0;
     }
+    if (entry->thread_state_addr != thread_state_addr) {
+        return 0;
+    }
 
     assert(entry->num_addrs >= 0 && entry->num_addrs <= FRAME_CACHE_MAX_FRAMES);
 
@@ -189,8 +232,24 @@ frame_cache_lookup_and_extend(
         return 0;  // Not found
     }
     assert(start_idx < entry->num_addrs);
+    // Synthetic marker frames (<native>/<GC>) are stored as addr-0 entries but
+    // never increment last_profiled_frame_seq in the target (only real frame
+    // pops do). Count the real frames before start_idx so the sequence check is
+    // not thrown off by markers sitting between the leaf and the anchor.
+    uintptr_t real_pops = 0;
+    for (Py_ssize_t i = 0; i < start_idx; i++) {
+        if (entry->addrs[i] != 0) {
+            real_pops++;
+        }
+    }
+    if (entry->last_profiled_frame_seq + real_pops != last_profiled_frame_seq) {
+        return 0;
+    }
 
     Py_ssize_t num_frames = PyList_GET_SIZE(entry->frame_list);
+    if (start_idx >= num_frames) {
+        return 0;
+    }
 
     // Extend frame_info with frames ABOVE start_idx (not including it).
     // The frame at start_idx (last_profiled_frame) was the executing frame
@@ -200,6 +259,11 @@ frame_cache_lookup_and_extend(
     if (cache_start >= num_frames) {
         return 0;  // Nothing above last_profiled_frame to extend with
     }
+    if (!frame_cache_anchor_matches(unwinder, thread_state_addr,
+                                    last_profiled_frame,
+                                    last_profiled_frame_seq)) {
+        return 0;
+    }
 
     PyObject *slice = PyList_GetSlice(entry->frame_list, cache_start, num_frames);
     if (!slice) {
@@ -235,6 +299,7 @@ frame_cache_store(
     const uintptr_t *addrs,
     Py_ssize_t num_addrs,
     uintptr_t thread_state_addr,
+    uintptr_t last_profiled_frame_seq,
     uintptr_t base_frame_addr,
     uintptr_t last_frame_visited)
 {
@@ -277,6 +342,7 @@ frame_cache_store(
     }
     entry->thread_id = thread_id;
     entry->thread_state_addr = thread_state_addr;
+    entry->last_profiled_frame_seq = last_profiled_frame_seq;
     if (entry->thread_id_obj == NULL) {
         entry->thread_id_obj = PyLong_FromUnsignedLongLong(thread_id);
         if (entry->thread_id_obj == NULL) {
diff --git a/Modules/_remote_debugging/frames.c b/Modules/_remote_debugging/frames.c
diff --git a/Modules/_remote_debugging/threads.c b/Modules/_remote_debugging/threads.c
diff --git a/Python/pystate.c b/Python/pystate.c

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+Fix another way the Tachyon profiler frame cache could produce impossible`
	`2`	+mixed stack traces when ``_PyInterpreterFrame`` addresses are reused, by
	`3`	`+validating cached frame anchors with a sequence counter.`