Skip to content

Commit a52b578

Browse files
authored
Merge pull request #3886 from DataDog/glopes/fix-gshutdown-crash
pass ddtrace_globals explicitly through sidecar/telemetry GSHUTDOWN
2 parents d8164b5 + 847641e commit a52b578

11 files changed

Lines changed: 244 additions & 21 deletions

File tree

.gitlab/generate-appsec.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@
129129
TERM=dumb ./gradlew loadCaches --info
130130
fi
131131

132-
TERM=dumb ./gradlew $targets --info -Pbuildscan --scan $HELPER_RUST_FLAG
132+
TERM=dumb ./gradlew $targets --info -Pbuildscan --scan -PcheckCoreDumps $HELPER_RUST_FLAG
133133
TERM=dumb ./gradlew saveCaches --info
134134
after_script:
135135
- mkdir -p "${CI_PROJECT_DIR}/artifacts"
@@ -343,7 +343,7 @@
343343
# Build helper-rust with coverage instrumentation
344344
TERM=dumb ./gradlew buildHelperRustWithCoverage --info -Pbuildscan --scan
345345
# Run integration tests with coverage-instrumented binary
346-
TERM=dumb ./gradlew test8.3-debug --info -Pbuildscan --scan -PuseHelperRustCoverage
346+
TERM=dumb ./gradlew test8.3-debug --info -Pbuildscan --scan -PcheckCoreDumps -PuseHelperRustCoverage
347347
# Generate coverage report from profraw files
348348
TERM=dumb ./gradlew generateHelperRustIntegrationCoverage --info -Pbuildscan --scan
349349
TERM=dumb ./gradlew saveCaches --info

