Fix debugger ephemerals handling (#3685)

This is now safe for nested debugger log probes as well not leaking into subsequent requests when probes are removed during runtime.

Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>

Author: bwoebi

ext/ddtrace.c

@@ -1615,7 +1615,6 @@ static PHP_MSHUTDOWN_FUNCTION(ddtrace) {
 
     ddtrace_sidecar_shutdown();
 
-    ddtrace_live_debugger_mshutdown();
     ddtrace_process_tags_mshutdown();
 
 #if PHP_VERSION_ID >= 80000 && PHP_VERSION_ID < 80100

ext/ddtrace.h

@@ -41,6 +41,13 @@ typedef struct ddtrace_span_event ddtrace_span_event;
 typedef struct ddtrace_exception_span_event ddtrace_exception_span_event;
 typedef struct ddtrace_git_metadata ddtrace_git_metadata;
 
+typedef struct dd_refcounted_linked dd_refcounted_linked;
+
+typedef struct {
+    zend_arena *arena;
+    dd_refcounted_linked *ephemerals;
+} dd_capture_arena;
+
 extern datadog_php_sapi ddtrace_active_sapi;
 
 extern ddog_CharSlice php_version_rt;
@@ -147,8 +154,7 @@ ZEND_BEGIN_MODULE_GLOBALS(ddtrace)
     ddog_AgentRemoteConfigReader *agent_config_reader;
     ddog_RemoteConfigState *remote_config_state;
     ddog_AgentInfoReader *agent_info_reader;
-    zend_arena *debugger_capture_arena;
-    HashTable debugger_capture_ephemerals;
+    dd_capture_arena debugger_capture_arena;
     ddog_Vec_DebuggerPayload exception_debugger_buffer;
     HashTable active_rc_hooks;
     HashTable *agent_rate_by_service;

ext/exception_serialize.c

@@ -91,7 +91,7 @@ static void ddtrace_capture_string_value(zend_string *str, struct ddog_CaptureVa
     if (!value->not_captured_reason.len) {
         value->value = (ddog_CharSlice) {.ptr = ZSTR_VAL(str), .len = ZSTR_LEN(str)};
         if (value->value.len > config->max_length) {
-            char *integer = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), 20);
+            char *integer = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, 20);
             int len = sprintf(integer, "%" PRIuPTR, value->value.len);
             value->size = (ddog_CharSlice) {.ptr = integer, .len = len};
             value->value.len = config->max_length;
@@ -103,7 +103,7 @@ static void ddtrace_capture_string_value(zend_string *str, struct ddog_CaptureVa
 static void ddtrace_capture_long_value(zend_long num, struct ddog_CaptureValue *value) {
     value->type = DDOG_CHARSLICE_C("int");
     if (!value->not_captured_reason.len) {
-        char *integer = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), 20);
+        char *integer = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, 20);
         int len = sprintf(integer, ZEND_LONG_FMT, num);
         value->value = (ddog_CharSlice) {.ptr = integer, .len = len};
     }
@@ -133,7 +133,7 @@ void ddtrace_create_capture_value(zval *zv, struct ddog_CaptureValue *value, con
         case IS_DOUBLE: {
             value->type = DDOG_CHARSLICE_C("float");
             if (!value->not_captured_reason.len) {
-                char *num = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), 20);
+                char *num = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, 20);
                 php_gcvt(Z_DVAL_P(zv), (int) EG(precision), '.', 'E', num);
                 value->value = (ddog_CharSlice) {.ptr = num, .len = strlen(num)};
             }
@@ -239,7 +239,7 @@ void ddtrace_create_capture_value(zval *zv, struct ddog_CaptureValue *value, con
                 } else if (ZSTR_VAL(key)[1] == '*') { // skip \0*\0
                     fieldname = (ddog_CharSlice) {.ptr = ZSTR_VAL(key) + 3, .len = ZSTR_LEN(key) - 3};
                 } else {
-                    char *name = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), ZSTR_LEN(key));
+                    char *name = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, ZSTR_LEN(key));
                     int classname_len = strlen(ZSTR_VAL(key) + 1);
                     memcpy(name, ZSTR_VAL(key) + 1, classname_len);
                     name[classname_len++] = ':';
