Fix use-after-free in exception replay (#2947)

Depending on where the exception originates, a span may have been part of it and be freed during serialization. Immediately send to avoid dangling pointers. Signed-off-by: Bob Weinand <[email protected]>
DataDog · Nov 13, 2024 · 1f33697 · 1f33697
1 parent e2b0a15
commit 1f33697
Show file tree

Hide file tree

Showing 2 changed files with 9 additions and 10 deletions.
diff --git a/ext/exception_serialize.c b/ext/exception_serialize.c
@@ -398,6 +398,8 @@ static void ddtrace_collect_exception_debug_data(zend_object *exception, zend_st
 
     add_meta(context, DDTRACE_STRING_LITERAL("_dd.debug.error.exception_capture_id"), (ddtrace_string){exception_id, uuid_len});
 
+    memset(&DDTRACE_G(exception_debugger_buffer), 0, sizeof(DDTRACE_G(exception_debugger_buffer)));
+
     zval *frame;
     int frame_num = 0;
     ZEND_HASH_FOREACH_NUM_KEY_VAL(Z_ARR_P(trace), frame_num, frame) {
@@ -470,6 +472,13 @@ static void ddtrace_collect_exception_debug_data(zend_object *exception, zend_st
         }
     }
 
+    // Note: We MUST immediately send this, and not defer, as stuff may be freed during span processing. Including stuff potentially contained within the exception debugger payload.
+    ddtrace_sidecar_send_debugger_data(DDTRACE_G(exception_debugger_buffer));
+    if (DDTRACE_G(debugger_capture_arena)) {
+        zend_arena_destroy(DDTRACE_G(debugger_capture_arena));
+        DDTRACE_G(debugger_capture_arena) = NULL;
+    }
+
     zend_string_release(key_locals);
 }
 

diff --git a/ext/span.c b/ext/span.c
@@ -725,8 +725,6 @@ void ddtrace_drop_span(ddtrace_span_data *span) {
 
 void ddtrace_serialize_closed_spans(zval *serialized) {
     if (DDTRACE_G(top_closed_stack)) {
-        memset(&DDTRACE_G(exception_debugger_buffer), 0, sizeof(DDTRACE_G(exception_debugger_buffer)));
-
         ddtrace_span_stack *rootstack = DDTRACE_G(top_closed_stack);
         DDTRACE_G(top_closed_stack) = NULL;
         do {
@@ -759,14 +757,6 @@ void ddtrace_serialize_closed_spans(zval *serialized) {
                 }
             } while (stack);
         } while (rootstack);
-
-        if (ddtrace_exception_debugging_is_active()) {
-            ddtrace_sidecar_send_debugger_data(DDTRACE_G(exception_debugger_buffer));
-            if (DDTRACE_G(debugger_capture_arena)) {
-                zend_arena_destroy(DDTRACE_G(debugger_capture_arena));
-                DDTRACE_G(debugger_capture_arena) = NULL;
-            }
-        }
     }
 
     // Reset closed span counter for limit-refresh, don't touch open spans