appsec/tests/integration/build.gradle

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,9 @@ def runMainTask = { String phpVersion, String variant ->
620620
if (project.hasProperty('XDEBUG')) {
621621
systemProperty 'XDEBUG', '1'
622622
}
623+
if (project.hasProperty('checkCoreDumps')) {
624+
systemProperty 'checkCoreDumps', '1'
625+
}
623626
systemProperty 'PHP_VERSION', phpVersion
624627
systemProperty 'VARIANT', variant
625628

@@ -693,6 +696,9 @@ def runMainTask = { String phpVersion, String variant ->
693696
if (project.hasProperty('XDEBUG')) {
694697
it.systemProperty 'XDEBUG', '1'
695698
}
699+
if (project.hasProperty('checkCoreDumps')) {
700+
it.systemProperty 'checkCoreDumps', '1'
701+
}
696702
if (project.hasProperty('helperBinary')) {
697703
it.systemProperty 'USE_HELPER_RUST', '1'
698704
it.systemProperty 'HELPER_BINARY_PATH', project.getProperty('helperBinary')
@@ -801,6 +807,9 @@ if (project.hasProperty('testClass')) {
801807
if (project.hasProperty('XDEBUG')) {
802808
it.systemProperty 'XDEBUG', '1'
803809
}
810+
if (project.hasProperty('checkCoreDumps')) {
811+
it.systemProperty 'checkCoreDumps', '1'
812+
}
804813
if (project.hasProperty('helperBinary')) {
805814
it.systemProperty 'USE_HELPER_RUST', '1'
806815
it.systemProperty 'HELPER_BINARY_PATH', project.getProperty('helperBinary')

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import com.github.dockerjava.api.command.CreateContainerCmd
1010
import com.github.dockerjava.api.command.ExecCreateCmdResponse
1111
import com.github.dockerjava.api.exception.NotFoundException
1212
import com.github.dockerjava.api.model.Bind
13+
import com.github.dockerjava.api.model.Ulimit
1314
import com.github.dockerjava.api.model.Volume
1415
import com.google.common.util.concurrent.SettableFuture
1516
import groovy.json.JsonOutput
@@ -49,8 +50,10 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
4950
private File logsDir
5051
// Set to true to include sidecar.log in stdout (very verbose)
5152
private static final boolean TAIL_SIDECAR_LOG = false
53+
private static final boolean CHECK_CORE_DUMPS = System.getProperty('checkCoreDumps') != null
5254
private String wwwDir
5355
private String wwwSrcDir
56+
private String savedCorePattern = null
5457
public final HttpClient httpClient = HttpClient.newBuilder()
5558
.followRedirects(HttpClient.Redirect.NEVER)
5659
.connectTimeout(Duration.ofSeconds(5))
@@ -110,6 +113,7 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
110113
this.logConsumer = createLogConsumer()
111114
followOutput(logConsumer)
112115
overlayWww()
116+
enableCoredumps()
113117
runInitialize()
114118
}
115119

@@ -315,11 +319,69 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
315319
}
316320
}
317321

322+
private void enableCoredumps() {
323+
if (!CHECK_CORE_DUMPS) {
324+
return
325+
}
326+
try {
327+
ExecResult res = execInContainer('cat', '/proc/sys/kernel/core_pattern')
328+
if (res.exitCode == 0) {
329+
savedCorePattern = res.stdout.trim()
330+
}
331+
execInContainer('sh', '-c',
332+
'mkdir -p /tmp/cores && chmod 1777 /tmp/cores' +
333+
' && echo /tmp/cores/core.%e.%p.%t > /proc/sys/kernel/core_pattern')
334+
} catch (Exception e) {
335+
log.warn("Could not enable coredumps: {}", e.message)
336+
}
337+
}
338+
339+
private List<String> detectCrashes() {
340+
if (!CHECK_CORE_DUMPS) {
341+
return []
342+
}
343+
List<String> crashes = []
344+
try {
345+
ExecResult res = execInContainer('find', '/tmp/cores', '-type', 'f')
346+
if (res.exitCode == 0 && res.stdout.trim()) {
347+
res.stdout.trim().readLines()*.trim().findAll { it }.each { String f ->
348+
crashes << "Core dump: $f".toString()
349+
log.error("Crash core dump found: {}", f)
350+
}
351+
}
352+
} catch (Exception e) {
353+
log.warn("Could not check for core dumps: {}", e.message)
354+
}
355+
crashes
356+
}
357+
358+
void clearCoreFiles() {
359+
execInContainer('sh', '-c', 'rm -f /tmp/cores/core.*')
360+
}
361+
362+
private void restoreCorePattern() {
363+
if (savedCorePattern != null) {
364+
try {
365+
execInContainer('sh', '-c',
366+
"printf '%s' '${savedCorePattern}' > /proc/sys/kernel/core_pattern")
367+
} catch (Exception e) {
368+
log.warn("Could not restore core pattern: {}", e.message)
369+
}
370+
}
371+
}
372+
318373
void close() {
319374
flushProfilingData()
320375
copyLogs()
376+
List<String> crashes = detectCrashes()
377+
restoreCorePattern()
321378
mockDatadogAgent.drainTraces()
322379
super.close()
380+
if (crashes) {
381+
throw new AssertionError(
382+
("Process crash(es) detected in container during test run (${crashes.size()} crash(es)):\n" +
383+
crashes.join('\n')).toString())
384+
}
323385
}
324386

325387
private static final Random RAND = new Random()
@@ -440,6 +502,10 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
440502

441503
privilegedMode = true
442504

505+
withCreateContainerCmdModifier { cmd ->
506+
cmd.hostConfig.withUlimits([new Ulimit('core', -1L, -1L)] as Ulimit[])
507+
}
508+
443509
this.wwwDir ="src/test/www/${options.get('www', 'base')}"
444510
if (options['www_src']) {
445511
this.wwwSrcDir = "src/test/www/${options['www_src']}"
@@ -630,5 +696,18 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
630696
copyFileFromContainer(it, new File(logsDir, new File(it).name).absolutePath)
631697
}
632698
}
699+
700+
if (CHECK_CORE_DUMPS) {
701+
ExecResult coresRes = execInContainer('find', '/tmp/cores', '-type', 'f')
702+
if (coresRes.exitCode == 0) {
703+
coresRes.stdout.eachLine {
704+
it = it.trim()
705+
if (it) {
706+
log.info("Copying core dump: {}", it)
707+
copyFileFromContainer(it, new File(logsDir, new File(it).name).absolutePath)
708+
}
709+
}
710+
}
711+
}
633712
}
634713
}
Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
package com.datadog.appsec.php.integration
2+
3+
import com.datadog.appsec.php.docker.AppSecContainer
4+
import com.datadog.appsec.php.docker.InspectContainerHelper
5+
import groovy.util.logging.Slf4j
6+
import org.junit.jupiter.api.Test
7+
import org.junit.jupiter.api.condition.EnabledIf
8+
import org.testcontainers.junit.jupiter.Container
9+
import org.testcontainers.junit.jupiter.Testcontainers
10+
11+
import java.net.http.HttpResponse
12+
13+
import static com.datadog.appsec.php.integration.TestParams.getPhpVersion
14+
import static com.datadog.appsec.php.integration.TestParams.getVariant
15+
import static org.testcontainers.containers.Container.ExecResult
16+
17+
/**
18+
* Regression test for a ZTS-only crash in PHP_GSHUTDOWN_FUNCTION(ddtrace).
19+
*
20+
* Under Apache MPM event with MaxConnectionsPerChild 1, worker threads are
21+
* cancelled without calling tsrm_thread_exit(). PHP's ts_free_id then iterates
22+
* every thread's TSRM storage from the main thread and invokes
23+
* zm_globals_dtor_ddtrace for each per-thread slot.
24+
*
25+
* Only runs on ZTS variants (MPM event is only used on ZTS), PHP >= 7.4, and
26+
* only when -PcheckCoreDumps is passed.
27+
*
28+
* PHP 7.0-7.3 is excluded because of a PHP bug in zend_llist_destroy: it does
29+
* not null out the head/tail pointers after freeing elements. When
30+
* php_request_shutdown() calls php_shutdown_ticks() -> zend_llist_destroy(),
31+
* the tick-functions list elements are freed but head is left dangling. The
32+
* subsequent call to php_shutdown_ticks() from core_globals_dtor() (via
33+
* ts_free_id) then hits a double-free -> SIGABRT. (There remains the bug that
34+
* shutdown_ticks() should not refer to PG() from GSHUTDOWN, but at least
35+
* PHP >= 7.4 doesn't crash).
36+
*/
37+
@Testcontainers
38+
@Slf4j
39+
@EnabledIf('isZtsAndCheckCoreDumps')
40+
class ZtsGshutdownTests {
41+
/** Only enabled on ZTS variants, PHP >= 7.4, and when -PcheckCoreDumps is passed. */
42+
static boolean isZtsAndCheckCoreDumps() {
43+
variant.contains('zts') &&
44+
System.getProperty('checkCoreDumps') != null &&
45+
phpVersion >= '7.4'
46+
}
47+
48+
@Container
49+
public static final AppSecContainer CONTAINER =
50+
new AppSecContainer(
51+
workVolume: this.name,
52+
baseTag: 'apache2-mod-php',
53+
phpVersion: phpVersion,
54+
phpVariant: variant,
55+
www: 'base',
56+
)
57+
.withEnv('DD_CRASHTRACKING_ENABLED', '0')
58+
.withEnv('DD_INSTRUMENTATION_TELEMETRY_ENABLED', '0')
59+
60+
static void main(String[] args) {
61+
InspectContainerHelper.run(CONTAINER)
62+
}
63+
64+
@Test
65+
void 'no crash during GSHUTDOWN when MaxConnectionsPerChild 1 triggers ZTS worker lifecycle'() {
66+
long errorLogOffset = (CONTAINER.execInContainer('sh', '-c',
67+
'stat -c %s /tmp/logs/apache2/error.log 2>/dev/null || echo 0')
68+
.stdout.trim() as long)
69+
70+
ExecResult backupResult = CONTAINER.execInContainer('sh', '-c',
71+
'cp /etc/apache2/mods-enabled/mpm_event.conf /etc/apache2/mods-enabled/mpm_event.conf.bak_zts')
72+
assert backupResult.exitCode == 0
73+
74+
try {
75+
// Append MaxConnectionsPerChild 1 + KeepAlive Off so each TCP connection
76+
// causes Apache to call clean_child_exit(), which destroys the APR child
77+
// pool and triggers PHP MSHUTDOWN + GSHUTDOWN.
78+
ExecResult cfgResult = CONTAINER.execInContainer('sh', '-c',
79+
'OLD=$(pgrep -P $(pgrep -f /usr/sbin/apache2 | head -1))' +
80+
' && echo "MaxConnectionsPerChild 1\nKeepAlive Off" >> /etc/apache2/mods-enabled/mpm_event.conf' +
81+
' && apache2ctl restart' +
82+
' && for p in $OLD; do while kill -0 $p 2>/dev/null; do sleep 0.05; done; done')
83+
assert cfgResult.exitCode == 0: "apache2 config failed: ${cfgResult.stderr}"
84+
85+
String apacheParent = CONTAINER.execInContainer('sh', '-c',
86+
'pgrep -f /usr/sbin/apache2 | head -1').stdout.trim()
87+
88+
for (int i = 0; i < 3; i++) {
89+
// Snapshot workers before the request — MaxConnectionsPerChild 1
90+
// means exactly one worker will call clean_child_exit() after
91+
// responding, running PHP MSHUTDOWN/GSHUTDOWN before it exits.
92+
Set<String> workersBefore = CONTAINER.execInContainer('sh', '-c',
93+
"pgrep -P $apacheParent").stdout.trim().readLines().toSet()
94+
95+
CONTAINER.traceFromRequest('/hello.php', { HttpResponse<InputStream> resp ->
96+
assert resp.statusCode() == 200: "request ${i} failed: ${resp.statusCode()}"
97+
})
98+
99+
// Wait until at least one pre-request worker has exited, confirming
100+
// its GSHUTDOWN completed before we inspect for crashes.
101+
long deadline = System.currentTimeMillis() + 10_000
102+
while (System.currentTimeMillis() < deadline) {
103+
Set<String> workersNow = CONTAINER.execInContainer('sh', '-c',
104+
"pgrep -P $apacheParent").stdout.trim().readLines().toSet()
105+
if (!workersNow.containsAll(workersBefore)) break
106+
Thread.sleep(100)
107+
}
108+
}
109+
110+
// Core dump detection is handled automatically by AppSecContainer.close().
111+
// Additionally check Apache's error.log for crashes that generate SIGABRT
112+
// before a core dump can be written (e.g. Rust allocator panics on
113+
// poisoned memory).
114+
ExecResult logCheck = CONTAINER.execInContainer('sh', '-c',
115+
"tail -c +${errorLogOffset + 1} /tmp/logs/apache2/error.log")
116+
String errorLog = logCheck.stdout ?: ''
117+
assert !errorLog.contains('exit signal Aborted'):
118+
"Apache worker exited via SIGABRT during GSHUTDOWN:\n" + errorLog
119+
assert !errorLog.contains('exit signal Segmentation'):
120+
"Apache worker segfaulted during GSHUTDOWN:\n" + errorLog
121+
} finally {
122+
CONTAINER.execInContainer('sh', '-c',
123+
'cp /etc/apache2/mods-enabled/mpm_event.conf.bak_zts' +
124+
' /etc/apache2/mods-enabled/mpm_event.conf' +
125+
' && apache2ctl restart')
126+
}
127+
}
128+
}

