appsec: fix bailouts during rshutdown

appsec/src/extension/commands_helpers.c

@@ -179,7 +179,7 @@ static dd_result _dd_command_exec(dd_conn *nonnull conn,
                 "Response message for %.*s does not have the expected form",
                 NAME_L);
 
-            return dd_error;
+            return dd_network;
         }
         if (res != dd_success && res != dd_should_block &&
             res != dd_should_redirect && res != dd_should_record) {

appsec/src/extension/ddappsec.h

@@ -38,6 +38,7 @@ ZEND_BEGIN_MODULE_GLOBALS(ddappsec)
     bool to_be_configured : 1;
 
     bool skip_rshutdown : 1;
+    // used to avoid a bailout during request shutdown
     bool during_request_shutdown : 1;
 ZEND_END_MODULE_GLOBALS(ddappsec)
 // clang-format on

appsec/src/extension/request_lifecycle.c

@@ -283,7 +283,39 @@ static zend_array *nullable _do_request_begin(
     return spec;
 }
 
+static void _do_req_lifecycle_rshutdown(bool ignore_verdict, bool force);
+
 void dd_req_lifecycle_rshutdown(bool ignore_verdict, bool force)
+{
+    // temporarily remove the memory limit to avoid bailouts
+    // and as a safety net do a try-catch
+    __auto_type orig_mem_limit = PG(memory_limit);
+    zend_set_memory_limit((size_t)Z_L(-1) >> 1);
+
+    zend_try {
+        _do_req_lifecycle_rshutdown(ignore_verdict, force);
+    } zend_catch {
+        if (PG(last_error_message)) {
+            mlog_g(dd_log_error,
+                "Bailout in request shutdown; disconnecting from helper: %s",
+                ZSTR_VAL(PG(last_error_message)));
+        } else {
+            mlog_g(dd_log_error,
+                "Bailout in request shutdown; disconnecting from helper");
+        }
+        _reset_globals();
+        dd_helper_close_conn(); // note: not completely bailout-safe,
+                                // but should be fine with the raised mem limit
+    } zend_end_try();
+
+    __auto_type res = zend_set_memory_limit(orig_mem_limit);
+    if (res == FAILURE) {
+        mlog(dd_log_error,
+            "Failed to restore memory limit in request shutdown");
+    }
+}
+
+static void _do_req_lifecycle_rshutdown(bool ignore_verdict, bool force)
 {
     if (DDAPPSEC_G(enabled) == APPSEC_FULLY_DISABLED) {
         mlog_g(dd_log_debug, "Skipping all request shutdown actions because "
@@ -437,6 +469,7 @@ static zend_array *_do_request_finish_user_req(bool ignore_verdict,
     return spec;
 }
 
+// Should be bailout-safe -- means, in particular, no emalloc allocations
 static void _reset_globals(void)
 {
     _set_cur_span(NULL);
@@ -491,7 +524,10 @@ static void _set_cur_span(zend_object *nullable span)
     }
 }
 
-bool dd_req_lifecycle_is_active(void) { return _between_init_shutdown_msgs; }
+bool dd_req_lifecycle_is_active(void)
+{
+    return _between_init_shutdown_msgs && DDAPPSEC_G(active);
+}
 
 zend_object *nullable dd_req_lifecycle_get_cur_span(void)
 {

appsec/src/extension/user_tracking.c

@@ -228,6 +228,12 @@ void dd_find_and_apply_verdict_for_user(zend_string *nullable user_id,
         return;
     }
 
+    if (!dd_req_lifecycle_is_active()) {
+        mlog_g(dd_log_info,
+            "Not running inside a tracked request; skipping user verdict");
+        return;
+    }
+
     dd_conn *conn = dd_helper_mgr_cur_conn();
     if (conn == NULL) {
         mlog(dd_log_debug, "No connection; unable to check user");

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

@@ -0,0 +1,49 @@
+package com.datadog.appsec.php.docker
+
+/**
+ * A log file inside an {@link AppSecContainer}. Supports marking the current
+ * end-of-file position and then reading only what was appended since the mark,
+ * which is the usual way a test isolates the log output of a single action.
+ *
+ * <pre>
+ *   def lf = new LogFile(container, 'helper.log')
+ *   lf.markEndPos()
+ *   ... do something that writes to the log ...
+ *   lf.linesSinceMark().any { it.contains('boom') }
+ * </pre>
+ *
+ * A {@code name} without a leading {@code /} is resolved under {@link #LOG_DIR}.
+ */
+class LogFile {
+    static final String LOG_DIR = '/tmp/logs'
+
+    private final AppSecContainer container
+    final String path
+    private long markPos = 0
+
+    LogFile(AppSecContainer container, String name) {
+        this.container = container
+        this.path = name.startsWith('/') ? name : "${LOG_DIR}/${name}"
+    }
+
+    /** Current size of the log in bytes (0 if it does not exist yet). */
+    long size() {
+        container.execInContainer('bash', '-c', "wc -c < ${path} 2>/dev/null || echo 0".toString())
+                .stdout.trim() as long
+    }
+
+    /** Record the current end position; subsequent reads start from here. */
+    void markEndPos() {
+        markPos = size()
+    }
+
+    /** Raw text appended since the last {@link #markEndPos()}. */
+    String getTextSinceMark() {
+        container.execInContainer('bash', '-c', "tail -c +${markPos + 1} ${path}".toString()).stdout
+    }
+
+    /** Lines appended since the last {@link #markEndPos()}. */
+    List<String> getLinesSinceMark() {
+        getTextSinceMark().readLines()
+    }
+}

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

@@ -0,0 +1,97 @@
+package com.datadog.appsec.php.docker
+
+import groovy.util.logging.Slf4j
+import org.testcontainers.containers.Container.ExecResult
+
+/**
+ * Helpers for reconfiguring PHP-FPM inside a running {@link AppSecContainer}.
+ *
+ * Two strategies are offered:
+ * <ul>
+ *   <li>{@link #restart} hard-kills every php-fpm process and relaunches the
+ *       master, optionally exporting extra environment variables or loading a
+ *       different php.ini. Use it for changes that only take effect on a cold
+ *       start (environment variables, php.ini edits).</li>
+ *   <li>{@link #reload} sends SIGUSR2 for a graceful reload and blocks until the
+ *       pre-reload workers have been replaced. Use it for pool.d settings (e.g.
+ *       {@code pm.max_children}) that FPM re-reads on USR2; pair it with
+ *       {@link #setPoolValue} / {@link #backupPoolConfig} / {@link #restorePoolConfig}.</li>
+ * </ul>
+ */
+@Slf4j
+class PhpFpm {
+    static final String FPM_CONF = '/etc/php-fpm.conf'
+    static final String DEFAULT_INI = '/etc/php/php.ini'
+    static final String POOL_CONF = '/etc/php-fpm.d/www.conf'
+
+    private final AppSecContainer container
+
+    PhpFpm(AppSecContainer container) {
+        this.container = container
+    }
+
+    /**
+     * Hard-restart php-fpm: kill every php-fpm process and relaunch the master.
+     *
+     * @param env extra environment variables exported before launch
+     * @param iniPath the php.ini to load (defaults to {@link #DEFAULT_INI})
+     */
+    ExecResult restart(Map<String, String> env = [:], String iniPath = DEFAULT_INI) {
+        container.flushProfilingData()
+        String exports = env.collect { k, v -> "export ${k}=${v};" }.join(' ')
+        ExecResult res = container.execInContainer('bash', '-c',
+                "kill -9 `pgrep php-fpm`; ${exports} php-fpm -y ${FPM_CONF} -c ${iniPath}".toString())
+        assert res.exitCode == 0 : "php-fpm restart failed: ${res.stderr}"
+        res
+    }
+
+    /** Rewrite a single pool directive (e.g. {@code pm.max_children}) in place. */
+    void setPoolValue(String key, String value, String poolConf = POOL_CONF) {
+        container.execInContainer('sed', '-i', "s/${key} = .*/${key} = ${value}/".toString(), poolConf)
+    }
+
+    /** Back up the pool config so {@link #restorePoolConfig} can revert any edits. */
+    void backupPoolConfig(String poolConf = POOL_CONF) {
+        container.execInContainer('cp', poolConf, "${poolConf}.bak".toString())
+    }
+
+    /** Restore the pool config saved by {@link #backupPoolConfig}. */
+    void restorePoolConfig(String poolConf = POOL_CONF) {
+        container.execInContainer('mv', "${poolConf}.bak".toString(), poolConf)
+    }
+
+    /**
+     * Gracefully reload the FPM master (re-reads pool config without dropping the
+     * socket) and block until every pre-reload worker has been replaced by a
+     * freshly-spawned one.
+     */
+    void reload(long timeoutMillis = 5_000) {
+        List<String> old = workerPids()
+        // Locate the master via its pid file, falling back to the oldest php-fpm process.
+        container.execInContainer('bash', '-c',
+                'kill -USR2 $(cat /run/php-fpm*.pid /var/run/php-fpm*.pid 2>/dev/null | head -1) ' +
+                '2>/dev/null || pkill -USR2 -o php-fpm || true')
+        waitForWorkerTurnover(old, timeoutMillis)
+    }
+
+    /** PIDs of the current pool worker processes (the master is excluded). */
+    List<String> workerPids() {
+        container.execInContainer('bash', '-c', "pgrep -f 'php-fpm: pool' || true")
+                .stdout.readLines()*.trim().findAll { it }
+    }
+
+    private void waitForWorkerTurnover(List<String> old, long timeoutMillis) {
+        long deadline = System.currentTimeMillis() + timeoutMillis
+        while (true) {
+            List<String> current = workerPids()
+            if (!current.isEmpty() && current.intersect(old).isEmpty()) {
+                return
+            }
+            if (System.currentTimeMillis() > deadline) {
+                throw new IllegalStateException(
+                        "php-fpm workers were not reloaded in time (old=${old}, current=${current})".toString())
+            }
+            Thread.sleep(100)
+        }
+    }
+}

appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/Apache2FpmTests.groovy

@@ -3,6 +3,7 @@ package com.datadog.appsec.php.integration
 import com.datadog.appsec.php.docker.AppSecContainer
 import com.datadog.appsec.php.docker.FailOnUnmatchedTraces
 import com.datadog.appsec.php.docker.InspectContainerHelper
+import com.datadog.appsec.php.docker.PhpFpm
 import groovy.util.logging.Slf4j
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.condition.DisabledIf
@@ -79,21 +80,11 @@ class Apache2FpmTests implements CommonTests, SamplingTestsInFpm, EndpointFallba
     }
 
     void setRateLimit(String limit) {
-        flushProfilingData()
-        def res = container.execInContainer(
-                'bash', '-c',
-                """kill -9 `pgrep php-fpm`;
-               export DD_APPSEC_TRACE_RATE_LIMIT=$limit;
-               php-fpm -y /etc/php-fpm.conf -c /etc/php/php.ini""")
-        assert res.exitCode == 0
+        new PhpFpm(container).restart([DD_APPSEC_TRACE_RATE_LIMIT: limit])
     }
 
     private void resetFpm() {
-        flushProfilingData()
-        container.execInContainer(
-                'bash', '-c',
-                '''kill -9 `pgrep php-fpm`;
-               php-fpm -y /etc/php-fpm.conf -c /etc/php/php.ini''')
+        new PhpFpm(container).restart()
     }
 
     @Test

appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/EndpointFallbackSamplingTests.groovy

@@ -1,6 +1,7 @@
 package com.datadog.appsec.php.integration
 
 import com.datadog.appsec.php.docker.AppSecContainer
+import com.datadog.appsec.php.docker.PhpFpm
 import org.junit.jupiter.api.Test
 
 import java.net.http.HttpResponse
@@ -110,12 +111,6 @@ trait EndpointFallbackSamplingTests extends SamplingTestsInFpm {
     }
 
     void disableEndpointRenaming() {
-        flushProfilingData()
-        def res = container.execInContainer(
-                'bash', '-c',
-                '''kill -9 `pgrep php-fpm`;
-               export DD_TRACE_RESOURCE_RENAMING_ENABLED=false;
-               php-fpm -y /etc/php-fpm.conf -c /etc/php/php.ini''')
-        assert res.exitCode == 0
+        new PhpFpm(container).restart([DD_TRACE_RESOURCE_RENAMING_ENABLED: 'false'])
     }
 }

appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/NginxFpmTests.groovy

@@ -3,7 +3,10 @@ package com.datadog.appsec.php.integration
 import com.datadog.appsec.php.docker.AppSecContainer
 import com.datadog.appsec.php.docker.FailOnUnmatchedTraces
 import com.datadog.appsec.php.docker.InspectContainerHelper
+import com.datadog.appsec.php.docker.LogFile
+import com.datadog.appsec.php.docker.PhpFpm
 import groovy.util.logging.Slf4j
+import org.junit.jupiter.api.Assumptions
 import org.junit.jupiter.api.Tag
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.condition.DisabledIf
@@ -14,6 +17,7 @@ import java.net.http.HttpResponse
 
 import static com.datadog.appsec.php.integration.TestParams.getPhpVersion
 import static com.datadog.appsec.php.integration.TestParams.getVariant
+import static java.net.http.HttpResponse.BodyHandlers.ofString
 
 @Testcontainers
 @Slf4j
@@ -47,4 +51,72 @@ class NginxFpmTests implements CommonTests {
         }
     }
 
+    /**
+     * Regression test: OOM inside dd_entity_body_convert() (called from
+     * dd_request_shutdown() via _request_pack()) triggers zend_bailout(), which
+     * the per-module zend_try swallows, so the helper never gets RequestShutdown
+     * and sees an out-of-order RequestInit on the next request.
+     */
+    @Test
+    void 'no unexpected RequestInit due to RSHUTDOWN OOM bail'() {
+        Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null,
+                'the C++ helper silently swallows out-of-order commands.')
+        // PHP 8.3 release only (zts already excluded at class level): the debug
+        // allocator's heap-protection turns the mid-allocation OOM bailout into
+        // a spurious "zend_mm_heap corrupted" SIGABRT that masks the real bug.
+        Assumptions.assumeTrue(phpVersion == '8.3' && !variant.contains('debug'),
+                'requires a PHP 8.3 release build')
+
+        // Drop the pool to a single worker so the OOM request and the follow-up
+        // land on the same FPM process / helper socket.
+        PhpFpm fpm = new PhpFpm(container)
+        fpm.backupPoolConfig()
+
+        try {
+            fpm.setPoolValue('pm.max_children', '1')
+            fpm.reload()
+
+            LogFile helperLog = new LogFile(container, 'helper.log')
+            helperLog.markEndPos()
+
+            // Warm-up: establish the helper connection so it is in its outer loop
+            // waiting for request_init before the OOM request.
+            container.traceFromRequest('/hello.php') { HttpResponse<InputStream> resp ->
+                assert resp.statusCode() == 200
+            }
+
+            // Trigger Pattern B: rshutdown_oom.php sets a 32M memory_limit, pins
+            // ~28 MiB live in $GLOBALS, then emits ~400 KiB of JSON. During
+            // RSHUTDOWN, dd_request_shutdown()'s _request_pack() callback parses
+            // that body via dd_entity_body_convert(), overflowing the ceiling;
+            // zend_bailout() fires before _omsg_send(), so the socket is untouched.
+            container.traceFromRequest('/rshutdown_oom.php', ofString()) {
+                HttpResponse<String> resp ->
+                    // The script completes (OOM happens during RSHUTDOWN, after the
+                    // response is on the wire).
+                    assert resp.statusCode() == 200
+            }
+
+            // originally, the would actually fail here because the bailout during
+            // rshutdown would skip _reset_globals() and _cur_req_span would not be
+            // reset. This would either lead to a crash (502), or, if the same slot
+            // was used for the span in the next request, for the span to be
+            // prematurely deleted on the next request as _cur_req_span was being
+            // replaced
+            container.traceFromRequest('/hello.php') { HttpResponse<InputStream> resp ->
+                assert resp.statusCode() == 200
+            }
+
+            List<String> lines = helperLog.linesSinceMark
+            assert !lines.any { it.contains('unexpected command RequestInit') } :
+                    'Error message found. Relevant helper log:\n' +
+                    lines.findAll {
+                        it.contains('unexpected command') || it.contains('error in request loop')
+                    }.join('\n')
+        } finally {
+            fpm.restorePoolConfig()
+            fpm.reload()
+        }
+    }
+
 }