From dcbf93a90b023141b13d9539087c23f2a1fadbe8 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Mon, 19 Aug 2024 16:22:23 +0200 Subject: [PATCH] Fix JIT crash with instrumented generators The JIT will look up zend_op_trace_info in (char*)EX(opline) + offset, where offset is generally somewhere in valid memory, but will effectively have random values, and crash when using these. Also add tooling to trivially open a randomized corefile from circleci. Signed-off-by: Bob Weinand --- .../install_hook/trace_generator_jit.phpt | 42 +++++++++++++ tests/randomized/README.md | 12 +++- tests/randomized/circleci_core.sh | 59 +++++++++++++++++++ zend_abstract_interface/CMakeLists.txt | 3 + zend_abstract_interface/hook/CMakeLists.txt | 3 + zend_abstract_interface/hook/hook.c | 8 +++ .../hook/tests/CMakeLists.txt | 3 + .../jit_utils/CMakeLists.txt | 22 +++++++ 8 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 tests/ext/sandbox/install_hook/trace_generator_jit.phpt create mode 100755 tests/randomized/circleci_core.sh create mode 100644 zend_abstract_interface/jit_utils/CMakeLists.txt diff --git a/tests/ext/sandbox/install_hook/trace_generator_jit.phpt b/tests/ext/sandbox/install_hook/trace_generator_jit.phpt new file mode 100644 index 0000000000..a447530e89 --- /dev/null +++ b/tests/ext/sandbox/install_hook/trace_generator_jit.phpt @@ -0,0 +1,42 @@ +--TEST-- +generator hooking works with JIT +--SKIPIF-- + + +--INI-- +opcache.enable=1 +opcache.enable_cli = 1 +opcache.jit_buffer_size=512M +opcache.jit=1255 +zend_extension=opcache.so +--FILE-- + function($args, $retval) { + var_dump($retval); + } +]); + +for ($i = 0; $i < 2; ++$i) { + foreach (gen() as $val) { + var_dump($val); + } +} + +?> +--EXPECT-- +int(1) +int(1) +int(2) +int(2) +int(1) +int(1) +int(2) +int(2) diff --git a/tests/randomized/README.md b/tests/randomized/README.md index dfa4247811..cd0b5017e2 100644 --- a/tests/randomized/README.md +++ b/tests/randomized/README.md @@ -145,6 +145,16 @@ Note that the generated scenarios can be run individually. In the directory `.tm ### Analyzing a core dump generated in CI +Run a script `circleci_core.sh` which will install the used extension, and launch a gdb shell with the core dump: + +``` +tests/randomized/circleci_core.sh https://app.circleci.com/pipelines/github/DataDog/dd-trace-php/17113/workflows/196b5740-f3ce-4faf-8731-e9a4b15d114a/jobs/4964970/steps +``` + +The only argument to that script is the URL of the job. + +#### Alternative: Manual steps + Run a container with the most recent version of the proper docker image for the specific version of PHP. For example, assuming PHP 8.0: ``` @@ -172,7 +182,7 @@ Then load it in `gdb`: gdb --core=/tmp/core php-fpm|httpd|php ``` -### Analyzing using ASAN +#### Manual steps using ASAN In some cases it might be tricky to find the memory corruption, but you can always use an address sanitizer to try and find problems. diff --git a/tests/randomized/circleci_core.sh b/tests/randomized/circleci_core.sh new file mode 100755 index 0000000000..154e2348af --- /dev/null +++ b/tests/randomized/circleci_core.sh @@ -0,0 +1,59 @@ +#!/bin/bash + +# e.g. https://app.circleci.com/pipelines/github/DataDog/dd-trace-php/17113/workflows/196b5740-f3ce-4faf-8731-e9a4b15d114a/jobs/4964970/steps +URL=${1} + +if ! [[ $URL =~ workflows/([0-9a-f-]+)/jobs/([0-9]+) ]]; then + echo "Invalid URL?" + exit 1 +fi + +CIRCLE_WORKFLOW_ID=${BASH_REMATCH[1]} +CIRCLE_JOB_ID=${BASH_REMATCH[2]} + +TEST_NAME=$(curl -s -X GET "https://circleci.com/api/v2/project/gh/DataDog/dd-trace-php/job/$CIRCLE_JOB_ID" -H "Accept: application/json" | grep -Eo '"name":"randomized[^"]+') +TEST_NAME=${TEST_NAME:8} + +echo "Found test run: $TEST_NAME" + +COREFILES=($(curl -s -X GET "https://circleci.com/api/v2/project/gh/DataDog/dd-trace-php/$CIRCLE_JOB_ID/artifacts" -H "Accept: application/json" | grep -Eo 'https://[^"]+core')) + +if [[ -z $COREFILES ]]; then + echo "Found no core files..." + exit 1 +fi + +num=1 +for file in "${COREFILES[@]}"; do + echo "$((num++)): $(echo "$file" | grep -Eo 'randomized-[^/]+')" +done + +while : ; do + echo -n "Select one core file: " + read num + if [[ $num -gt ${#COREFILES[@]} || $num -le 0 ]]; then + echo "Invalid number $num" + else + break + fi +done + +COREFILE=${COREFILES[$((num - 1))]} +corefilename=$(echo "$COREFILE" | grep -Eo 'randomized-[^/]+') +container="datadog/dd-trace-ci:php-randomizedtests-$(if [[ $COREFILE == *buster* ]]; then echo buster; else echo centos7; fi)-${corefilename: -2:1}.${corefilename: -1}-2" + +if [[ $TEST_NAME == *asan* ]]; then + ARTIFACTS_JOB='package extension zts-debug-asan' +else + ARTIFACTS_JOB='package extension' +fi + +PACKAGE_ARCH=$(if [[ $TEST_NAME == *arm* ]]; then echo aarch64; else echo x86_64; fi) + +parent_job_id=$(curl -s -X GET "https://circleci.com/api/v2/workflow/$CIRCLE_WORKFLOW_ID/job" -H "Accept: application/json" | grep -Eo '\{[^}]*"'"$ARTIFACTS_JOB"'"[^}]*' | grep -Eo '"job_number":[^,]+' | tail -c +14) +ARTIFACTS_RESULT=$(curl -s -X GET "https://circleci.com/api/v2/project/github/DataDog/dd-trace-php/$parent_job_id/artifacts" -H "Accept: application/json") +artifact_url=$(echo "$ARTIFACTS_RESULT" | grep -Eo '\{[^}]*"dd-library-php-[^"]+'"${PACKAGE_ARCH}"'-linux-gnu.tar.gz"[^}]*' | grep -Eo '"url":"[^"]+' | tail -c +8) +setup_url=$(echo "$ARTIFACTS_RESULT" | grep -Eo '\{[^}]*"datadog-setup.php"[^}]*' | grep -Eo '"url":"[^"]+' | tail -c +8) + +set -x +docker run --platform=linux/$(if [[ $TEST_NAME == *arm* ]]; then echo arm64; else echo amd64; fi) --rm -ti "$container" bash -c "curl -Lo /tmp/datadog-setup.php '$setup_url'; curl -Lo /tmp/${artifact_url##*/} '$artifact_url'; curl -Lo /tmp/core '$COREFILE'; php /tmp/datadog-setup.php --php-bin all --file /tmp/${artifact_url##*/} --enable-profiling; $(if [[ $TEST_NAME == *asan* ]]; then echo "switch-php debug-zts-asan; "; fi)exec bash --rcfile <(echo 'gdb \$(file /tmp/core | grep -Po "'from\\s\\x27\\K[^\\x27:]+'") --core=/tmp/core -ix /usr/local/src/php/.gdbinit')" diff --git a/zend_abstract_interface/CMakeLists.txt b/zend_abstract_interface/CMakeLists.txt index e5ae6a1ded..f100b0285f 100644 --- a/zend_abstract_interface/CMakeLists.txt +++ b/zend_abstract_interface/CMakeLists.txt @@ -66,6 +66,9 @@ add_subdirectory(env) add_subdirectory(exceptions) add_subdirectory(config) add_subdirectory(json) +if(PhpConfig_VERNUM GREATER_EQUAL "80000") + add_subdirectory(jit_utils) +endif() add_subdirectory(symbols) add_subdirectory(hook) add_subdirectory(interceptor) diff --git a/zend_abstract_interface/hook/CMakeLists.txt b/zend_abstract_interface/hook/CMakeLists.txt index e8f4b2ca8c..c96ca3932a 100644 --- a/zend_abstract_interface/hook/CMakeLists.txt +++ b/zend_abstract_interface/hook/CMakeLists.txt @@ -7,6 +7,9 @@ target_include_directories( target_compile_features(zai_hook PUBLIC c_std_99) target_link_libraries(zai_hook PUBLIC Tea::Php Zai::Symbols) +if(PhpConfig_VERNUM GREATER_EQUAL "80000") + target_link_libraries(zai_hook PUBLIC Zai::JitUtils) +endif() set_target_properties(zai_hook PROPERTIES EXPORT_NAME Hook VERSION ${PROJECT_VERSION}) diff --git a/zend_abstract_interface/hook/hook.c b/zend_abstract_interface/hook/hook.c index 0cbb8f3731..a2fe7d770c 100644 --- a/zend_abstract_interface/hook/hook.c +++ b/zend_abstract_interface/hook/hook.c @@ -1,6 +1,7 @@ #include "../tsrmls_cache.h" #include #include +#include /* {{{ */ @@ -399,6 +400,13 @@ static void zai_hook_resolve_hooks_entry(zai_hooks_entry *hooks, zend_function * #endif } hooks->is_generator = (resolved->common.fn_flags & ZEND_ACC_GENERATOR) != 0; + if (hooks->is_generator) { +#if ZAI_JIT_BLACKLIST_ACTIVE + // Generator observers may replace the EX(opline) by a custom op. The tracing JIT will not like this. Blacklist them. + // Note that we cannot defer the blacklisting until the observer hook is invoked as that one will not be called before ZEND_GENERATOR_CREATE. + zai_jit_blacklist_function_inlining(&resolved->op_array); +#endif + } #endif if ((resolved->common.fn_flags & ZEND_ACC_CLOSURE) == 0) { diff --git a/zend_abstract_interface/hook/tests/CMakeLists.txt b/zend_abstract_interface/hook/tests/CMakeLists.txt index 69a9eb045d..d7abe945c3 100644 --- a/zend_abstract_interface/hook/tests/CMakeLists.txt +++ b/zend_abstract_interface/hook/tests/CMakeLists.txt @@ -4,6 +4,9 @@ add_executable(hooks ) target_link_libraries(hooks PUBLIC catch2_main Tea::Tea Zai::Symbols Zai::Hook) +if(PhpConfig_VERNUM GREATER_EQUAL "80000") + target_link_libraries(hooks PUBLIC Zai::JitUtils) +endif() file(COPY ${CMAKE_CURRENT_SOURCE_DIR}/stubs DESTINATION ${CMAKE_CURRENT_BINARY_DIR}) diff --git a/zend_abstract_interface/jit_utils/CMakeLists.txt b/zend_abstract_interface/jit_utils/CMakeLists.txt new file mode 100644 index 0000000000..18d5d50caf --- /dev/null +++ b/zend_abstract_interface/jit_utils/CMakeLists.txt @@ -0,0 +1,22 @@ +add_library(zai_jit_utils jit_blacklist.c) + +target_include_directories( + zai_jit_utils PUBLIC $ + $) + +target_compile_features(zai_jit_utils PUBLIC c_std_99) + +target_link_libraries(zai_jit_utils PUBLIC Tea::Php dl) + +set_target_properties(zai_jit_utils PROPERTIES EXPORT_NAME JitUtils + VERSION ${PROJECT_VERSION}) + +add_library(Zai::JitUtils ALIAS zai_jit_utils) + +install( + FILES ${CMAKE_CURRENT_SOURCE_DIR}/jit_blacklist.h + DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/jit_utils/) + +target_link_libraries(zai_zend_abstract_interface INTERFACE zai_jit_utils) + +install(TARGETS zai_jit_utils EXPORT ZendAbstractInterfaceTargets)