ext/ddtrace.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -706,7 +706,7 @@ static PHP_GSHUTDOWN_FUNCTION(ddtrace) {
706706
zend_hash_destroy(&ddtrace_globals->git_metadata);
707707

708708
// Drop the per-thread sidecar transport (thread-lifetime, one per thread).
709-
ddtrace_sidecar_gshutdown();
709+
ddtrace_sidecar_gshutdown(ddtrace_globals);
710710

711711
tsrm_mutex_free(ddtrace_globals->sidecar_universal_service_tags_mutex);
712712

ext/ddtrace.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,10 @@ ZEND_END_MODULE_GLOBALS(ddtrace)
204204
# endif
205205
extern TSRM_TLS void *ATTR_TLS_GLOBAL_DYNAMIC TSRMLS_CACHE;
206206
# define DDTRACE_G(v) ZEND_TSRMG(ddtrace_globals_id, zend_ddtrace_globals *, v)
207+
# define DDTRACE_GLOBALS_PTR() TSRMG_BULK_STATIC(ddtrace_globals_id, zend_ddtrace_globals *)
207208
#else
208209
# define DDTRACE_G(v) (ddtrace_globals.v)
210+
# define DDTRACE_GLOBALS_PTR() (&ddtrace_globals)
209211
#endif
210212

211213
#define PHP_DDTRACE_EXTNAME "ddtrace"

ext/sidecar.c

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -875,16 +875,18 @@ void ddtrace_sidecar_rshutdown(void) {
875875
ddog_Vec_Tag_drop(DDTRACE_G(active_global_tags));
876876
}
877877

878-
void ddtrace_sidecar_gshutdown(void) {
879-
if (DDTRACE_G(sidecar)) {
880-
if (DDTRACE_G(sidecar) == ddtrace_sidecar_for_signal) {
878+
void ddtrace_sidecar_gshutdown(zend_ddtrace_globals *ddtrace_globals) {
879+
// NOTE: do not use DDTRACE_G() in this function; it may be called from the
880+
// main thread via ts_free_id()
881+
if (ddtrace_globals->sidecar) {
882+
if (ddtrace_globals->sidecar == ddtrace_sidecar_for_signal) {
881883
ddtrace_sidecar_for_signal = NULL;
882884
}
883885

884886
// Drain any accumulated background-sender metrics before the transport goes away.
885-
ddtrace_telemetry_flush_bgs_metrics_final();
886-
ddog_sidecar_transport_drop(DDTRACE_G(sidecar));
887-
DDTRACE_G(sidecar) = NULL;
887+
ddtrace_telemetry_flush_bgs_metrics_final(ddtrace_globals);
888+
ddog_sidecar_transport_drop(ddtrace_globals->sidecar);
889+
ddtrace_globals->sidecar = NULL;
888890
}
889891
}
890892

ext/sidecar.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ void ddtrace_sidecar_send_debugger_datum(ddog_DebuggerPayload *payload);
6464
void ddtrace_sidecar_activate(void);
6565
void ddtrace_sidecar_rinit(void);
6666
void ddtrace_sidecar_rshutdown(void);
67-
void ddtrace_sidecar_gshutdown(void);
67+
void ddtrace_sidecar_gshutdown(zend_ddtrace_globals *ddtrace_globals);
6868

6969
void ddtrace_sidecar_dogstatsd_count(zend_string *metric, zend_long value, zval *tags);
7070
void ddtrace_sidecar_dogstatsd_distribution(zend_string *metric, double value, zval *tags);

0 commit comments

Comments
 (0)