@@ -254,12 +254,14 @@ void ddtrace_create_capture_value(zval *zv, struct ddog_CaptureValue *value, con
             } ZEND_HASH_FOREACH_END();
             if (ce->type == ZEND_INTERNAL_CLASS) {
 #if PHP_VERSION_ID < 70400
-                if (is_temp) {
-                    zend_hash_next_index_insert_ptr(&DDTRACE_G(debugger_capture_ephemerals), ht);
-                }
-#else
-                zend_hash_next_index_insert_ptr(&DDTRACE_G(debugger_capture_ephemerals), ht);
+                if (is_temp)
 #endif
+                {
+                    dd_refcounted_linked *node = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, sizeof(dd_refcounted_linked));
+                    node->value = (zend_refcounted *)ht;
+                    node->next = DDTRACE_G(debugger_capture_arena).ephemerals;
+                    DDTRACE_G(debugger_capture_arena).ephemerals = node;
+                }
             }
             break;
         }
@@ -281,10 +283,10 @@ void ddtrace_create_capture_value(zval *zv, struct ddog_CaptureValue *value, con
 #define hash_len 16
 
 static ddog_DebuggerCapture *dd_create_frame_and_collect_locals(char *exception_id, char *exception_hash, int frame_num, ddog_CharSlice class_slice, ddog_CharSlice func_slice, zval *locals, zend_string *service_name, const ddog_CaptureConfiguration *capture_config, uint64_t time, ddog_SpanBytes *span) {
-    char *snapshot_id = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), uuid_len);
+    char *snapshot_id = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, uuid_len);
     ddog_snapshot_format_new_uuid((uint8_t(*)[uuid_len])snapshot_id);
 
-    char *msg = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), 40);
+    char *msg = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, 40);
     int len = sprintf(msg, "_dd.debug.error.%d.snapshot_id", frame_num);
     ddog_add_span_meta(span, (ddog_CharSlice){.ptr = msg, .len = len}, (ddog_CharSlice){.ptr = snapshot_id, .len = uuid_len});
 
@@ -368,14 +370,14 @@ static void ddtrace_collect_exception_debug_data(zend_object *exception, zend_st
     zend_string *key_locals = zend_string_init(ZEND_STRL("locals"), 0);
     zval *locals = zai_exception_read_property(exception, key_locals);
 
-    bool has_arena = DDTRACE_G(debugger_capture_arena);
+    bool has_arena = DDTRACE_G(debugger_capture_arena).arena;
     if (!has_arena) {
-        DDTRACE_G(debugger_capture_arena) = zend_arena_create(65536);
+        DDTRACE_G(debugger_capture_arena) = (dd_capture_arena){.arena = zend_arena_create(65536), .ephemerals = NULL};
     }
 
     const ddog_CaptureConfiguration capture_config = ddog_capture_defaults();
 
-    char *exception_hash = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), hash_len);
+    char *exception_hash = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, hash_len);
     zend_ulong exception_long_hash = ddtrace_compute_exception_hash(exception);
     php_hash_bin2hex(exception_hash, (unsigned char *)&exception_long_hash, sizeof(exception_long_hash));
 
@@ -387,7 +389,7 @@ static void ddtrace_collect_exception_debug_data(zend_object *exception, zend_st
         goto cleanup;
     }
 
-    char *exception_id = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), uuid_len);
+    char *exception_id = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, uuid_len);
     ddog_snapshot_format_new_uuid((uint8_t(*)[uuid_len])exception_id);
 
     ddog_add_str_span_meta_CharSlice(span, "_dd.debug.error.exception_capture_id", (ddog_CharSlice){.ptr = exception_id, .len = uuid_len});
@@ -439,11 +441,11 @@ static void ddtrace_collect_exception_debug_data(zend_object *exception, zend_st
                         zend_string *name = func->op_array.arg_info[idx].name;
                         arg_name = (ddog_CharSlice) {.ptr = ZSTR_VAL(name), .len = ZSTR_LEN(name)};
                     } else {
-                        const char *name = func->internal_function.arg_info[idx].name;
+                        const char *name = (const char*) func->internal_function.arg_info[idx].name;
                         arg_name = (ddog_CharSlice) {.ptr = name, .len = strlen(name)};
                     }
                 } else {
-                    char *integer = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena), 23);
+                    char *integer = zend_arena_alloc(&DDTRACE_G(debugger_capture_arena).arena, 23);
                     int len = sprintf(integer, "arg" ZEND_LONG_FMT, idx);
                     arg_name = (ddog_CharSlice){ .ptr = integer, .len = len };
                 }
