gh-151613: Fix remote debugging frame cache ABA

pablogsal · pablogsal · commit 000eedb4b9e2 · 2026-06-17T22:21:21.000+01:00
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/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,16 @@ frame_cache_lookup_and_extend(
         return 0;  // Not found
     }
     assert(start_idx < entry->num_addrs);
+    if (entry->last_profiled_frame_seq + (uintptr_t)start_idx !=
+        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 +251,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 +291,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 +334,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
