From 7a0560eb184dea6fe4c6f20029370c62ab4d254f Mon Sep 17 00:00:00 2001 From: Pablo Galindo Salgado Date: Sun, 22 Mar 2026 20:12:57 +0000 Subject: [PATCH 1/3] gh-146308: Fix error handling issues in _remote_debugging module --- ...-03-22-19-30-00.gh-issue-146308.AxnRVA.rst | 5 ++ Modules/_remote_debugging/_remote_debugging.h | 1 + Modules/_remote_debugging/asyncio.c | 3 ++ Modules/_remote_debugging/binary_io.h | 9 ++-- Modules/_remote_debugging/binary_io_reader.c | 49 +++++++++++++++++-- Modules/_remote_debugging/code_objects.c | 8 +++ Modules/_remote_debugging/frames.c | 2 + Modules/_remote_debugging/module.c | 1 + Modules/_remote_debugging/object_reading.c | 2 + Modules/_remote_debugging/threads.c | 23 +++++---- 10 files changed, 84 insertions(+), 19 deletions(-) create mode 100644 Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst new file mode 100644 index 00000000000000..57bd79bc5c0c26 --- /dev/null +++ b/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. diff --git a/Modules/_remote_debugging/_remote_debugging.h b/Modules/_remote_debugging/_remote_debugging.h index 7bcb2f483234ec..eaf6cdb500c47b 100644 --- a/Modules/_remote_debugging/_remote_debugging.h +++ b/Modules/_remote_debugging/_remote_debugging.h @@ -307,6 +307,7 @@ typedef struct { #endif #ifdef __APPLE__ uint64_t thread_id_offset; + int thread_id_offset_initialized; #endif #ifdef MS_WINDOWS PVOID win_process_buffer; diff --git a/Modules/_remote_debugging/asyncio.c b/Modules/_remote_debugging/asyncio.c index 67a97a53db6415..12a8a9acc13bac 100644 --- a/Modules/_remote_debugging/asyncio.c +++ b/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; } diff --git a/Modules/_remote_debugging/binary_io.h b/Modules/_remote_debugging/binary_io.h index f8399f4aebe74b..d90546078bf68c 100644 --- a/Modules/_remote_debugging/binary_io.h +++ b/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)); } diff --git a/Modules/_remote_debugging/binary_io_reader.c b/Modules/_remote_debugging/binary_io_reader.c index cb58a0ed199d4a..208a646e07dc22 100644 --- a/Modules/_remote_debugging/binary_io_reader.c +++ b/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,11 @@ 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 == prev_offset) { + return -1; + } } ts->current_stack_depth = final_depth; return 0; @@ -1222,6 +1258,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 */ diff --git a/Modules/_remote_debugging/code_objects.c b/Modules/_remote_debugging/code_objects.c index 91f7a02005391a..7b95c0f2d4fa8d 100644 --- a/Modules/_remote_debugging/code_objects.c +++ b/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; } diff --git a/Modules/_remote_debugging/frames.c b/Modules/_remote_debugging/frames.c index 2ace0c0f7676ae..a0b4a1e8a1e542 100644 --- a/Modules/_remote_debugging/frames.c +++ b/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; } diff --git a/Modules/_remote_debugging/module.c b/Modules/_remote_debugging/module.c index 040bd3db377315..fc67e770b7b7a2 100644 --- a/Modules/_remote_debugging/module.c +++ b/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 diff --git a/Modules/_remote_debugging/object_reading.c b/Modules/_remote_debugging/object_reading.c index 447b7fd5926064..59c28e223c545f 100644 --- a/Modules/_remote_debugging/object_reading.c +++ b/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; diff --git a/Modules/_remote_debugging/threads.c b/Modules/_remote_debugging/threads.c index 3100b83c8f4899..4efcb639cb1aff 100644 --- a/Modules/_remote_debugging/threads.c +++ b/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 @@ -384,12 +387,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; From ad81c5b0f4437dbbb296b748504e2f09b88ddc60 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Salgado Date: Sun, 22 Mar 2026 20:19:08 +0000 Subject: [PATCH 2/3] Update Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst Co-authored-by: Brian Schubert --- .../2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst b/Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst index 57bd79bc5c0c26..9bc2f1c59a8c0c 100644 --- a/Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst +++ b/Misc/NEWS.d/next/Core_and_Builtins/2026-03-22-19-30-00.gh-issue-146308.AxnRVA.rst @@ -1,4 +1,4 @@ -Fixed multiple error handling issues in the :mod:`_remote_debugging` module +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 From 359cf5c8ead187f09448846d4468f053cbc4c338 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Salgado Date: Sun, 22 Mar 2026 20:45:50 +0000 Subject: [PATCH 3/3] Update Modules/_remote_debugging/binary_io_reader.c Co-authored-by: devdanzin <74280297+devdanzin@users.noreply.github.com> --- Modules/_remote_debugging/binary_io_reader.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/_remote_debugging/binary_io_reader.c b/Modules/_remote_debugging/binary_io_reader.c index 208a646e07dc22..616213541e12e1 100644 --- a/Modules/_remote_debugging/binary_io_reader.c +++ b/Modules/_remote_debugging/binary_io_reader.c @@ -733,6 +733,7 @@ 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; }