gh-146308: Fix error handling issues in _remote_debugging module (#146309)

Author: pablogsal

Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst

@@ -0,0 +1,5 @@
+Fixed multiple error handling issues in the :mod:`!_remote_debugging` module
+including a double-free in code object caching, memory leaks on allocation
+failure, missing exception checks in binary format varint decoding, reference
+leaks on error paths in frame chain processing, and inconsistent thread status
+error reporting across platforms. Patch by Pablo Galindo.

Modules/_remote_debugging/_remote_debugging.h

@@ -308,6 +308,7 @@ typedef struct {
 #endif
 #ifdef __APPLE__
     uint64_t thread_id_offset;
+    int thread_id_offset_initialized;
 #endif
 #ifdef MS_WINDOWS
     PVOID win_process_buffer;

Modules/_remote_debugging/asyncio.c

@@ -940,6 +940,9 @@ process_running_task_chain(
     PyObject *coro_chain = PyStructSequence_GET_ITEM(task_info, 2);
     assert(coro_chain != NULL);
     if (PyList_GET_SIZE(coro_chain) != 1) {
+        PyErr_Format(PyExc_RuntimeError,
+            "Expected single-item coro chain, got %zd items",
+            PyList_GET_SIZE(coro_chain));
         set_exception_cause(unwinder, PyExc_RuntimeError, "Coro chain is not a single item");
         return -1;
     }

Modules/_remote_debugging/binary_io.h

@@ -415,8 +415,8 @@ decode_varint_u32(const uint8_t *data, size_t *offset, size_t max_size)
 {
     size_t saved_offset = *offset;
     uint64_t value = decode_varint_u64(data, offset, max_size);
-    if (PyErr_Occurred()) {
-        return 0;
+    if (*offset == saved_offset) {
+        return 0;  /* decode_varint_u64 already set PyErr */
     }
     if (UNLIKELY(value > UINT32_MAX)) {
         *offset = saved_offset;
@@ -430,9 +430,10 @@ decode_varint_u32(const uint8_t *data, size_t *offset, size_t max_size)
 static inline int32_t
 decode_varint_i32(const uint8_t *data, size_t *offset, size_t max_size)
 {
+    size_t saved_offset = *offset;
     uint32_t zigzag = decode_varint_u32(data, offset, max_size);
-    if (PyErr_Occurred()) {
-        return 0;
+    if (*offset == saved_offset) {
+        return 0;  /* decode_varint_u32 already set PyErr */
     }
     return (int32_t)((zigzag >> 1) ^ -(int32_t)(zigzag & 1));
 }

Modules/_remote_debugging/binary_io_reader.c

@@ -571,15 +571,16 @@ reader_get_or_create_thread_state(BinaryReader *reader, uint64_t thread_id,
             return NULL;
         }
     } else if (reader->thread_state_count >= reader->thread_state_capacity) {
-        reader->thread_states = grow_array(reader->thread_states,
-                                           &reader->thread_state_capacity,
-                                           sizeof(ReaderThreadState));
-        if (!reader->thread_states) {
+        ReaderThreadState *new_states = grow_array(reader->thread_states,
+                                                   &reader->thread_state_capacity,
+                                                   sizeof(ReaderThreadState));
+        if (!new_states) {
             return NULL;
         }
+        reader->thread_states = new_states;
     }
 
-    ReaderThreadState *ts = &reader->thread_states[reader->thread_state_count++];
+    ReaderThreadState *ts = &reader->thread_states[reader->thread_state_count];
     memset(ts, 0, sizeof(ReaderThreadState));
     ts->thread_id = thread_id;
     ts->interpreter_id = interpreter_id;
@@ -590,6 +591,9 @@ reader_get_or_create_thread_state(BinaryReader *reader, uint64_t thread_id,
         PyErr_NoMemory();
         return NULL;
     }
+    // Increment count only after successful allocation to avoid
+    // leaving a half-initialized entry visible to future lookups
+    reader->thread_state_count++;
     return ts;
 }
 
@@ -604,7 +608,11 @@ static inline int
 decode_stack_full(ReaderThreadState *ts, const uint8_t *data,
                   size_t *offset, size_t max_size)
 {
+    size_t prev_offset = *offset;
     uint32_t depth = decode_varint_u32(data, offset, max_size);
+    if (*offset == prev_offset) {
+        return -1;
+    }
 
     /* Validate depth against capacity to prevent buffer overflow */
     if (depth > ts->current_stack_capacity) {
@@ -615,7 +623,11 @@ decode_stack_full(ReaderThreadState *ts, const uint8_t *data,
 
     ts->current_stack_depth = depth;
     for (uint32_t i = 0; i < depth; i++) {
+        size_t prev_offset = *offset;
         ts->current_stack[i] = decode_varint_u32(data, offset, max_size);
+        if (*offset == prev_offset) {
+            return -1;
+        }
     }
     return 0;
 }
@@ -627,8 +639,16 @@ static inline int
 decode_stack_suffix(ReaderThreadState *ts, const uint8_t *data,
                     size_t *offset, size_t max_size)
 {
+    size_t prev_offset = *offset;
     uint32_t shared = decode_varint_u32(data, offset, max_size);
+    if (*offset == prev_offset) {
+        return -1;
+    }
+    prev_offset = *offset;
     uint32_t new_count = decode_varint_u32(data, offset, max_size);
+    if (*offset == prev_offset) {
+        return -1;
+    }
 
     /* Validate shared doesn't exceed current stack depth */
     if (shared > ts->current_stack_depth) {
@@ -664,7 +684,11 @@ decode_stack_suffix(ReaderThreadState *ts, const uint8_t *data,
     }
 
     for (uint32_t i = 0; i < new_count; i++) {
+        size_t prev_offset = *offset;
         ts->current_stack[i] = decode_varint_u32(data, offset, max_size);
+        if (*offset == prev_offset) {
+            return -1;
+        }
     }
     ts->current_stack_depth = final_depth;
     return 0;
@@ -677,8 +701,16 @@ static inline int
 decode_stack_pop_push(ReaderThreadState *ts, const uint8_t *data,
                       size_t *offset, size_t max_size)
 {
+    size_t prev_offset = *offset;
     uint32_t pop = decode_varint_u32(data, offset, max_size);
+    if (*offset == prev_offset) {
+        return -1;
+    }
+    prev_offset = *offset;
     uint32_t push = decode_varint_u32(data, offset, max_size);
+    if (*offset == prev_offset) {
+        return -1;
+    }
     size_t keep = (ts->current_stack_depth > pop) ? ts->current_stack_depth - pop : 0;
 
     /* Validate final depth doesn't exceed capacity */
@@ -699,7 +731,12 @@ decode_stack_pop_push(ReaderThreadState *ts, const uint8_t *data,
     }
 
     for (uint32_t i = 0; i < push; i++) {
+        size_t prev_offset = *offset;
         ts->current_stack[i] = decode_varint_u32(data, offset, max_size);
+        /* If offset didn't advance, varint decoding failed */
+        if (*offset == prev_offset) {
+            return -1;
+        }
     }
     ts->current_stack_depth = final_depth;
     return 0;
@@ -1222,6 +1259,9 @@ binary_reader_close(BinaryReader *reader)
         reader->mapped_data = NULL;  /* Prevent use-after-free */
         reader->mapped_size = 0;
     }
+    /* Clear sample_data which may point into the now-unmapped region */
+    reader->sample_data = NULL;
+    reader->sample_data_size = 0;
     if (reader->fd >= 0) {
         close(reader->fd);
         reader->fd = -1;  /* Mark as closed */

Modules/_remote_debugging/code_objects.c

@@ -110,6 +110,7 @@ cache_tlbc_array(RemoteUnwinderObject *unwinder, uintptr_t code_addr, uintptr_t
     void *key = (void *)code_addr;
     if (_Py_hashtable_set(unwinder->tlbc_cache, key, entry) < 0) {
         tlbc_cache_entry_destroy(entry);
+        PyErr_NoMemory();
         set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to store TLBC entry in cache");
         return 0; // Cache error
     }
@@ -408,7 +409,14 @@ parse_code_object(RemoteUnwinderObject *unwinder,
         meta->addr_code_adaptive = real_address + (uintptr_t)unwinder->debug_offsets.code_object.co_code_adaptive;
 
         if (unwinder && unwinder->code_object_cache && _Py_hashtable_set(unwinder->code_object_cache, key, meta) < 0) {
+            // Ownership of func/file/linetable was transferred to meta,
+            // so NULL them before destroying meta to prevent double-free
+            // in the error label's Py_XDECREF calls.
+            func = NULL;
+            file = NULL;
+            linetable = NULL;
             cached_code_metadata_destroy(meta);
+            PyErr_NoMemory();
             set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to cache code metadata");
             goto error;
         }

Modules/_remote_debugging/frames.c

@@ -348,10 +348,12 @@ process_frame_chain(
             PyObject *extra_frame_info = make_frame_info(
                 unwinder, _Py_LATIN1_CHR('~'), Py_None, extra_frame, Py_None);
             if (extra_frame_info == NULL) {
+                Py_XDECREF(frame);
                 return -1;
             }
             if (PyList_Append(ctx->frame_info, extra_frame_info) < 0) {
                 Py_DECREF(extra_frame_info);
+                Py_XDECREF(frame);
                 set_exception_cause(unwinder, PyExc_RuntimeError, "Failed to append extra frame");
                 return -1;
             }

Modules/_remote_debugging/module.c

@@ -420,6 +420,7 @@ _remote_debugging_RemoteUnwinder___init___impl(RemoteUnwinderObject *self,
 
 #if defined(__APPLE__)
     self->thread_id_offset = 0;
+    self->thread_id_offset_initialized = 0;
 #endif
 
 #ifdef MS_WINDOWS

Modules/_remote_debugging/object_reading.c

@@ -196,6 +196,8 @@ read_py_long(
 
     // Validate size: reject garbage (negative or unreasonably large)
     if (size < 0 || size > MAX_LONG_DIGITS) {
+        PyErr_Format(PyExc_RuntimeError,
+            "Invalid PyLong digit count: %zd (expected 0-%d)", size, MAX_LONG_DIGITS);
         set_exception_cause(unwinder, PyExc_RuntimeError,
             "Invalid PyLong size (corrupted remote memory)");
         return -1;

Modules/_remote_debugging/threads.c

@@ -157,11 +157,11 @@ find_running_frame(
 int
 get_thread_status(RemoteUnwinderObject *unwinder, uint64_t tid, uint64_t pthread_id) {
 #if defined(__APPLE__) && TARGET_OS_OSX
-   if (unwinder->thread_id_offset == 0) {
+   if (!unwinder->thread_id_offset_initialized) {
         uint64_t *tids = (uint64_t *)PyMem_Malloc(MAX_NATIVE_THREADS * sizeof(uint64_t));
         if (!tids) {
-            PyErr_NoMemory();
-            return -1;
+            // Non-fatal: thread status is best-effort
+            return THREAD_STATE_UNKNOWN;
         }
         int n = proc_pidinfo(unwinder->handle.pid, PROC_PIDLISTTHREADS, 0, tids, MAX_NATIVE_THREADS * sizeof(uint64_t)) / sizeof(uint64_t);
         if (n <= 0) {
@@ -176,6 +176,7 @@ get_thread_status(RemoteUnwinderObject *unwinder, uint64_t tid, uint64_t pthread
             }
         }
         unwinder->thread_id_offset = min_offset;
+        unwinder->thread_id_offset_initialized = 1;
         PyMem_Free(tids);
     }
     struct proc_threadinfo ti;
@@ -239,20 +240,21 @@ get_thread_status(RemoteUnwinderObject *unwinder, uint64_t tid, uint64_t pthread
         unwinder->win_process_buffer_size = n;
         PVOID new_buffer = PyMem_Realloc(unwinder->win_process_buffer, n);
         if (!new_buffer) {
-            return -1;
+            // Match Linux/macOS: degrade gracefully on alloc failure
+            return THREAD_STATE_UNKNOWN;
         }
         unwinder->win_process_buffer = new_buffer;
         return get_thread_status(unwinder, tid, pthread_id);
     }
     if (status != STATUS_SUCCESS) {
-        return -1;
+        return THREAD_STATE_UNKNOWN;
     }
 
     SYSTEM_PROCESS_INFORMATION *pi = (SYSTEM_PROCESS_INFORMATION *)unwinder->win_process_buffer;
     while ((ULONG)(ULONG_PTR)pi->UniqueProcessId != unwinder->handle.pid) {
         if (pi->NextEntryOffset == 0) {
-            // We didn't find the process
-            return -1;
+            // Process not found (may have exited)
+            return THREAD_STATE_UNKNOWN;
         }
         pi = (SYSTEM_PROCESS_INFORMATION *)(((BYTE *)pi) + pi->NextEntryOffset);
     }
@@ -264,7 +266,8 @@ get_thread_status(RemoteUnwinderObject *unwinder, uint64_t tid, uint64_t pthread
         }
     }
 
-    return -1;
+    // Thread not found (may have exited)
+    return THREAD_STATE_UNKNOWN;
 #else
     return THREAD_STATE_UNKNOWN;
 #endif
@@ -385,12 +388,12 @@ unwind_stack_for_thread(
     long pthread_id = GET_MEMBER(long, ts, unwinder->debug_offsets.thread_state.thread_id);
 
     // Optimization: only check CPU status if needed by mode because it's expensive
-    int cpu_status = -1;
+    int cpu_status = THREAD_STATE_UNKNOWN;
     if (unwinder->mode == PROFILING_MODE_CPU || unwinder->mode == PROFILING_MODE_ALL) {
         cpu_status = get_thread_status(unwinder, tid, pthread_id);
     }
 
-    if (cpu_status == -1) {
+    if (cpu_status == THREAD_STATE_UNKNOWN) {
         status_flags |= THREAD_STATUS_UNKNOWN;
     } else if (cpu_status == THREAD_STATE_RUNNING) {
         status_flags |= THREAD_STATUS_ON_CPU;