@@ -470,8 +472,9 @@ static void ddtrace_collect_exception_debug_data(zend_object *exception, zend_st
 
 cleanup:
     if (!has_arena) {
-        zend_arena_destroy(DDTRACE_G(debugger_capture_arena));
-        DDTRACE_G(debugger_capture_arena) = NULL;
+        dd_free_capture_ephemerals(DDTRACE_G(debugger_capture_arena).ephemerals);
+        zend_arena_destroy(DDTRACE_G(debugger_capture_arena).arena);
+        DDTRACE_G(debugger_capture_arena) = (dd_capture_arena){0};
     }
 
     zend_string_release(key_locals);

ext/live_debugger.c

@@ -366,7 +366,7 @@ typedef struct {
     bool rejected;
     ddog_DebuggerPayload *payload;
     zend_string *service;
-    zend_arena *capture_arena;
+    dd_capture_arena capture_arena;
 } dd_log_probe_dyn;
 
 static bool dd_log_probe_eval_condition(dd_log_probe_def *def, zend_execute_data *execute_data, zval *retval) {
@@ -554,8 +554,9 @@ static void dd_log_probe_end(zend_ulong invocation, zend_execute_data *execute_d
         if (dyn->payload) {
             ddog_drop_debugger_payload(dyn->payload);
             zend_string_release(dyn->service);
-            if (dyn->capture_arena) {
-                zend_arena_destroy(dyn->capture_arena);
+            if (dyn->capture_arena.arena) {
+                dd_free_capture_ephemerals(dyn->capture_arena.ephemerals);
+                zend_arena_destroy(dyn->capture_arena.arena);
             }
         }
         return;
@@ -574,8 +575,8 @@ static void dd_log_probe_end(zend_ulong invocation, zend_execute_data *execute_d
     dd_log_probe_ensure_payload(dyn, def, &result_msg);
 
     if (def->parent.probe.probe.log.capture_snapshot) {
-        bool already_snapshotted = dyn->capture_arena;
-        DDTRACE_G(debugger_capture_arena) = dyn->capture_arena ? dyn->capture_arena : zend_arena_create(65536);
+        bool already_snapshotted = dyn->capture_arena.arena;
+        DDTRACE_G(debugger_capture_arena) = already_snapshotted ? dyn->capture_arena : (dd_capture_arena){.arena = zend_arena_create(65536), .ephemerals = NULL};
         ddog_DebuggerCapture *capture = ddog_snapshot_exit(dyn->payload);
         if (!already_snapshotted) {
             dd_probe_capture_stack(dyn->payload, execute_data);
@@ -587,9 +588,10 @@ static void dd_log_probe_end(zend_ulong invocation, zend_execute_data *execute_d
         ddog_snapshot_add_field(capture, DDOG_FIELD_TYPE_ARG, DDOG_CHARSLICE_C("@return"), capture_value);
     }
     ddtrace_sidecar_send_debugger_datum(dyn->payload);
-    if (DDTRACE_G(debugger_capture_arena)) {
-        zend_arena_destroy(DDTRACE_G(debugger_capture_arena));
-        DDTRACE_G(debugger_capture_arena) = NULL;
+    if (DDTRACE_G(debugger_capture_arena).arena) {
+        dd_free_capture_ephemerals(DDTRACE_G(debugger_capture_arena).ephemerals);
+        zend_arena_destroy(DDTRACE_G(debugger_capture_arena).arena);
+        DDTRACE_G(debugger_capture_arena) = (dd_capture_arena){0};
     }
     zend_string_release(result);
     zend_string_release(dyn->service);
@@ -607,17 +609,17 @@ static bool dd_log_probe_begin(zend_ulong invocation, zend_execute_data *execute
 
     dyn->payload = NULL;
     dyn->rejected = def->parent.probe.evaluate_at == DDOG_EVALUATE_AT_ENTRY && !dd_log_probe_eval_condition(def, execute_data, &retval);
-    dyn->capture_arena = NULL;
+    dyn->capture_arena = (dd_capture_arena){0};
 
     if (!dyn->rejected && def->parent.probe.evaluate_at == DDOG_EVALUATE_AT_ENTRY) {
         dd_log_probe_ensure_payload(dyn, def, NULL);
         if (def->parent.probe.probe.log.capture_snapshot) {
             ddog_DebuggerCapture *capture = ddog_snapshot_entry(dyn->payload);
-            DDTRACE_G(debugger_capture_arena) = zend_arena_create(65536);
+            DDTRACE_G(debugger_capture_arena) = (dd_capture_arena){.arena = zend_arena_create(65536), .ephemerals = NULL};
             dd_probe_capture_stack(dyn->payload, execute_data);
             dd_log_probe_capture_snapshot(capture, def, execute_data);
             dyn->capture_arena = DDTRACE_G(debugger_capture_arena);
-            DDTRACE_G(debugger_capture_arena) = NULL;
+            DDTRACE_G(debugger_capture_arena) = (dd_capture_arena){0};
         }
     }
 
@@ -1438,19 +1440,19 @@ ddog_LiveDebuggerSetup ddtrace_live_debugger_setup = {
     .evaluator = &dd_evaluator,
 };
 
-static void dd_ht_ephemerals_dtor(void *pData) {
-    HashTable *ht = *((HashTable **)pData);
-
+void dd_free_capture_ephemerals(dd_refcounted_linked *ephemerals) {
+    while (ephemerals) {
+        HashTable *ht = (HashTable *)ephemerals->value;
 #if PHP_VERSION_ID < 70400
-    zend_array_release(ht);
+        zend_array_release(ht);
 #else
-    zend_release_properties(ht);
+        zend_release_properties(ht);
 #endif
+        ephemerals = ephemerals->next;
+    }
 }
 
 void ddtrace_live_debugger_minit(void) {
-    zend_hash_init(&DDTRACE_G(debugger_capture_ephemerals), 8, NULL, (dtor_func_t)dd_ht_ephemerals_dtor, 1);
-
     zend_string *value;
     ZEND_HASH_FOREACH_STR_KEY(get_global_DD_DYNAMIC_INSTRUMENTATION_REDACTED_IDENTIFIERS(), value) {
         ddog_snapshot_add_redacted_name(dd_zend_string_to_CharSlice(value));
@@ -1460,10 +1462,6 @@ void ddtrace_live_debugger_minit(void) {
     } ZEND_HASH_FOREACH_END();
 }
 
-void ddtrace_live_debugger_mshutdown(void) {
-    zend_hash_destroy(&DDTRACE_G(debugger_capture_ephemerals));
-}
-
 bool ddtrace_alter_dynamic_instrumentation_config(zval *old_value, zval *new_value, zend_string *new_str) {
     UNUSED(old_value, new_str);
     if (DDTRACE_G(remote_config_state) && !ddog_remote_config_alter_dynamic_config(DDTRACE_G(remote_config_state), DDOG_CHARSLICE_C("datadog.dynamic_instrumentation.enabled"), zend_string_copy(new_str))) {

ext/live_debugger.h

@@ -7,7 +7,6 @@
 extern ddog_LiveDebuggerSetup ddtrace_live_debugger_setup;
 
 void ddtrace_live_debugger_minit(void);
-void ddtrace_live_debugger_mshutdown(void);
 bool ddtrace_alter_dynamic_instrumentation_config(zval *old_value, zval *new_value, zend_string *new_str);
 ddog_DynamicInstrumentationConfigState ddtrace_dynamic_instrumentation_state(void);
 
@@ -17,4 +16,10 @@ static inline void ddtrace_snapshot_redacted_name(ddog_CaptureValue *capture_val
     }
 }
 
+struct dd_refcounted_linked {
+    struct dd_refcounted_linked *next;
+    zend_refcounted *value;
+};
+void dd_free_capture_ephemerals(struct dd_refcounted_linked *ephemerals);
+
 #endif // DD_LIVE_DEBUGGER_H

ext/sidecar.c

@@ -582,13 +582,11 @@ void ddtrace_sidecar_submit_root_span_data(void) {
 void ddtrace_sidecar_send_debugger_data(ddog_Vec_DebuggerPayload payloads) {
     LOGEV(DEBUG, UNUSED(log); ddog_log_debugger_data(&payloads););
     ddog_sidecar_send_debugger_data(&ddtrace_sidecar, ddtrace_sidecar_instance_id, DDTRACE_G(sidecar_queue_id), payloads);
-    zend_hash_clean(&DDTRACE_G(debugger_capture_ephemerals));
 }
 
 void ddtrace_sidecar_send_debugger_datum(ddog_DebuggerPayload *payload) {
     LOGEV(DEBUG, UNUSED(log); ddog_log_debugger_datum(payload););
     ddog_sidecar_send_debugger_datum(&ddtrace_sidecar, ddtrace_sidecar_instance_id, DDTRACE_G(sidecar_queue_id), payload);
-    zend_hash_clean(&DDTRACE_G(debugger_capture_ephemerals));
 }
 
 void ddtrace_sidecar_activate(void) {