wip

appsec/tests/integration/gradle/images.gradle

@@ -110,7 +110,6 @@ def buildApache2FpmTask = { String version, String variant ->
         description = "Build the image for Apache2 + fpm ${version} ${variant}"
         inputs.dir 'src/docker/apache2-fpm'
         inputs.dir 'src/docker/fpm-common'
-        inputs.dir 'src/docker/common'
         outputs.upToDateWhen {
             imageUpToDate(inputs, image)() &&
                     imageIsNewerThan(image, "$repo:php-$version-$variant")
@@ -140,7 +139,6 @@ def buildNginxFpmTask = { String version, String variant ->
         description = "Build the image for Nginx + fpm ${version} ${variant}"
         inputs.dir 'src/docker/nginx-fpm'
         inputs.dir 'src/docker/fpm-common'
-        inputs.dir 'src/docker/common'
         outputs.upToDateWhen {
             imageUpToDate(inputs, image)() &&
                     imageIsNewerThan(image, "$repo:php-$version-$variant")
@@ -160,6 +158,9 @@ tasks.register('buildAllNginxFpm') {
         dependsOn "buildNginxFpm-${spec[0]}-${spec[1]}"
     }
 }
+task buildAll {
+    dependsOn 'buildAllPhp', 'buildAllApache2Mod', 'buildAllApache2Fpm', 'buildAllNginxFpm'
+}
 
 def buildPushTask = { String tag, requirement ->
     def task = tasks.register("pushImage-${tag}", Exec) {

ext/ddtrace.c

@@ -783,11 +783,6 @@ static void dd_disable_if_incompatible_sapi_detected(void) {
     }
 }
 
-static void ddtrace_execute_ex(zend_execute_data *execute_data)
-{
-    execute_ex(execute_data);
-}
-
 static PHP_MINIT_FUNCTION(ddtrace) {
     UNUSED(type);
 
@@ -875,12 +870,6 @@ static PHP_MINIT_FUNCTION(ddtrace) {
     dd_ip_extraction_startup();
     ddtrace_user_request_startup();
 
-    // we need to trigger the slow path in the fcall handler in order to be able to suppress calls
-    // otherwise OPLINE is not refreshed after calling the begin observers
-    if (zend_execute_ex == execute_ex) {
-        zend_execute_ex = ddtrace_execute_ex;
-    }
-
     return SUCCESS;
 }
 

ext/hook/uhook.c

@@ -218,6 +218,30 @@ static bool dd_uhook_match_filepath(zend_string *file, zend_string *source) {
     return false;
 }
 
+#if PHP_VERSION_ID >= 80000
+static void (*orig_zend_interrupt_function)(zend_execute_data *);
+ZEND_TLS zend_execute_data *expected_ex;
+static void dd_zend_interrupt_function(zend_execute_data *ex)
+{
+    if (!expected_ex) {
+        goto call_orig;
+    }
+
+    if (ex != expected_ex) {
+        expected_ex = NULL;
+        goto call_orig;
+    }
+
+    expected_ex = NULL;
+    ex->opline = ex->func->op_array.opcodes;
+
+call_orig:
+    if (orig_zend_interrupt_function) {
+        orig_zend_interrupt_function(ex);
+    }
+}
+#endif
+
 static bool dd_uhook_begin(zend_ulong invocation, zend_execute_data *execute_data, void *auxiliary, void *dynamic) {
     dd_uhook_def *def = auxiliary;
     dd_uhook_dynamic *dyn = dynamic;
@@ -260,32 +284,45 @@ static bool dd_uhook_begin(zend_ulong invocation, zend_execute_data *execute_dat
 
     if (dyn->hook_data->suppress_call) {
         if (ZEND_USER_CODE(execute_data->func->type)) {
-            static union {
+            static struct {
                 zend_op zop;
                 zval zv;
-            } retop[] = {[0].zop =
+            } retop = { .zop =
                              {
                                  .opcode = ZEND_RETURN,
                                  .op1_type = IS_CONST,
-                                 .op1 = {.constant = sizeof(retop[0])},
+                                 .op1 = {.constant = offsetof(typeof(retop), zv)},
                                  .op2_type = IS_UNUSED,
                              },
-                         [1].zv = {.u1.type_info = IS_NULL}};
+                         .zv = {.u1.type_info = IS_NULL}};
             // the race condition doesn't matter
-            if (!retop[0].zop.handler) {
-                zend_vm_set_opcode_handler(&retop[0].zop);
+            if (!retop.zop.handler) {
+                zend_vm_set_opcode_handler(&retop.zop);
             }
             struct {
                 zend_function new_func;
                 zend_function *orig_func; } *fs = emalloc(sizeof *fs);
             memcpy(&fs->new_func, execute_data->func, sizeof fs->new_func);
             fs->orig_func = execute_data->func;
             fs->new_func.op_array.last = 1;
-            fs->new_func.op_array.opcodes = (zend_op *)retop;
+            fs->new_func.op_array.opcodes = &retop.zop;
+            fs->new_func.op_array.opcodes = &retop.zop;
+            int zf_rid = zai_get_zend_func_rid(&execute_data->func->op_array);
+            if (zf_rid >= 0) {
+                fs->new_func.op_array.reserved[zf_rid] = 0;
+            }
             execute_data->func = &fs->new_func;
-            execute_data->opline = &retop[0].zop;
+#if PHP_VERSION_ID >= 80200
+            expected_ex = execute_data;
+            zend_atomic_bool_store_ex(&EG(vm_interrupt), true);
+#elif PHP_VERSION_ID >= 80000
+            expected_ex = execute_data;
+            EG(vm_interrupt) = 1;
+#else
+            execute_data->opline = &retop.zop;
+#endif
         } else {
-            // XXX: not supported yet
+            // TODO: not supported yet (JIT support appearing problematic)
         }
     }
 
@@ -390,7 +427,7 @@ static void dd_uhook_end(zend_ulong invocation, zend_execute_data *execute_data,
             efree(execute_data->func);
             execute_data->func = orig_func;
         } else {
-            // XXX: not supported yet
+            // TODO: not supported yet (JIT support appearing problematic)
         }
     }
 
@@ -850,6 +887,24 @@ ZEND_METHOD(DDTrace_HookData, overrideReturnValue) {
     RETURN_TRUE;
 }
 
+ZEND_METHOD(DDTrace_HookData, disableJitInlining) {
+    dd_hook_data *hookData = (dd_hook_data *)Z_OBJ_P(ZEND_THIS);
+
+    if (zend_parse_parameters_none() == FAILURE) {
+        return;
+    }
+
+    if (hookData->execute_data->func->type != ZEND_USER_FUNCTION) {
+        RETURN_FALSE;
+    }
+
+#if ZAI_JIT_BLACKLIST_ACTIVE
+    zai_jit_blacklist_function_inlining(&hookData->execute_data->func->op_array);
+#endif
+
+    RETURN_TRUE;
+}
+
 ZEND_METHOD(DDTrace_HookData, suppressCall) {
     dd_hook_data *hookData = (dd_hook_data *)Z_OBJ_P(ZEND_THIS);
 
@@ -862,7 +917,7 @@ ZEND_METHOD(DDTrace_HookData, suppressCall) {
     RETURN_TRUE;
 }
 
-ZEND_METHOD(DDTrace_HookData, enableAdviceOnRecursiveCall) {
+ZEND_METHOD(DDTrace_HookData, allowNestedHook) {
     dd_hook_data *hookData = (dd_hook_data *)Z_OBJ_P(ZEND_THIS);
 
     if (zend_parse_parameters_none() == FAILURE) {
@@ -975,6 +1030,12 @@ void zai_uhook_minit(int module_number) {
 
     efree(closure);
     EG(objects_store) = objects_store;
+
+    // We must have an interrupt function to be able to suppress calls
+#if PHP_VERSION_ID >= 80000
+    orig_zend_interrupt_function = zend_interrupt_function;
+    zend_interrupt_function = &dd_zend_interrupt_function;
+#endif
 }
 
 #if PHP_VERSION_ID >= 80000

ext/hook/uhook.stub.php

@@ -83,18 +83,25 @@ public function overrideReturnValue(mixed $value): bool;
      */
     public function overrideException(\Throwable|null $exception): bool;
 
+    /**
+     * Disables inlining of this method.
+     * @return bool true iif we have a user function
+     */
+    public function disableJitInlining(): bool;
+
     /**
      * Suppresses the call to the hooked function. Must be called within a pre-hook.
+     * The method disableJitInlining() should be called unconditionally in hooks using this method.
      * @return bool always 'true'
      */
     public function suppressCall(): bool;
 
     /**
-     * By default, advice is not called when the instrumented function from its advice.
-     * This method can be used to override this behavior.
-     * @return bool 'true' if called from the advice, which should always be the case
+     * By default, hooks are not called if the hooked function is called from the hook.
+     * This method can be used to override this behavior. The next recursive call will trigger the hook.
+     * @return bool 'true' if called from the hook, which should always be the case
      */
-    public function enableAdviceOnRecursiveCall(): bool;
+    public function allowNestedHook(): bool;
 
     /**
      * The name of the file where the function/method call was made from.

ext/hook/uhook_arginfo.h

@@ -1,5 +1,5 @@
 /* This is a generated file, edit the .stub.php file instead.
- * Stub hash: 8fa4825b6822c2bd3c0388cb1e7a5294cb212233 */
+ * Stub hash: f0aa3f3648af20b10037d7353c1fe7baa6b8e0d7 */
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_DDTrace_install_hook, 0, 1, IS_LONG, 0)
 	ZEND_ARG_OBJ_TYPE_MASK(0, target, Closure|Generator, MAY_BE_STRING|MAY_BE_CALLABLE, NULL)
@@ -31,10 +31,12 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_DDTrace_HookData_overrideE
 	ZEND_ARG_OBJ_INFO(0, exception, Throwable, 1)
 ZEND_END_ARG_INFO()
 
-ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_DDTrace_HookData_suppressCall, 0, 0, _IS_BOOL, 0)
+ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_DDTrace_HookData_disableJitInlining, 0, 0, _IS_BOOL, 0)
 ZEND_END_ARG_INFO()
 
-#define arginfo_class_DDTrace_HookData_enableAdviceOnRecursiveCall arginfo_class_DDTrace_HookData_suppressCall
+#define arginfo_class_DDTrace_HookData_suppressCall arginfo_class_DDTrace_HookData_disableJitInlining
+
+#define arginfo_class_DDTrace_HookData_allowNestedHook arginfo_class_DDTrace_HookData_disableJitInlining
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_class_DDTrace_HookData_getSourceFile, 0, 0, IS_STRING, 0)
 ZEND_END_ARG_INFO()
@@ -47,8 +49,9 @@ ZEND_METHOD(DDTrace_HookData, unlimitedSpan);
 ZEND_METHOD(DDTrace_HookData, overrideArguments);
 ZEND_METHOD(DDTrace_HookData, overrideReturnValue);
 ZEND_METHOD(DDTrace_HookData, overrideException);
+ZEND_METHOD(DDTrace_HookData, disableJitInlining);
 ZEND_METHOD(DDTrace_HookData, suppressCall);
-ZEND_METHOD(DDTrace_HookData, enableAdviceOnRecursiveCall);
+ZEND_METHOD(DDTrace_HookData, allowNestedHook);
 ZEND_METHOD(DDTrace_HookData, getSourceFile);
 
 
@@ -65,8 +68,9 @@ static const zend_function_entry class_DDTrace_HookData_methods[] = {
 	ZEND_ME(DDTrace_HookData, overrideArguments, arginfo_class_DDTrace_HookData_overrideArguments, ZEND_ACC_PUBLIC)
 	ZEND_ME(DDTrace_HookData, overrideReturnValue, arginfo_class_DDTrace_HookData_overrideReturnValue, ZEND_ACC_PUBLIC)
 	ZEND_ME(DDTrace_HookData, overrideException, arginfo_class_DDTrace_HookData_overrideException, ZEND_ACC_PUBLIC)
+	ZEND_ME(DDTrace_HookData, disableJitInlining, arginfo_class_DDTrace_HookData_disableJitInlining, ZEND_ACC_PUBLIC)
 	ZEND_ME(DDTrace_HookData, suppressCall, arginfo_class_DDTrace_HookData_suppressCall, ZEND_ACC_PUBLIC)
-	ZEND_ME(DDTrace_HookData, enableAdviceOnRecursiveCall, arginfo_class_DDTrace_HookData_enableAdviceOnRecursiveCall, ZEND_ACC_PUBLIC)
+	ZEND_ME(DDTrace_HookData, allowNestedHook, arginfo_class_DDTrace_HookData_allowNestedHook, ZEND_ACC_PUBLIC)
 	ZEND_ME(DDTrace_HookData, getSourceFile, arginfo_class_DDTrace_HookData_getSourceFile, ZEND_ACC_PUBLIC)
 	ZEND_FE_END
 };

src/Integrations/Integrations/Roadrunner/RoadrunnerIntegration.php

@@ -206,16 +206,13 @@ function (HookData $hook) use (&$activeSpan, &$suppressResponse, $integration, $
                         \DDTrace\close_span();
                         $activeSpan = null;
 
-                        // Ideally, I would call the method again to prevent the worker from
-                        // exiting after returning null, but this hook wouldn't be called for such
-                        // a recursive call
-                        $hook->enableAdviceOnRecursiveCall();
                         if ($recCall++ > 128) {
                             // too many recursive calls. Exit so that the worker can be restarted
                             $hook->overrideReturnValue(null);
                             $this->getWorker()->stop();
                             return;
                         }
+                        $hook->allowNestedHook();
                         $newRet = $this->waitRequest();
                         $hook->overrideReturnValue($newRet);
                         $recCall = 0;
@@ -235,6 +232,7 @@ static function ($res) use (&$activeSpan, &$suppressResponse, $thiz) {
 
         \DDTrace\install_hook('Spiral\RoadRunner\Http\HttpWorker::respond',
             function (HookData $hook) use (&$activeSpan, &$suppressResponse) {
+                $hook->disableJitInlining();
                 if (!$activeSpan || count($hook->args) < 3) {
                     return;
                 }

tests/ext/sandbox/install_hook/allow_nested_hook.phpt

@@ -0,0 +1,54 @@
+--TEST--
+allowNestedHook()
+--FILE--
+<?php
+function foo() {
+    var_dump('foo');
+}
+
+$count = 0;
+$hook = DDTrace\install_hook("foo", function($hook) use (&$count) {
+    var_dump("hook called: $count");
+    $count++;
+    if ($count == 1) {
+        $hook->allowNestedHook();
+        foo();
+    } else if ($count == 2) {
+        foo();
+    }
+});
+
+echo "Before hook:\n";
+foo();
+
+DDTrace\remove_hook($hook);
+
+$count = 0;
+$hook = DDTrace\install_hook("foo", function ($hook) {}, function($hook) use (&$count) {
+    var_dump("hook called: $count");
+    $count++;
+    if ($count == 1) {
+        $hook->allowNestedHook();
+        foo();
+    } else if ($count == 2) {
+        foo();
+    }
+});
+
+echo "After hook:\n";
+foo();
+
+?>
+--EXPECT--
+Before hook:
+string(14) "hook called: 0"
+string(14) "hook called: 1"
+string(3) "foo"
+string(3) "foo"
+string(3) "foo"
+After hook:
+string(3) "foo"
+string(14) "hook called: 0"
+string(3) "foo"
+string(14) "hook called: 1"
+string(3) "foo"