wip

appsec/tests/integration/README.md

@@ -0,0 +1,60 @@
+# AppSec Integration Tests
+
+This is a gradle project. The gradle files are divided into two parts:
+
+* `build.gradle` — contains the main tasks for running the tests.
+* `gradle/images.gradle` — contains the tasks for building the docker images.
+
+Generally, you don't need to build the docker images yourself, only if you need
+to change them.
+
+## Running the tests
+
+To run all the tests, do `./gradlew check`.
+
+To run tests for a specific PHP version, do e.g. `./gradlew test8.3-release`
+
+To run a specific test suite, do e.g.
+`./gradlew test8.3-release --tests RoadRunnerTests`
+
+To run a specific test, do e.g.
+`./gradlew test8.3-release --tests "com.datadog.appsec.php.integration.RoadRunnerTests.blocking json on request start"`
+(you need to provide the fully qualified name of the test class here).
+
+The tracer and appsec modules are built automatically, if required.
+
+You can attach a Java debugger with `--debug-jvm`. E.g.:
+`./gradlew test8.3-release --tests RoadRunnerTests --debugJvm`.
+
+You can attach a PHP debugger with `-PXDEBUG=1`. E.g.:
+`./gradlew test8.3-release --tests RoadRunnerTests -PXDEBUG=1`.
+
+It can be also helpful to run gradle with `--info`. This will show the output of
+the tests in the console. Or you can look at the html test report afterwards.
+
+Some test classes also have a `main()` method so that you can run the container
+without running the tests against it. Generally, changes you make to the PHP
+files also become instantly visible in the container, though with roadrunner you
+will need to kill the existing workers. Do:
+
+```bash
+./gradlew runMain8.3-release -PtestClass=com.datadog.appsec.php.integration.RoadRunnerTests
+```
+
+Don't forget to the testClass property. Otherwise, the task won't even be
+created.
+
+## Building the images
+
+These can be fetched from docker hub, but if you need to build them yourself,
+do `./gradlew buildAll`. You can also build a specific image. You can see the
+list of these individual tasks with `./gradlew tasks --all`.
+
+## Cleaning
+
+If for some reason you need to clean the builds of the tracer or appSec or some
+state for a specific test (e.g. composer packages), you can remove the
+corresponding volumes, (see `docker volume ls`).
+
+You can also completely clean the project with `./gradlew clean`.
+

appsec/tests/integration/build.gradle

@@ -330,18 +330,33 @@ def runUnitTestsTask = { String phpVersion, String variant ->
     }
 }
 
-['7.0', '7.1', '7.2', '7.3', '7.4', '8.0', '8.1', '8.2'].each { phpVersion ->
-    ['release', 'release-zts'].each { variant ->
-        buildTracerTask(phpVersion, variant)
-        buildAppSecTask(phpVersion, variant)
-        runUnitTestsTask(phpVersion, variant)
+def runMainTask = { String phpVersion, String variant ->
+    tasks.create("runMain$phpVersion-$variant", JavaExec) {
+        mainClass = project.property('testClass')
+        classpath = sourceSets.test.runtimeClasspath
+        standardInput = System.in
+
+        if (project.hasProperty('XDEBUG')) {
+            systemProperty 'XDEBUG', '1'
+        }
+        systemProperty 'PHP_VERSION', phpVersion
+        systemProperty 'VARIANT', variant
+
+        dependsOn "buildTracer-$phpVersion-$variant"
+        dependsOn "buildAppsec-$phpVersion-$variant"
     }
 }
 
 testMatrix.each { spec ->
-    def phpVersion = spec[0]
-    def variant = spec[1]
-
+    String phpVersion = spec[0]
+    String variant = spec[1]
+
+    buildTracerTask(phpVersion, variant)
+    buildAppSecTask(phpVersion, variant)
+    runUnitTestsTask(phpVersion, variant)
+    if (project.hasProperty('testClass')) {
+        runMainTask(phpVersion, variant)
+    }
 
     def task = tasks.register("test${phpVersion}-$variant", Test) {
         group = 'Verification'

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) {

appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/AppSecContainer.groovy

@@ -71,6 +71,7 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
         withEnv 'DD_SERVICE', 'appsec_int_tests'
         withEnv 'DD_ENV', 'integration'
         withEnv 'DD_TRACE_AGENT_FLUSH_AFTER_N_REQUESTS', '0'
+        withEnv 'DD_TRACE_AGENT_FLUSH_INTERVAL', '0'
         withEnv 'DD_TRACE_DEBUG', '1'
         withEnv 'DD_AUTOLOAD_NO_COMPILE', 'true' // must be exactly 'true'
         if (System.getProperty('XDEBUG') == '1') {

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

@@ -64,6 +64,7 @@ typedef struct {
     bool *running_ptr;
     bool returns_reference;
     bool suppress_call;
+    bool dis_jit_inlining_called;
 } dd_hook_data;
 
 #define EXCEPTION_OVERRIDE_CLEAR ((zend_object *)0x1)
@@ -218,6 +219,25 @@ 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) {
+        if (ex == expected_ex) {
+            ex->opline = ex->func->op_array.opcodes;
+        }
+
+        expected_ex = NULL;
+    }
+
+    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 +280,46 @@ 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;
+#if ZAI_JIT_BLACKLIST_ACTIVE
+            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;
+            }
+#endif
             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 +424,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,19 +884,48 @@ 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;
+    }
+
+    hookData->dis_jit_inlining_called = true;
+
+    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);
 
     if (zend_parse_parameters_none() == FAILURE) {
         return;
     }
 
+    if (!hookData->dis_jit_inlining_called) {
+        LOG(Error, "suppressCall called without disableJitInlining before");
+    }
+
+    if (hookData->execute_data->func->type != ZEND_USER_FUNCTION) {
+        LOG(Error, "suppressCall is only supported for user functions");
+        RETURN_FALSE;
+    }
+
     hookData->suppress_call = true;
 
     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 +1038,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
 };