Fix hook dynamic data zeroing when new hooks are added during hook ex…

…ecution (#1730) * Fix hook dynamic data zeroing when new hooks are added during hook execution Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com> * Add test for adding hooks during hook begin handler Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
DataDog · Sep 7, 2022 · 9ddbb5c · 9ddbb5c
1 parent 6e3ac23
commit 9ddbb5c
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 2 deletions.
diff --git a/tea/src/sapi.c b/tea/src/sapi.c
@@ -29,7 +29,11 @@ static ssize_t ini_entries_len = -1;
 void (*tea_sapi_register_custom_server_variables)(zval *track_vars_server_array);
 
 static int ts_startup(sapi_module_struct *sapi_module) {
+#if PHP_VERSION_ID < 80200
     return php_module_startup(sapi_module, tea_extension_module(), 1);
+#else
+    return php_module_startup(sapi_module, tea_extension_module());
+#endif
 }
 
 static int ts_deactivate() { return SUCCESS; }

diff --git a/zend_abstract_interface/hook/hook.c b/zend_abstract_interface/hook/hook.c
@@ -379,6 +379,7 @@ zai_hook_continued zai_hook_continue(zend_execute_data *ex, zai_hook_memory_t *m
     int allocated_hook_count = zend_hash_num_elements(&hooks->hooks);
     size_t hook_info_size = allocated_hook_count * sizeof(zai_hook_info);
     size_t dynamic_size = hooks->dynamic + hook_info_size;
+    // a vector of first N hook_info entries, then N entries of variable size (as much memory as the individual hooks require)
     memory->dynamic = ecalloc(1, dynamic_size);
     memory->invocation = ++zai_hook_invocation;
 
@@ -415,9 +416,12 @@ zai_hook_continued zai_hook_continue(zend_execute_data *ex, zai_hook_memory_t *m
             if (new_dynamic_size > dynamic_size) {
                 memory->dynamic = erealloc(memory->dynamic, new_dynamic_size);
             }
+            // Create some space for zai_hook_info entries in between, and some new dynamic memory at the end
             memmove(memory->dynamic + new_hook_info_size, memory->dynamic + hook_info_size, dynamic_size - hook_info_size);
             if (new_dynamic_size > dynamic_size) {
-                memset(memory->dynamic + dynamic_size, 0, new_dynamic_size - dynamic_size);
+                // and ensure the new dynamic memory is zeroed
+                size_t hook_info_size_delta = new_hook_info_size - hook_info_size;
+                memset(memory->dynamic + dynamic_size + hook_info_size_delta, 0, new_dynamic_size - dynamic_size - hook_info_size_delta);
                 dynamic_size = new_dynamic_size;
             }
             hook_info_size = new_hook_info_size;

diff --git a/zend_abstract_interface/hook/hook.h b/zend_abstract_interface/hook/hook.h
@@ -67,7 +67,7 @@ typedef enum {
     ZAI_HOOK_SKIP,
 } zai_hook_continued;
 
-/* {{{ zai_hook_continue shall execute begin handlers and return false if
+/* {{{ zai_hook_continue shall execute begin handlers and return ZAI_HOOK_BAILOUT if
         the caller should bail out (one of the handlers returned false) */
 zai_hook_continued zai_hook_continue(zend_execute_data *ex, zai_hook_memory_t *memory); /* }}} */
 

diff --git a/zend_abstract_interface/hook/tests/internal/request.cc b/zend_abstract_interface/hook/tests/internal/request.cc
@@ -30,6 +30,8 @@ extern "C" {
         zai_hook_test_begin_fixed   = fixed;
         zai_hook_test_begin_dynamic = dynamic;
 
+        CHECK(dynamic->u == 0);
+
         zai_hook_test_begin_check++;
 
         return zai_hook_test_begin_return;
@@ -328,3 +330,74 @@ HOOK_TEST_CASE("unresolved removal", {
 }, {
     REQUIRE(zai_hook_remove(ZAI_STRING_EMPTY, zai_hook_test_nonexistent, zai_hook_test_index));
 });
+
+extern "C" {
+    static zai_hook_test_dynamic_t zai_hook_add_test_begin_dynamic = {42};
+
+    static bool zai_hook_test_hook_add_begin(zend_ulong invocation, zend_execute_data *ex, zai_hook_test_fixed_t *fixed, zai_hook_test_dynamic_t *dynamic) {
+        CHECK(dynamic->u == 0);
+        dynamic->u = 1;
+
+        REQUIRE(zai_hook_install(
+            ZAI_STRING_EMPTY,
+            zai_hook_test_target,
+            zai_hook_test_begin,
+            zai_hook_test_end,
+            ZAI_HOOK_AUX(&zai_hook_test_fixed_first, NULL),
+            sizeof(zai_hook_test_dynamic_t)) != -1);
+
+        REQUIRE(zai_hook_install(
+            ZAI_STRING_EMPTY,
+            zai_hook_test_target,
+            zai_hook_test_begin,
+            zai_hook_test_end,
+            ZAI_HOOK_AUX(&zai_hook_test_fixed_second, NULL),
+            sizeof(zai_hook_test_dynamic_t)) != -1);
+
+        CHECK(dynamic->u == 1);
+        dynamic->u = 2;
+
+        zai_hook_test_begin_check++;
+        return true;
+    }
+
+    static void zai_hook_test_hook_add_end(zend_ulong invocation, zend_execute_data *ex, zval *rv, zai_hook_test_fixed_t *fixed, zai_hook_test_dynamic_t *dynamic) {
+        zai_hook_add_test_begin_dynamic = *dynamic;
+
+        CHECK(zai_hook_test_end_check == 2); // called last
+        zai_hook_test_end_check++;
+    }
+}
+
+HOOK_TEST_CASE("hook add during begin", {
+    zai_hook_test_reset(true);
+}, {
+    zai_hook_test_index = zai_hook_install(
+        ZAI_STRING_EMPTY,
+        zai_hook_test_target,
+        zai_hook_test_hook_add_begin,
+        zai_hook_test_hook_add_end,
+        ZAI_HOOK_AUX(&zai_hook_test_fixed_first, NULL),
+        sizeof(zai_hook_test_dynamic_t));
+
+    REQUIRE(zai_hook_test_index != -1);
+}, {
+    zval result;
+
+    CHECK(zai_symbol_call(
+        ZAI_SYMBOL_SCOPE_GLOBAL, NULL,
+        ZAI_SYMBOL_FUNCTION_NAMED, &zai_hook_test_target,
+        &result, 0));
+
+    CHECK(zai_hook_test_begin_check == 3);
+    CHECK(zai_hook_test_end_check == 3);
+
+    CHECK(zai_hook_test_begin_dynamic != zai_hook_test_end_dynamic);
+
+    CHECK(zai_hook_test_begin_fixed == &zai_hook_test_fixed_second);
+    CHECK(zai_hook_test_end_fixed == &zai_hook_test_fixed_first);
+
+    CHECK(zai_hook_add_test_begin_dynamic.u == 2);
+
+    zval_ptr_dtor(&result);
+});