From 2eded5701475a2601d0d941a918f46aa70420ad2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Andr=C3=A9=20dos=20Santos=20Lopes?= Date: Fri, 29 May 2026 17:47:29 +0100 Subject: [PATCH 1/2] appsec: fix bailouts during rshutdown --- appsec/src/extension/ddappsec.h | 1 + appsec/src/extension/request_lifecycle.c | 38 +++++++- appsec/src/extension/user_tracking.c | 6 ++ .../datadog/appsec/php/docker/LogFile.groovy | 49 ++++++++++ .../datadog/appsec/php/docker/PhpFpm.groovy | 97 +++++++++++++++++++ .../php/integration/Apache2FpmTests.groovy | 15 +-- .../EndpointFallbackSamplingTests.groovy | 9 +- .../php/integration/NginxFpmTests.groovy | 72 ++++++++++++++ .../php/integration/RoadRunnerTests.groovy | 52 ++++++++-- .../php/integration/SamplingTestsInFpm.groovy | 15 +-- .../php/integration/ZtsGshutdownTests.groovy | 18 ++-- .../src/PostRespondTrackUserHandler.php | 23 +++++ .../test/www/base/public/rshutdown_oom.php | 66 +++++++++++++ .../src/test/www/roadrunner/worker.php | 1 + 14 files changed, 413 insertions(+), 49 deletions(-) create mode 100644 appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/LogFile.groovy create mode 100644 appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/PhpFpm.groovy create mode 100644 appsec/tests/integration/src/test/www/_handlers/src/PostRespondTrackUserHandler.php create mode 100644 appsec/tests/integration/src/test/www/base/public/rshutdown_oom.php diff --git a/appsec/src/extension/ddappsec.h b/appsec/src/extension/ddappsec.h index c19fa82618e..17d219f5060 100644 --- a/appsec/src/extension/ddappsec.h +++ b/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 diff --git a/appsec/src/extension/request_lifecycle.c b/appsec/src/extension/request_lifecycle.c index 4d9cb27088f..c627ac2ee0b 100644 --- a/appsec/src/extension/request_lifecycle.c +++ b/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) { diff --git a/appsec/src/extension/user_tracking.c b/appsec/src/extension/user_tracking.c index e1a865ee3d8..2ec46df104e 100644 --- a/appsec/src/extension/user_tracking.c +++ b/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"); diff --git a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/LogFile.groovy b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/LogFile.groovy new file mode 100644 index 00000000000..db95a1c2267 --- /dev/null +++ b/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. + * + * 
+ *   def lf = new LogFile(container, 'helper.log')
+ *   lf.markEndPos()
+ *   ... do something that writes to the log ...
+ *   lf.linesSinceMark().any { it.contains('boom') }
+ *
+ * + * 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 getLinesSinceMark() { + getTextSinceMark().readLines() + } +} diff --git a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/PhpFpm.groovy b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/PhpFpm.groovy new file mode 100644 index 00000000000..e7c768b4f89 --- /dev/null +++ b/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: + * + */ +@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 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 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 workerPids() { + container.execInContainer('bash', '-c', "pgrep -f 'php-fpm: pool' || true") + .stdout.readLines()*.trim().findAll { it } + } + + private void waitForWorkerTurnover(List old, long timeoutMillis) { + long deadline = System.currentTimeMillis() + timeoutMillis + while (true) { + List 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) + } + } +} diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/Apache2FpmTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/Apache2FpmTests.groovy index 9be5159b761..e92c05240b2 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/Apache2FpmTests.groovy +++ b/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 diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/EndpointFallbackSamplingTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/EndpointFallbackSamplingTests.groovy index 6c051a9ebd9..d9c32c1e6b8 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/EndpointFallbackSamplingTests.groovy +++ b/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']) } } diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/NginxFpmTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/NginxFpmTests.groovy index 8b22701d5d6..606b091b8ed 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/NginxFpmTests.groovy +++ b/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 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 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 resp -> + assert resp.statusCode() == 200 + } + + List 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() + } + } + } diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy index 51ade591d40..04ae7007b68 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/RoadRunnerTests.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.LogFile import groovy.util.logging.Slf4j import org.junit.jupiter.api.Assumptions import org.junit.jupiter.api.BeforeAll @@ -62,8 +63,8 @@ class RoadRunnerTests implements WorkerStrategyTests { Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null, 'This bug only manifests on the Rust helper (strict outer/inner loop state machine).') - long logOffset = (CONTAINER.execInContainer('bash', '-c', - 'wc -c < /tmp/logs/helper.log').stdout.trim() as long) + LogFile helperLog = new LogFile(CONTAINER, 'helper.log') + helperLog.markEndPos() // PostRespondLfiHandler sets a callback that calls fopen('../etc/passwd') // after respond() returns. By that point, request_shutdown has been sent @@ -78,15 +79,52 @@ class RoadRunnerTests implements WorkerStrategyTests { assert resp.statusCode() == 200 } - String helperLog = CONTAINER.execInContainer('bash', '-c', - "tail -c +${logOffset + 1} /tmp/logs/helper.log").stdout + List lines = helperLog.linesSinceMark + log.info("Helper log since offset:\n{}", lines.join('\n')) - log.info("Helper log since offset:\n{}", helperLog) + assert !lines.any { it.contains('unexpected command RequestExec') } : + "Helper received RequestExec in outer loop. " + + "Relevant log:\n" + + lines.findAll { + it.contains('unexpected command') || it.contains('error in request loop') + }.join('\n') + } + + /** + * Regression test for the AppSec helper "unexpected command RequestExec" bug, + * variant where the post-respond RequestExec sender is the user-tracking SDK + * (track_user_login_success -> dd_find_and_apply_verdict_for_user) rather than + * push_addresses. + */ + @Test + void 'no unexpected RequestExec in outer loop after post-respond track_user_login'() { + Assumptions.assumeTrue(System.getProperty('USE_HELPER_RUST') != null, + 'This bug only manifests on the Rust helper (strict outer/inner loop state machine).') + + LogFile helperLog = new LogFile(CONTAINER, 'helper.log') + helperLog.markEndPos() + + // PostRespondTrackUserHandler sets a callback that calls + // track_user_login_success() after respond() returns. By that point, + // request_shutdown has been sent via the response_committed hook. If + // dd_find_and_apply_verdict_for_user still reaches the helper (socket + // open, active=true), it sends RequestExec into the outer loop. + CONTAINER.traceFromRequest('/post-respond-track-user') { HttpResponse resp -> + assert resp.statusCode() == 200 + } + + // Follow-up request verifies the connection is still usable. + CONTAINER.traceFromRequest('/') { HttpResponse resp -> + assert resp.statusCode() == 200 + } + + List lines = helperLog.linesSinceMark + log.info("Helper log since offset:\n{}", lines.join('\n')) - assert !helperLog.contains('unexpected command RequestExec') : + assert !lines.any { it.contains('unexpected command RequestExec') } : "Helper received RequestExec in outer loop. " + "Relevant log:\n" + - helperLog.readLines().findAll { + lines.findAll { it.contains('unexpected command') || it.contains('error in request loop') }.join('\n') } diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SamplingTestsInFpm.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SamplingTestsInFpm.groovy index 7fbea7b5ec3..6a7b8f643df 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SamplingTestsInFpm.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/SamplingTestsInFpm.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 com.datadog.appsec.php.model.Trace import org.junit.jupiter.api.Test @@ -102,21 +103,11 @@ trait SamplingTestsInFpm { } void setSamplingPeriod(String period) { - flushProfilingData() - def res = container.execInContainer( - 'bash', '-c', - """kill -9 `pgrep php-fpm`; - export DD_API_SECURITY_SAMPLE_DELAY=$period; - php-fpm -y /etc/php-fpm.conf -c /etc/php/php.ini""") - assert res.exitCode == 0 + new PhpFpm(container).restart([DD_API_SECURITY_SAMPLE_DELAY: period]) } 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() } void flushProfilingData() { diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/ZtsGshutdownTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/ZtsGshutdownTests.groovy index 2552a9dfd84..a8fc83ceb00 100644 --- a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/ZtsGshutdownTests.groovy +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/ZtsGshutdownTests.groovy @@ -2,6 +2,7 @@ package com.datadog.appsec.php.integration import com.datadog.appsec.php.docker.AppSecContainer import com.datadog.appsec.php.docker.InspectContainerHelper +import com.datadog.appsec.php.docker.LogFile import groovy.util.logging.Slf4j import org.junit.jupiter.api.Test import org.junit.jupiter.api.condition.EnabledIf @@ -63,9 +64,8 @@ class ZtsGshutdownTests { @Test void 'no crash during GSHUTDOWN when MaxConnectionsPerChild 1 triggers ZTS worker lifecycle'() { - long errorLogOffset = (CONTAINER.execInContainer('sh', '-c', - 'stat -c %s /tmp/logs/apache2/error.log 2>/dev/null || echo 0') - .stdout.trim() as long) + LogFile errorLog = new LogFile(CONTAINER, 'apache2/error.log') + errorLog.markEndPos() ExecResult backupResult = CONTAINER.execInContainer('sh', '-c', 'cp /etc/apache2/mods-enabled/mpm_event.conf /etc/apache2/mods-enabled/mpm_event.conf.bak_zts') @@ -111,13 +111,11 @@ class ZtsGshutdownTests { // Additionally check Apache's error.log for crashes that generate SIGABRT // before a core dump can be written (e.g. Rust allocator panics on // poisoned memory). - ExecResult logCheck = CONTAINER.execInContainer('sh', '-c', - "tail -c +${errorLogOffset + 1} /tmp/logs/apache2/error.log") - String errorLog = logCheck.stdout ?: '' - assert !errorLog.contains('exit signal Aborted'): - "Apache worker exited via SIGABRT during GSHUTDOWN:\n" + errorLog - assert !errorLog.contains('exit signal Segmentation'): - "Apache worker segfaulted during GSHUTDOWN:\n" + errorLog + String errorLogText = errorLog.getTextSinceMark() + assert !errorLogText.contains('exit signal Aborted'): + "Apache worker exited via SIGABRT during GSHUTDOWN:\n" + errorLogText + assert !errorLogText.contains('exit signal Segmentation'): + "Apache worker segfaulted during GSHUTDOWN:\n" + errorLogText } finally { CONTAINER.execInContainer('sh', '-c', 'cp /etc/apache2/mods-enabled/mpm_event.conf.bak_zts' + diff --git a/appsec/tests/integration/src/test/www/_handlers/src/PostRespondTrackUserHandler.php b/appsec/tests/integration/src/test/www/_handlers/src/PostRespondTrackUserHandler.php new file mode 100644 index 00000000000..cfa55535915 --- /dev/null +++ b/appsec/tests/integration/src/test/www/_handlers/src/PostRespondTrackUserHandler.php @@ -0,0 +1,23 @@ + 'text/plain'], 'OK'); + } +} diff --git a/appsec/tests/integration/src/test/www/base/public/rshutdown_oom.php b/appsec/tests/integration/src/test/www/base/public/rshutdown_oom.php new file mode 100644 index 00000000000..3c565281b6f --- /dev/null +++ b/appsec/tests/integration/src/test/www/base/public/rshutdown_oom.php @@ -0,0 +1,66 @@ +addRoute('/json', new \App\JsonHandler()); $router->addRoute('/xml', new \App\XmlHandler()); $router->addRoute('/post-respond-lfi', new \App\PostRespondLfiHandler()); +$router->addRoute('/post-respond-track-user', new \App\PostRespondTrackUserHandler()); while ($req = $httpWorker->waitRequest()) { /** @var \Spiral\RoadRunner\Http\Request $req */ From 9cdff756695f62239aa0f0560a8cbbbb38f4f387 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Andr=C3=A9=20dos=20Santos=20Lopes?= Date: Fri, 29 May 2026 18:17:40 +0100 Subject: [PATCH 2/2] appsec: treat post-response framing errors as dd_network After a successful command exchange (e.g. request_init), the helper has entered its inner request loop and is waiting for the matching request_shutdown. If _imsg_destroy() reports a msgpack framing error at that point, the code was returning dd_error. dd_error does not trigger dd_helper_close_conn(), so the connection stays open while the helper is blocked in the inner loop. On the next request the extension sends request_init into the inner loop, which the Rust helper treats as an unexpected command and aborts. Return dd_network instead, which causes the caller to close and abandon the connection. This keeps helper and extension state in sync even in the presence of malformed trailers. Co-Authored-By: Claude Opus 4.8 --- appsec/src/extension/commands_helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appsec/src/extension/commands_helpers.c b/appsec/src/extension/commands_helpers.c index f589b94d0b5..efc4eee3d5c 100644 --- a/appsec/src/extension/commands_helpers.c +++ b/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) {