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..9bc2f1c59a8c0c --- /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..616213541e12e1 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,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 */ 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;