Port PR #3490

Author: cataphract

tests/ext/bailout_double_hook_clear.phpt

@@ -0,0 +1,43 @@
+--TEST--
+Bailout during hook END should not cause double-clear of span (internal function)
+--SKIPIF--
+<?php if (PHP_VERSION_ID >= 80000) die('skip: PHP 7 only'); ?>
+--FILE--
+<?php
+
+DDTrace\trace_function('array_sum', [
+    'prehook' => function (\DDTrace\SpanData $data, $args) {
+        echo "HOOK 1 PRE\n";
+    },
+    'posthook' => function(\DDTrace\SpanData $span, $args, $retval) {
+        static $ba = true;
+        echo "Hook 1 END\n";
+        if ($ba) {
+            echo "WILL BAILOUT!\n";
+        }
+        trigger_error("Datadog blocked the request", E_USER_ERROR);
+    }
+]);
+
+DDTrace\trace_function('array_sum', [
+    'posthook' => function(\DDTrace\SpanData $data, $args, $retval) {
+        echo "Hook 2 END\n";
+    }
+]);
+
+register_shutdown_function(function() {
+    echo "Shutdown function executed\n";
+});
+
+echo "Calling array_sum\n";
+array_sum([1, 2, 3]);
+echo "After array_sum (should not print)\n";
+
+?>
+--EXPECTF--
+Calling array_sum
+HOOK 1 PRE
+Hook 2 END
+Hook 1 END
+WILL BAILOUT!
+Shutdown function executed

zend_abstract_interface/hook/hook.c

@@ -1112,6 +1112,12 @@ void zai_hook_finish(zend_execute_data *ex, zval *rv, zai_hook_memory_t *memory)
     for (zai_hook_info *hook_start = memory->dynamic, *hook_info = hook_start + memory->hook_count - 1; hook_info >= hook_start; --hook_info) {
         zai_hook_t *hook = hook_info->hook;
 
+        // if we propagate bailout during hook->end, we don't try to finish
+        // again in zai_interceptor_terminate_all_pending_observers
+        // This may even segfault due to too many calls to
+        // ddtrace_clear_execute_data_span
+        memory->hook_count--;
+
         if (hook->end) {
             hook->end(memory->invocation, ex, rv, hook->aux.data, (char *)memory->dynamic + hook_info->dynamic_offset);
         }

zend_abstract_interface/interceptor/php7/interceptor.c

@@ -67,8 +67,8 @@ typedef struct {
 ZEND_TLS HashTable zai_hook_memory;
 // execute_data is 16 byte aligned (except when it isn't, but it doesn't matter as zend_execute_data is big enough
 // our goal is to reduce conflicts
-static inline void zai_hook_memory_table_insert(zend_execute_data *index, zai_interceptor_frame_memory *inserting) {
-    zend_hash_index_update_mem(&zai_hook_memory, ((zend_ulong)index) >> 4, inserting, sizeof(*inserting));
+static inline zai_interceptor_frame_memory *zai_hook_memory_table_insert(zend_execute_data *index, zai_interceptor_frame_memory *inserting) {
+    return zend_hash_index_update_mem(&zai_hook_memory, ((zend_ulong)index) >> 4, inserting, sizeof(*inserting));
 }
 
 static inline void *zai_hook_memory_table_insert_generator(zend_execute_data *index, zai_interceptor_generator_frame_memory *inserting) {
@@ -497,7 +497,7 @@ static inline void zai_interceptor_execute_internal_impl(zend_execute_data *exec
         }
         frame_memory.execute_data = execute_data;
         frame_memory.implicit = false;
-        zai_hook_memory_table_insert(execute_data, &frame_memory);
+        zai_interceptor_frame_memory *stored_frame_memory = zai_hook_memory_table_insert(execute_data, &frame_memory);
 
         // we do not use try / catch here as to preserve order of hooks, LIFO style, in bailout handler
         if (prev) {
@@ -506,7 +506,7 @@ static inline void zai_interceptor_execute_internal_impl(zend_execute_data *exec
             func->internal_function.handler(execute_data, return_value);
         }
 
-        zai_hook_finish(execute_data, return_value, &frame_memory.hook_data);
+        zai_hook_finish(execute_data, return_value, &stored_frame_memory->hook_data);
         zai_hook_memory_table_del(execute_data);
     } else {
         skip: ;