8000 Fix crash during preloading on 8.1+ · DataDog/dd-trace-php@01e983f · GitHub
[go: up one dir, main page]
Skip to content

Commit 01e983f

Browse files
bwoebibwoebi
committed
Fix crash during preloading on 8.1+
Fixes #1795. Since 8.1.0 opcache will free our objects before module_shutdown during preloading. Work around it by intercepting the latest possible point in time before objects freeing, then do our normal rshutdown sequence early. Adding asan for opcache tests. Looking at asan output, also some small changes had to be done to zai config and hooking. Signed-off-by: Bob Weinand <bob.weinand@datadoghq.com>
1 parent 01c6bd1 commit 01e983f

File tree

6 files changed

+90
-75
lines changed

6 files changed

+90
-75
lines changed

‎.circleci/config.yml

Lines changed: 10 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -391,27 +391,6 @@ jobs:
391391
name: Test post-install hook
392392
command: bash tests/PostInstallHook/run-tests.sh
393393

394-
opcache_tests:
395-
parameters:
396-
docker_image:
397-
type: string
398-
working_directory: ~/datadog
399-
executor:
400-
name: with_agent
401-
docker_image: << parameters.docker_image >>
402-
steps:
403-
- <<: *STEP_ATTACH_WORKSPACE
404-
- <<: *STEP_EXT_INSTALL
405-
- <<: *STEP_EXPORT_CI_ENV
406-
- <<: *STEP_PREPARE_TEST_RESULTS_DIR
407-
- run:
408-
name: Run opcache tests
409-
command: |
410-
export REPORT_EXIT_STATUS=1
411-
export DD_TRACE_CLI_ENABLED=1
412-
cd tmp/build_extension
413-
php run-tests.php -g FAIL,XFAIL,BORK,WARN,LEAK,XLEAK,SKIP -p $(which php) --show-all -d zend_extension=opcache.so ../../tests/opcache
414-
415394
xdebug_tests:
416395
parameters:
417396
docker_image:
@@ -482,13 +461,14 @@ jobs:
482461
command: |
483462
set -euo pipefail
484463
make << parameters.make_target >> PHPUNIT_OPTS="--log-junit test-results/php-unit/results.xml" 2>&1 | tee /dev/stderr | { ! grep -qe "=== Total [0-9]+ memory leaks detected ==="; }
464+
rm -rf tmp/build_extension/tests/opcache/file_cache/* || true
485465
- <<: *STEP_PERSIST_TO_WORKSPACE
486466
- <<: *STEP_STORE_TEST_RESULTS
487467
- run:
488468
command: |
489469
mkdir -p /tmp/artifacts/core_dumps
490470
find tmp -name "core.*" | xargs -I % -n 1 cp % /tmp/artifacts/core_dumps
491-
cp -a tmp/build_extension/tests/ext /tmp/artifacts/tests
471+
cp -a tmp/build_extension/tests/$(if [[ << parameters.make_target >> == *opcache* ]]; then echo opcache; else echo ext; fi) /tmp/artifacts/tests
492472
when: on_fail
493473
- store_artifacts:
494474
path: /tmp/artifacts
@@ -2752,6 +2732,7 @@ workflows:
< 6D40 /code>
27522732
- test_c2php
27532733
- test_c_disabled
27542734
- test_internal_api_randomized
2735+
- test_opcache
27552736
- coverage:
27562737
requires: [ 'Prepare Code' ]
27572738
matrix:
@@ -2783,6 +2764,13 @@ workflows:
27832764
- test_c_asan
27842765
- test_with_init_hook_asan
27852766
- test_internal_api_randomized
2767+
- test_opcache_asan
2768+
exclude:
2769+
# apparently for no discernible reason, on x86 asan builds PHP 7.4. fails to allocate opcache shared memory
2770+
- php_major_minor: '7.4'
2771+
resource_class: medium
2772+
make_target: test_opcache_asan
2773+
switch_php_version: debug-zts-asan
27862774

27872775
- integration:
27882776
requires: [ 'Prepare Code' ]
@@ -2857,19 +2845,6 @@ workflows:
28572845
make_target:
28582846
- test_integrations_phpredis5
28592847

2860-
- opcache_tests:
2861-
requires: [ 'Prepare Code' ]
2862-
matrix:
2863-
parameters:
2864-
docker_image:
2865-
- datadog/dd-trace-ci:php-7.0_buster
2866-
- datadog/dd-trace-ci:php-7.1_buster
2867-
- datadog/dd-trace-ci:php-7.2_buster
2868-
- datadog/dd-trace-ci:php-7.3_buster
2869-
- datadog/dd-trace-ci:php-7.4_buster
2870-
- datadog/dd-trace-ci:php-8.0_buster
2871-
- datadog/dd-trace-ci:php-8.1_buster
2872-
28732848
- xdebug_tests:
28742849
requires: [ 'Prepare Code' ]
28752850
name: "PHP 70 Xdebug tests"

‎Makefile

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ RUN_TESTS_CMD := REPORT_EXIT_STATUS=1 TEST_PHP_SRCDIR=$(PROJECT_ROOT) USE_TRACKE
3636

3737
C_FILES := $(shell find components ext src/dogstatsd zend_abstract_interface -name '*.c' -o -name '*.h' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
3838
TEST_FILES := $(shell find tests/ext -name '*.php*' -o -name '*.inc' -o -name '*.json' -o -name 'CONFLICTS' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
39+
TEST_OPCACHE_FILES := $(shell find tests/opcache -name '*.php*' -o -name '.gitkeep' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
3940
TEST_STUB_FILES := $(shell find tests/ext -type d -name 'stubs' -exec find '{}' -type f \; | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
4041
INIT_HOOK_TEST_FILES := $(shell find tests/C2PHP -name '*.phpt' -o -name '*.inc' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
4142
M4_FILES := $(shell find m4 -name '*.m4*' | awk '{ printf "$(BUILD_DIR)/%s\n", $$1 }' )
@@ -98,21 +99,21 @@ install_all: install install_ini
9899
run_tests: $(TEST_FILES) $(TEST_STUB_FILES) $(BUILD_DIR)/configure
99100
$(RUN_TESTS_CMD) $(BUILD_DIR)/$(TESTS)
100101

101-
test_c: export DD_TRACE_CLI_ENABLED=1
102102
test_c: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES)
103-
$(RUN_TESTS_CMD) -d extension=$(SO_FILE) $(BUILD_DIR)/$(TESTS)
103+
DD_TRACE_CLI_ENABLED=1 $(RUN_TESTS_CMD) -d extension=$(SO_FILE) $(BUILD_DIR)/$(TESTS)
104104

105105
test_c_coverage: dist_clean
106106
DD_TRACE_DOCKER_DEBUG=1 EXTRA_CFLAGS="-fprofile-arcs -ftest-coverage" $(MAKE) test_c || exit 0
107107

108-
test_c_disabled: export DD_TRACE_CLI_ENABLED=0
109-
test_c_disabled: export DD_TRACE_DEBUG=1
110108
test_c_disabled: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES)
111109
( \
112-
$(RUN_TESTS_CMD) -d extension=$(SO_FILE) $(BUILD_DIR)/$(TESTS) || true; \
110+
DD_TRACE_CLI_ENABLED=0 DD_TRACE_DEBUG=1 $(RUN_TESTS_CMD) -d extension=$(SO_FILE) $(BUILD_DIR)/$(TESTS) || true; \
113111
! grep -E 'Successfully triggered flush with trace of size|=== Total [0-9]+ memory leaks detected ===|Segmentation fault' $$(find $(BUILD_DIR)/$(TESTS) -name "*.out" | grep -v segfault_backtrace_enabled.out); \
114112
)
115113

114+
test_opcache: $(SO_FILE) $(TEST_OPCACHE_FILES)
115+
DD_TRACE_CLI_ENABLED=1 $(RUN_TESTS_CMD) -d extension=$(SO_FILE) -d zend_extension=opcache.so $(BUILD_DIR)/tests/opcache
116+
116117
test_c_mem: export DD_TRACE_CLI_ENABLED=1
117118
test_c_mem: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES)
118119
$(RUN_TESTS_CMD) -d extension=$(SO_FILE) -m $(BUILD_DIR)/$(TESTS)
@@ -136,13 +137,20 @@ test_with_init_hook_asan: $(SO_FILE) $(INIT_HOOK_TEST_FILES)
136137
$(RUN_TESTS_CMD) -d extension=$(SO_FILE) -d ddtrace.request_init_hook=$$(pwd)/bridge/dd_wrap_autoloader.php --asan $(INIT_HOOK_TEST_FILES); \
137138
)
138139

139-
test_c_asan: export DD_TRACE_CLI_ENABLED=1
140140
test_c_asan: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES)
141141
( \
142142
set -xe; \
143143
export TEST_PHP_JUNIT=$(JUNIT_RESULTS_DIR)/asan-extension-test.xml; \
144144
$(MAKE) -C $(BUILD_DIR) CFLAGS="-g -fsanitize=address" LDFLAGS="-fsanitize=address" clean all; \
145-
$(RUN_TESTS_CMD) -d extension=$(SO_FILE) --asan $(BUILD_DIR)/$(TESTS); \
145+
USE_ZEND_ALLOC=0 DD_TRACE_CLI_ENABLED=1 $(RUN_TESTS_CMD) -d extension=$(SO_FILE) --asan $(BUILD_DIR)/$(TESTS); \
146+
)
147+
148+
test_opcache_asan: $(SO_FILE) $(TEST_OPCACHE_FILES)
149+
( \
150+
set -xe; \
151+
export TEST_PHP_JUNIT=$(JUNIT_RESULTS_DIR)/asan-extension-test.xml; \
152+
$(MAKE) -C $(BUILD_DIR) CFLAGS="-g -fsanitize=address" LDFLAGS="-fsanitize=address" clean all; \
153+
USE_ZEND_ALLOC=0 DD_TRACE_CLI_ENABLED=1 $(RUN_TESTS_CMD) -d extension=$(SO_FILE) -d zend_extension=opcache.so $(BUILD_DIR)/tests/opcache \
146154
)
147155

148156
test_extension_ci: $(SO_FILE) $(TEST_FILES) $(TEST_STUB_FILES)
@@ -304,7 +312,7 @@ build_components_coverage:
304312
)
305313

306314
test_components_coverage: build_components_coverage
307-
$(MAKE) -C $(COMPONENTS_BUILD_DIR) test $(shell [ -z "${TESTS}"] || echo "ARGS='--test-dir ${TESTS}'") && ! grep -e "=== Total .* memory leaks detected ===" $(COMPONENTS_BUILD_DIR)/Testing/Temporary/LastTest.log
315+
$(MAKE) -C $(COMPONENTS_BUILD_DIR) test $(shell [ -z "${TESTS}" ] || echo "ARGS='--test-dir ${TESTS}'") && ! grep -e "=== Total .* memory leaks detected ===" $(COMPONENTS_BUILD_DIR)/Testing/Temporary/LastTest.log
308316

309317
test_coverage_collect:
310318
$(Q) lcov \

‎ext/ddtrace.c

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,6 @@ static void ddtrace_activate(void) {
204204

205205
if (DDTRACE_G(disable)) {
206206
ddtrace_disable_tracing_in_current_request();
207-
} else {
208-
zai_hook_activate();
209207
}
210208

211209
DDTRACE_G(request_init_hook_loaded) = 0;
@@ -830,6 +828,11 @@ static PHP_RINIT_FUNCTION(ddtrace) {
830828
zai_interceptor_rinit();
831829
#endif
832830

831+
if (!DDTRACE_G(disable)) {
832+
// With internal functions also being hookable, they must not be hooked before the CG(map_ptr_base) is zeroed
833+
zai_hook_activate();
834+
}
835+
833836
if (get_DD_TRACE_ENABLED()) {
834837
dd_initialize_request();
835838
}
@@ -879,19 +882,7 @@ static void dd_shutdown_hooks_and_observer(void) {
879882
#endif
880883
}
881884

882-
static PHP_RSHUTDOWN_FUNCTION(ddtrace) {
883-
UNUSED(module_number, type);
884-
885-
zend_hash_destroy(&DDTRACE_G(traced_spans));
886-
887-
if (!get_DD_TRACE_ENABLED()) {
888-
if (!DDTRACE_G(disable)) {
889-
dd_shutdown_hooks_and_observer();
890-
}
891-
892-
return SUCCESS;
893-
}
894-
885+
void dd_force_shutdown_tracing(void) {
895886
DDTRACE_G(in_shutdown) = true;
896887

897888
ddtrace_close_all_open_spans(true); // All remaining userland spans (and root span)
@@ -906,6 +897,22 @@ static PHP_RSHUTDOWN_FUNCTION(ddtrace) {
906897
dd_shutdown_hooks_and_observer();
907898

908899
DDTRACE_G(in_shutdown) = false;
900+
}
901+
902+
static PHP_RSHUTDOWN_FUNCTION(ddtrace) {
903+
UNUSED(module_number, type);
904+
905+
zend_hash_destroy(&DDTRACE_G(traced_spans));
906+
907+
if (!get_DD_TRACE_ENABLED()) {
908+
if (!DDTRACE_G(disable)) {
909+
dd_shutdown_hooks_and_observer();
910+
}
911+
912+
return SUCCESS;
913+
}
914+
F438 915+
dd_force_shutdown_tracing();
909916

910917
return SUCCESS;
911918
}

‎ext/ddtrace.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ void dd_prepare_for_new_trace(void);
6565
void ddtrace_disable_tracing_in_current_request(void);
6666
bool ddtrace_alter_dd_trace_disabled_config(zval *old_value, zval *new_value);
6767
bool ddtrace_alter_sampling_rules_file_config(zval *old_value, zval *new_value);
68+
void dd_force_shutdown_tracing(void);
6869

6970
typedef struct {
7071
int type;

‎ext/handlers_exception.c

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
#include <exceptions/exceptions.h>
33
#include <php.h>
44

5+
#include "configuration.h"
56
#include "engine_hooks.h" // For 'ddtrace_resource'
67
#include "handlers_internal.h"
78
#include "serializer.h"
@@ -13,7 +14,8 @@
1314
// Adding support for both is open to future scope, should the need arise.
1415

1516
static zend_class_entry dd_exception_or_error_handler_ce;
16-
static zend_object_handlers dd_exception_or_error_handler_handlers;
17+
static zend_object_handlers dd_exception_handler_handlers;
18+
static zend_object_handlers dd_error_handler_handlers;
1719

1820
static zval *dd_exception_or_error_handler_handler(zend_object *obj) { return OBJ_PROP_NUM(obj, 0); }
1921

@@ -136,19 +138,11 @@ static PHP_FUNCTION(ddtrace_http_response_code) {
136138
dd_check_exception_in_header(old_response_code);
137139
}
138140

139-
#if PHP_VERSION_ID < 80000
140-
#define GC_NOT_COLLECTABLE GC_IMMUTABLE
141-
#endif
142-
143141
// inject ourselves into a set_exception_handler
144142
static void dd_wrap_exception_or_error_handler(zval *target, zend_bool is_error_handler) {
145143
zval wrapper;
146144
object_init_ex(&wrapper, &dd_exception_or_error_handler_ce);
147-
if (is_error_handler) {
148-
// this does not participate in GC, so fine to mark it that way, to distinguish from exception handler
149-
GC_ADD_FLAGS(Z_OBJ(wrapper), GC_NOT_COLLECTABLE);
150-
}
151-
Z_OBJ(wrapper)->handlers = &dd_exception_or_error_handler_handlers;
145+
Z_OBJ(wrapper)->handlers = is_error_handler ? &dd_error_handler_handlers : &dd_exception_handler_handlers;
152146
ZVAL_COPY_VALUE(dd_exception_or_error_handler_handler(Z_OBJ(wrapper)), target);
153147
ZVAL_COPY_VALUE(target, &wrapper);
154148
}
@@ -204,7 +198,7 @@ ZEND_END_ARG_INFO()
204198
#endif
205199
static PHP_METHOD(DDTrace_ExceptionOrErrorHandler, execute) {
206200
volatile zend_bool has_bailout = false;
207-
zend_bool is_error_handler = (GC_FLAGS(Z_OBJ_P(ZEND_THIS)) & GC_NOT_COLLECTABLE) != 0;
201+
zend_bool is_error_handler = Z_OBJ_P(ZEND_THIS)->handlers == &dd_error_handler_handlers;
208202
zval *handler = dd_exception_or_error_handler_handler(Z_OBJ_P(ZEND_THIS));
209203

210204
if (is_error_handler) {
@@ -358,6 +352,19 @@ static int dd_exception_handler_get_closure(zend_object *obj, zend_class_entry *
358352
return SUCCESS;
359353
}
360354

355+
#if PHP_VERSION_ID >= 80100
356+
void dd_exception_handler_freed(zend_object *object) {
357+
zend_object_std_dtor(object);
358+
359+
if (!EG(current_execute_data) && get_DD_TRACE_ENABLED()) {
360+
// Here we are at the very last chance before objects are unconditionally freed.
361+
// Let's force-disable the tracing in case it wasn't yet
362+
// Typically RSHUTDOWN would handle that, but since 8.1.0 opcache will free our objects before module_shutdown during preloading
363+
dd_force_shutdown_tracing();
364+
}
365+
}
366+
#endif
367+
361368
void ddtrace_exception_handlers_startup(void) {
362369
ddtrace_exception_or_error_handler = (zend_internal_function){
363370
.type = ZEND_INTERNAL_FUNCTION,
@@ -373,8 +380,12 @@ void ddtrace_exception_handlers_startup(void) {
373380
zend_initialize_class_data(&dd_exception_or_error_handler_ce, false);
374381
dd_exception_or_error_handler_ce.info.internal.module = &ddtrace_module_entry;
375382
zend_declare_property_null(&dd_exception_or_error_handler_ce, "handler", sizeof("handler") - 1, ZEND_ACC_PUBLIC);
376-
memcpy(&dd_exception_or_error_handler_handlers, &std_object_handlers, sizeof(zend_object_handlers));
377-
dd_exception_or_error_handler_handlers.get_closure = dd_exception_handler_get_closure;
383+
memcpy(&dd_error_handler_handlers, &std_object_handlers, sizeof(zend_object_handlers));
384+
dd_error_handler_handlers.get_closure = dd_exception_handler_get_closure;
385+
memcpy(&dd_exception_handler_handlers, &dd_error_handler_handlers, sizeof(zend_object_handlers));
386+
#if PHP_VERSION_ID >= 80100
387+
dd_exception_handler_handlers.free_obj = dd_exception_handler_freed;
388+
#endif
378389

379390
datadog_php_zif_handler handlers[] = {
380391
{ZEND_STRL("header"), &dd_header, ZEND_FN(ddtrace_header)},

‎zend_abstract_interface/config/config_ini.c

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -284,13 +284,26 @@ void zai_config_ini_minit(zai_config_env_to_ini_name env_to_ini, int module_numb
284284
#endif
285285
}
286286

287-
void zai_config_ini_rinit() {
287+
void zai_config_ini_rinit(void) {
288288
// we have to cover two cases here:
289289
// a) update ini tables to take changes during first-time rinit into account on ZTS
290290
// b) read current env variables
291291
// c) apply and verify fpm&apache config/user.ini/htaccess settings
292+
293+
// effectively, also including preloading
294+
// in_startup = true, if zend_post_startup() hasn't been executed yet
295+
#if PHP_VERSION_ID < 70400
296+
bool in_startup = !php_get_module_initialized();
297+
#elif PHP_VERSION_ID < 80000
298+
// Sadly not precisely, observable on PHP 7.4, so we'll just explicitly support the preload case only
299+
bool in_startup = (CG(compiler_options) & ZEND_COMPILE_PRELOAD) != 0;
300+
#else
301+
bool in_startup = php_during_module_startup();
302+
#endif
303+
292304
#if ZTS
293-
if (env_to_ini_name) {
305+
// Skip during preloading, in that case EG(ini_directives) is the actual source of truth (NTS-like)
306+
if (env_to_ini_name && !in_startup) {
294307
for (uint8_t i = 0; i < zai_config_memoized_entries_count; ++i) {
295308
zai_config_memoized_entry *memoized = &zai_config_memoized_entries[i];
296309
if (!memoized->original_on_modify) {
@@ -345,7 +358,7 @@ void zai_config_ini_rinit() {
345358
* for the purposes of short circuiting decode
346359
*/
347360
if (env_to_ini_name) {
348-
zend_string *str = zend_string_init(buf.ptr, strlen(buf.ptr), 0);
361+
zend_string *str = zend_string_init(buf.ptr, strlen(buf.ptr), in_startup);
349362

350363
zend_ini_entry *ini = memoized->ini_entries[name_index];
351364
if (zend_alter_ini_entry_ex(ini->name, str, PHP_INI_USER, PHP_INI_STAGE_RUNTIME, 0) == SUCCESS) {
@@ -384,4 +397,4 @@ void zai_config_ini_rinit() {
384397
}
385398
}
386399

387-
void zai_config_ini_mshutdown() {}
400+
void zai_config_ini_mshutdown(void) {}

0 commit comments

Comments
 (0)
0