From 512a7c7eb9be8b8a05b1568b1d378ee6f70069ad Mon Sep 17 00:00:00 2001 From: Taegyun Kim Date: Mon, 1 Dec 2025 09:31:50 -0500 Subject: [PATCH] fix(profiling): remove unnecessary access to `frame.f_locals` (#15434) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We've been seeing a low number of crashes that have the following stack traces. ``` PyFrame_FastToLocalsWithError _PyObject_GenericGetAttrWithDict __pyx_f_7ddtrace_9profiling_9collector_10_traceback__extract_class_name.constprop.0 __pyx_pw_7ddtrace_9profiling_9collector_10_traceback_5pyframe_to_frames ``` which corresponds to `value = frame.f_locals[argname]` in `extract_class_name()` function. Fore more details see notebook, https://app.datadoghq.com/notebook/13515661/pyframe-to-frames-crashes The reason this could lead to a crash is that the `frame` could be in an invalid state, partially deallocated, or corrupted. And this doesn't happen on our stack sampler (Echion) as it doesn't use it, and just use `co_qualname` (Python 3.11+) or `co_name` (Python < 3.11) This code is no longer needed as we don't use `class_name` field from `DDFrame` named tuple. Memory profiler has been putting an empty string. Lock profiler populated it but then it didn't export it in the sample. More appropriate way to populate the class name for Python versions 3.11+ would be using `co_qualname` as Echion does. I'll follow up with this in a separate PR. And this is why we see fully qualified name on our CPU time view, but function name only on memory/lock views. CPU time view Screenshot 2025-11-26 at 2 02 50 PM Memory view Screenshot 2025-11-26 at 2 02 35 PM (cherry picked from commit 124b19de38a6b9e671bfc6dc73a9aa4918ad3772) --- ddtrace/profiling/collector/_memalloc_tb.cpp | 13 +-------- ddtrace/profiling/collector/_traceback.pyx | 29 ++----------------- ddtrace/profiling/collector/stack.pyx | 3 -- ddtrace/profiling/event.py | 2 +- .../profiling-f_locals-c345ca501ca9b74f.yaml | 5 ++++ tests/profiling/collector/test_memalloc.py | 9 ++---- tests/profiling/collector/test_stack.py | 6 ++-- tests/profiling/collector/test_traceback.py | 4 +-- 8 files changed, 15 insertions(+), 56 deletions(-) create mode 100644 releasenotes/notes/profiling-f_locals-c345ca501ca9b74f.yaml diff --git a/ddtrace/profiling/collector/_memalloc_tb.cpp b/ddtrace/profiling/collector/_memalloc_tb.cpp index b4990c3e9d0..47087b5526a 100644 --- a/ddtrace/profiling/collector/_memalloc_tb.cpp +++ b/ddtrace/profiling/collector/_memalloc_tb.cpp @@ -13,8 +13,6 @@ /* A string containing "" just in case we can't store the real function * or file name. */ static PyObject* unknown_name = NULL; -/* A string containing "" */ -static PyObject* empty_string = NULL; #define TRACEBACK_SIZE(NFRAME) (sizeof(traceback_t) + sizeof(frame_t) * (NFRAME - 1)) @@ -135,12 +133,6 @@ memalloc_tb_init(uint16_t max_nframe) PyUnicode_InternInPlace(&unknown_name); } - if (empty_string == NULL) { - empty_string = PyUnicode_FromString(""); - if (empty_string == NULL) - return -1; - PyUnicode_InternInPlace(&empty_string); - } return 0; } @@ -297,7 +289,7 @@ traceback_to_tuple(traceback_t* tb) PyObject* stack = PyTuple_New(tb->nframe); for (uint16_t nframe = 0; nframe < tb->nframe; nframe++) { - PyObject* frame_tuple = PyTuple_New(4); + PyObject* frame_tuple = PyTuple_New(3); frame_t* frame = &tb->frames[nframe]; @@ -306,9 +298,6 @@ traceback_to_tuple(traceback_t* tb) PyTuple_SET_ITEM(frame_tuple, 1, PyLong_FromUnsignedLong(frame->lineno)); Py_INCREF(frame->name); PyTuple_SET_ITEM(frame_tuple, 2, frame->name); - /* Class name */ - Py_INCREF(empty_string); - PyTuple_SET_ITEM(frame_tuple, 3, empty_string); // Try to set the class. If we cannot (e.g., if the sofile is reloaded // without module initialization), then this will result in an error if diff --git a/ddtrace/profiling/collector/_traceback.pyx b/ddtrace/profiling/collector/_traceback.pyx index a562b6e7629..40e991f12f3 100644 --- a/ddtrace/profiling/collector/_traceback.pyx +++ b/ddtrace/profiling/collector/_traceback.pyx @@ -8,31 +8,6 @@ from ddtrace.profiling.event import DDFrame log = get_logger(__name__) -cpdef _extract_class_name(frame): - # type: (...) -> str - """Extract class name from a frame, if possible. - - :param frame: The frame object. - """ - code = frame.f_code - if code.co_argcount > 0: - # Retrieve the name of the first argument, if the code object has any - argname = code.co_varnames[0] - try: - value = frame.f_locals[argname] - except Exception: - log.debug("Unable to extract class name from frame %r", frame, exc_info=True) - return "" - try: - if argname == "self": - return object.__getattribute__(type(value), "__name__") # use type() and object.__getattribute__ to avoid side-effects - if argname == "cls": - return object.__getattribute__(value, "__name__") - except AttributeError: - return "" - return "" - - cpdef traceback_to_frames(traceback, max_nframes): """Serialize a Python traceback object into a list of tuple of (filename, lineno, function_name). @@ -48,7 +23,7 @@ cpdef traceback_to_frames(traceback, max_nframes): frame = tb.tb_frame code = frame.f_code lineno = 0 if frame.f_lineno is None else frame.f_lineno - frames.insert(0, DDFrame(code.co_filename, lineno, code.co_name, _extract_class_name(frame))) + frames.insert(0, DDFrame(code.co_filename, lineno, code.co_name)) nframes += 1 tb = tb.tb_next return frames, nframes @@ -92,7 +67,7 @@ cpdef pyframe_to_frames(frame, max_nframes): return [], 0 lineno = 0 if frame.f_lineno is None else frame.f_lineno - frames.append(DDFrame(code.co_filename, lineno, code.co_name, _extract_class_name(frame))) + frames.append(DDFrame(code.co_filename, lineno, code.co_name)) nframes += 1 frame = frame.f_back return frames, nframes diff --git a/ddtrace/profiling/collector/stack.pyx b/ddtrace/profiling/collector/stack.pyx index 78fb0efd26a..34c07ef92d9 100644 --- a/ddtrace/profiling/collector/stack.pyx +++ b/ddtrace/profiling/collector/stack.pyx @@ -317,7 +317,6 @@ cdef stack_collect(ignore_profiler, thread_time, max_nframes, interval, wall_tim handle.push_threadinfo(thread_id, thread_native_id, thread_name) handle.push_task_id(task_id) handle.push_task_name(task_name) - handle.push_class_name(frames[0].class_name) for frame in frames: handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) handle.flush_sample() @@ -330,7 +329,6 @@ cdef stack_collect(ignore_profiler, thread_time, max_nframes, interval, wall_tim handle.push_cputime( cpu_time, 1) handle.push_walltime( wall_time, 1) handle.push_threadinfo(thread_id, thread_native_id, thread_name) - handle.push_class_name(frames[0].class_name) for frame in frames: handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) handle.push_span(span) @@ -346,7 +344,6 @@ cdef stack_collect(ignore_profiler, thread_time, max_nframes, interval, wall_tim handle.push_monotonic_ns(now_ns) handle.push_threadinfo(thread_id, thread_native_id, thread_name) handle.push_exceptioninfo(exc_type, 1) - handle.push_class_name(frames[0].class_name) for frame in frames: handle.push_frame(frame.function_name, frame.file_name, 0, frame.lineno) handle.push_span(span) diff --git a/ddtrace/profiling/event.py b/ddtrace/profiling/event.py index 1c5bc8f379e..de5ea1f3992 100644 --- a/ddtrace/profiling/event.py +++ b/ddtrace/profiling/event.py @@ -2,5 +2,5 @@ import typing -DDFrame = namedtuple("DDFrame", ["file_name", "lineno", "function_name", "class_name"]) +DDFrame = namedtuple("DDFrame", ["file_name", "lineno", "function_name"]) StackTraceType = typing.List[DDFrame] diff --git a/releasenotes/notes/profiling-f_locals-c345ca501ca9b74f.yaml b/releasenotes/notes/profiling-f_locals-c345ca501ca9b74f.yaml new file mode 100644 index 00000000000..af4206656ef --- /dev/null +++ b/releasenotes/notes/profiling-f_locals-c345ca501ca9b74f.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + profiling: Fixes a segmentation fault caused by accessing ``frame.f_locals`` + while trying to retrieve class name of a ``PyFrameObject``. diff --git a/tests/profiling/collector/test_memalloc.py b/tests/profiling/collector/test_memalloc.py index 54f1997a46b..6762bd1e748 100644 --- a/tests/profiling/collector/test_memalloc.py +++ b/tests/profiling/collector/test_memalloc.py @@ -90,11 +90,10 @@ def test_iter_events(): __file__, _ALLOC_LINE_NUMBER, "" if sys.version_info < (3, 12) else "_allocate_1k", - "", ): assert thread_id == threading.main_thread().ident if sys.version_info < (3, 12) and len(stack) > 1: - assert stack[1] == DDFrame(__file__, _ALLOC_LINE_NUMBER, "_allocate_1k", "") + assert stack[1] == DDFrame(__file__, _ALLOC_LINE_NUMBER, "_allocate_1k") object_count += sample.count assert object_count >= 1000 @@ -156,12 +155,11 @@ def test_iter_events_multi_thread(): __file__, _ALLOC_LINE_NUMBER, "" if sys.version_info < (3, 12) else "_allocate_1k", - "", ): if thread_id == threading.main_thread().ident: count_object += sample.count if sys.version_info < (3, 12) and len(stack) > 1: - assert stack[1] == DDFrame(__file__, _ALLOC_LINE_NUMBER, "_allocate_1k", "") + assert stack[1] == DDFrame(__file__, _ALLOC_LINE_NUMBER, "_allocate_1k") elif thread_id == t.ident: count_thread += sample.count entry = 2 if sys.version_info < (3, 12) else 1 @@ -205,7 +203,6 @@ def _test_heap_impl(collector, max_nframe): __file__, _ALLOC_LINE_NUMBER, "" if sys.version_info < (3, 12) else "_allocate_1k", - "", ): break else: @@ -229,7 +226,6 @@ def _test_heap_impl(collector, max_nframe): __file__, _ALLOC_LINE_NUMBER, "" if sys.version_info < (3, 12) else "_allocate_1k", - "", ): break else: @@ -258,7 +254,6 @@ def _test_heap_impl(collector, max_nframe): __file__, _ALLOC_LINE_NUMBER, "" if sys.version_info < (3, 12) else "_allocate_1k", - "", ) and stack[entry].function_name == "_test_heap_impl" ): diff --git a/tests/profiling/collector/test_stack.py b/tests/profiling/collector/test_stack.py index 149f0635ba4..8bc40f3bc36 100644 --- a/tests/profiling/collector/test_stack.py +++ b/tests/profiling/collector/test_stack.py @@ -114,11 +114,9 @@ def _find_sleep_event(events, class_name): for e in events: for frame in e.frames: - if frame[0] == __file__.replace(".pyc", ".py") and frame[2] == "sleep_class" and frame[3] == class_name: + if frame[0] == __file__.replace(".pyc", ".py") and frame[2] == "sleep_class": class_method_found = True - elif ( - frame[0] == __file__.replace(".pyc", ".py") and frame[2] == "sleep_instance" and frame[3] == class_name - ): + elif frame[0] == __file__.replace(".pyc", ".py") and frame[2] == "sleep_instance": class_classmethod_found = True if class_method_found and class_classmethod_found: diff --git a/tests/profiling/collector/test_traceback.py b/tests/profiling/collector/test_traceback.py index 2caeaa90002..f6d7fa3ff71 100644 --- a/tests/profiling/collector/test_traceback.py +++ b/tests/profiling/collector/test_traceback.py @@ -17,6 +17,6 @@ def test_check_traceback_to_frames(): this_file = __file__.replace(".pyc", ".py") assert frames == [ - (this_file, 7, "_x", ""), - (this_file, 15, "test_check_traceback_to_frames", ""), + (this_file, 7, "_x"), + (this_file, 15, "test_check_traceback_to_frames"), ]