wip

appsec/src/extension/php_compat.c

@@ -20,7 +20,8 @@ static const uint32_t uninitialized_bucket[-HT_MIN_MASK] = {
     HT_INVALID_IDX, HT_INVALID_IDX};
 
 const HashTable zend_empty_array = {.gc.refcount = 2,
-    .gc.u.type_info = IS_ARRAY,
+    .gc.u.v.type = IS_ARRAY,
+    .gc.u.v.flags = IS_ARRAY_IMMUTABLE,
     .u.flags =
         HASH_FLAG_STATIC_KEYS | HASH_FLAG_INITIALIZED | HASH_FLAG_PERSISTENT,
     .nTableMask = HT_MIN_MASK,

appsec/src/extension/php_compat.h

@@ -38,8 +38,22 @@ extern const HashTable zend_empty_array;
 
 #    define GC_ADDREF(x) (++GC_REFCOUNT(x))
 #    define GC_DELREF(x) (--GC_REFCOUNT(x))
-#    define GC_TRY_ADDREF GC_ADDREF
-#    define GC_TRY_DELREF GC_DELREF
+static zend_always_inline void _gc_try_addref(zend_refcounted_h *_rc)
+{
+    struct _zend_refcounted *rc = (struct _zend_refcounted *)_rc;
+    if (!(GC_FLAGS(rc) & IS_ARRAY_IMMUTABLE)) {
+        GC_ADDREF(rc);
+    }
+}
+#    define GC_TRY_ADDREF(p) _gc_try_addref(&(p)->gc)
+static zend_always_inline void _gc_try_delref(zend_refcounted_h *_rc)
+{
+    struct _zend_refcounted *rc = (struct _zend_refcounted *)_rc;
+    if (!(GC_FLAGS(rc) & IS_ARRAY_IMMUTABLE)) {
+        GC_DELREF(rc);
+    }
+}
+#    define GC_TRY_DELREF(p) _gc_try_delref(&(p)->gc)
 
 zend_bool zend_ini_parse_bool(zend_string *str);
 #   define zend_string_efree zend_string_free
@@ -56,19 +70,21 @@ static inline HashTable *zend_new_array(uint32_t nSize) {
 #    define tsrm_env_unlock()
 #endif
 
-#if PHP_VERSION_ID >= 70300 && PHP_VERSION_ID < 80100
+#if PHP_VERSION_ID >= 70300 && PHP_VERSION_ID < 80000
 static zend_always_inline void _gc_try_addref(zend_refcounted_h *rc)
 {
     if (!(rc->u.type_info & GC_IMMUTABLE)) {
         rc->refcount++;
     }
 }
+#define GC_TRY_ADDREF(p) _gc_try_addref(&(p)->gc)
+#endif
+#if PHP_VERSION_ID >= 70300 && PHP_VERSION_ID < 80100
 static zend_always_inline void _gc_try_delref(zend_refcounted_h *rc)
 {
     if (!(rc->u.type_info & GC_IMMUTABLE)) {
         rc->refcount--;
     }
 }
-#define GC_TRY_ADDREF(p) _gc_try_addref(&(p)->gc)
 #define GC_TRY_DELREF(p) _gc_try_delref(&(p)->gc)
 #endif

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

appsec/valgrind.supp

@@ -172,16 +172,18 @@
    fun:php_load_extension
    ...
 }
+
 {
-   <leak of unknown nature>
+   <leak after bailout>
    Memcheck:Leak
    match-leak-kinds: definite
+   fun:malloc
    ...
-   fun:__zend_malloc
    fun:zend_compile
    fun:compile_file
-   fun:zend_e
-   fun:php_request_startup
+   fun:zend_execute_scripts
+   fun:php_execute_script
+   ...
 }
 {
    <regex uninitialised move from tracer>
@@ -191,3 +193,22 @@
    fun:zm_activate_ddtrace
    ...
 }
+{
+   <reserve_rehash>
+   Memcheck:Leak
+   match-leak-kinds: possible
+   fun:malloc
+   fun:_ZN9hashbrown3raw21RawTable$LT$T$C$A$GT$14reserve_rehash17h273179532303855bE
+   ...
+}
+{
+   <arg error bailout>
+   Memcheck:Leak
+   match-leak-kinds: definite
+   fun:malloc
+   ...
+   fun:zend_throw_exception
+   fun:zend_type_error
+   fun:zend_verify_arg_error
+   ...
+}

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;
 }