From a0a6ef9ae4e73b10ac10d483cb6ea7df45b2aa36 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Fri, 5 May 2023 16:12:22 +0200 Subject: [PATCH 1/2] Fix #2030: Segmentation fault with autoloaders bailing out We fix this issue by ensuring that the opline is always connected to its proper execute_data and popping newer entries if they aren't matching. Signed-off-by: Bob Weinand --- .../class_resolver_bailout_hook.phpt | 31 +++++++++++++++++ .../interceptor/php7/interceptor.c | 7 ++-- .../interceptor/php7/resolver.c | 34 +++++++++++++++---- .../interceptor/php8/interceptor.c | 7 ++++ .../interceptor/php8/resolver_pre-8_2.c | 32 ++++++++++++++--- 5 files changed, 98 insertions(+), 13 deletions(-) create mode 100644 tests/ext/sandbox-regression/class_resolver_bailout_hook.phpt diff --git a/tests/ext/sandbox-regression/class_resolver_bailout_hook.phpt b/tests/ext/sandbox-regression/class_resolver_bailout_hook.phpt new file mode 100644 index 0000000000..8e4244257f --- /dev/null +++ b/tests/ext/sandbox-regression/class_resolver_bailout_hook.phpt @@ -0,0 +1,31 @@ +--TEST-- +Assert bailouts are gracefully handled within class autoloading +--SKIPIF-- += 70300 && PHP_VERSION_ID < 70400) die('skip: Bailing out in autoloaders is fundamentally broken in PHP 7.3'); ?> +--ENV-- +DD_TRACE_DEBUG=1 +--FILE-- + +--EXPECTF-- +Error raised in ddtrace's closure defined at %s:%d for x(): No D in %s +Leaving Autoloader +Flushing trace of size 2 to send-queue for %s diff --git a/zend_abstract_interface/interceptor/php7/interceptor.c b/zend_abstract_interface/interceptor/php7/interceptor.c index daec4fe1ad..3e2a4e0349 100644 --- a/zend_abstract_interface/interceptor/php7/interceptor.c +++ b/zend_abstract_interface/interceptor/php7/interceptor.c @@ -403,11 +403,11 @@ static int zai_interceptor_fast_ret_handler(zend_execute_data *execute_data) { return prev_fast_ret_handler ? prev_fast_ret_handler(execute_data) : ZEND_USER_OPCODE_DISPATCH; } -void zai_interceptor_check_for_opline_before_exception(void); +void zai_interceptor_check_for_opline_before_exception(zend_execute_data *execute_data); static user_opcode_handler_t prev_handle_exception_handler; static int zai_interceptor_handle_exception_handler(zend_execute_data *execute_data) { // not everything goes through zend_throw_exception_hook, in particular when zend_rethrow_exception alone is used (e.g. during zend_call_function) - zai_interceptor_check_for_opline_before_exception(); + zai_interceptor_check_for_opline_before_exception(execute_data); zai_interceptor_frame_memory *frame_memory; if (ZEND_HANDLE_EXCEPTION == EX(opline)->opcode && zai_hook_memory_table_find(execute_data, &frame_memory)) { @@ -1170,7 +1170,10 @@ static void zai_hook_memory_dtor(zval *zv) { efree(Z_PTR_P(zv)); } +void zai_interceptor_reset_resolver(void); void zai_interceptor_activate() { + zai_interceptor_reset_resolver(); + zend_hash_init(&zai_hook_memory, 8, nothing, zai_hook_memory_dtor, 0); } diff --git a/zend_abstract_interface/interceptor/php7/resolver.c b/zend_abstract_interface/interceptor/php7/resolver.c index 52ac43c3dd..9168b269d7 100644 --- a/zend_abstract_interface/interceptor/php7/resolver.c +++ b/zend_abstract_interface/interceptor/php7/resolver.c @@ -79,7 +79,7 @@ PHP_FUNCTION(zai_interceptor_resolve_after_class_alias) { #define ZAI_INTERCEPTOR_POST_DECLARE_OP 224 // random 8 bit number greater than ZEND_VM_LAST_OPCODE static zend_op zai_interceptor_post_declare_op; ZEND_TLS zend_op zai_interceptor_post_declare_ops[4]; -struct zai_interceptor_opline { const zend_op *op; struct zai_interceptor_opline *prev; }; +struct zai_interceptor_opline { const zend_op *op; const zend_execute_data *execute_data; struct zai_interceptor_opline *prev; }; ZEND_TLS struct zai_interceptor_opline zai_interceptor_opline_before_binding = {0}; static void zai_interceptor_install_post_declare_op(zend_execute_data *execute_data) { // We replace the current opline *before* it is executed. Thus we need to preserve opline data first: @@ -109,10 +109,24 @@ static void zai_interceptor_install_post_declare_op(zend_execute_data *execute_d zai_interceptor_opline_before_binding.prev = backup; } zai_interceptor_opline_before_binding.op = EX(opline); + zai_interceptor_opline_before_binding.execute_data = execute_data; EX(opline) = zai_interceptor_post_declare_ops; } -static void zai_interceptor_pop_opline_before_binding() { +static void zai_interceptor_pop_opline_before_binding(zend_execute_data *execute_data) { + // Normally the zai_interceptor_opline_before_binding stack should be in sync with the actual executing stack, but it might not after bailouts + if (execute_data) { + if (zai_interceptor_opline_before_binding.execute_data == execute_data) { + return; + } + + while (zai_interceptor_opline_before_binding.prev && zai_interceptor_opline_before_binding.prev->execute_data != execute_data) { + struct zai_interceptor_opline *backup = zai_interceptor_opline_before_binding.prev; + zai_interceptor_opline_before_binding = *backup; + efree(backup); + } + } + struct zai_interceptor_opline *backup = zai_interceptor_opline_before_binding.prev; if (backup) { zai_interceptor_opline_before_binding = *backup; @@ -171,8 +185,9 @@ static int zai_interceptor_post_declare_handler(zend_execute_data *execute_data) } } // preserve offset + zai_interceptor_pop_opline_before_binding(execute_data); EX(opline) = zai_interceptor_opline_before_binding.op + (EX(opline) - &zai_interceptor_post_declare_ops[0]); - zai_interceptor_pop_opline_before_binding(); + zai_interceptor_pop_opline_before_binding(NULL); return ZEND_USER_OPCODE_CONTINUE; } else if (prev_post_declare_handler) { return prev_post_declare_handler(execute_data); @@ -244,10 +259,11 @@ static int zai_interceptor_add_interface_handler(zend_execute_data *execute_data } #endif -void zai_interceptor_check_for_opline_before_exception(void) { +void zai_interceptor_check_for_opline_before_exception(zend_execute_data *execute_data) { if (EG(opline_before_exception) == zai_interceptor_post_declare_ops) { + zai_interceptor_pop_opline_before_binding(execute_data); EG(opline_before_exception) = zai_interceptor_opline_before_binding.op; - zai_interceptor_pop_opline_before_binding(); + zai_interceptor_pop_opline_before_binding(NULL); } } @@ -256,14 +272,20 @@ static void zai_interceptor_exception_hook(zval *ex) { zend_function *func = EG(current_execute_data)->func; if (func && ZEND_USER_CODE(func->type) && EG(current_execute_data)->opline == zai_interceptor_post_declare_ops) { // called right before setting EG(opline_before_exception), reset to original value to ensure correct throw_op handling + zai_interceptor_pop_opline_before_binding(EG(current_execute_data)); EG(current_execute_data)->opline = zai_interceptor_opline_before_binding.op; - zai_interceptor_pop_opline_before_binding(); + zai_interceptor_pop_opline_before_binding(NULL); } if (prev_exception_hook) { prev_exception_hook(ex); } } +void zai_interceptor_reset_resolver() { + // reset in case a prior request had a bailout + zai_interceptor_opline_before_binding = (struct zai_interceptor_opline){0}; +} + // post startup hook to be after opcache void zai_interceptor_setup_resolving_post_startup(void) { prev_compile_file = zend_compile_file; diff --git a/zend_abstract_interface/interceptor/php8/interceptor.c b/zend_abstract_interface/interceptor/php8/interceptor.c index 3ca12bef9e..55154d927a 100644 --- a/zend_abstract_interface/interceptor/php8/interceptor.c +++ b/zend_abstract_interface/interceptor/php8/interceptor.c @@ -818,9 +818,16 @@ static void zai_hook_memory_dtor(zval *zv) { efree(Z_PTR_P(zv)); } +#if PHP_VERSION_ID < 80200 +void zai_interceptor_reset_resolver(void); +#endif void zai_interceptor_activate(void) { zend_hash_init(&zai_hook_memory, 8, nothing, zai_hook_memory_dtor, 0); zend_hash_init(&zai_interceptor_implicit_generators, 8, nothing, NULL, 0); + +#if PHP_VERSION_ID < 80200 + zai_interceptor_reset_resolver(); +#endif } void zai_interceptor_deactivate(void) { diff --git a/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c b/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c index ede62cc604..ff55ecf0bb 100644 --- a/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c +++ b/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c @@ -82,7 +82,7 @@ PHP_FUNCTION(zai_interceptor_resolve_after_class_alias) { static zend_op zai_interceptor_post_declare_op; ZEND_TLS zend_op zai_interceptor_post_declare_ops[4]; -struct zai_interceptor_opline { const zend_op *op; struct zai_interceptor_opline *prev; }; +struct zai_interceptor_opline { const zend_op *op; const zend_execute_data *execute_data; struct zai_interceptor_opline *prev; }; ZEND_TLS struct zai_interceptor_opline zai_interceptor_opline_before_binding = {0}; static void zai_interceptor_install_post_declare_op(zend_execute_data *execute_data) { // We replace the current opline *before* it is executed. Thus we need to preserve opline data first: @@ -110,10 +110,24 @@ static void zai_interceptor_install_post_declare_op(zend_execute_data *execute_d zai_interceptor_opline_before_binding.prev = backup; } zai_interceptor_opline_before_binding.op = EX(opline); + zai_interceptor_opline_before_binding.execute_data = execute_data; EX(opline) = zai_interceptor_post_declare_ops; } -static void zai_interceptor_pop_opline_before_binding() { +static void zai_interceptor_pop_opline_before_binding(zend_execute_data *execute_data) { + // Normally the zai_interceptor_opline_before_binding stack should be in sync with the actual executing stack, but it might not after bailouts + if (execute_data) { + if (zai_interceptor_opline_before_binding.execute_data == execute_data) { + return; + } + + while (zai_interceptor_opline_before_binding.prev && zai_interceptor_opline_before_binding.prev->execute_data != execute_data) { + struct zai_interceptor_opline *backup = zai_interceptor_opline_before_binding.prev; + zai_interceptor_opline_before_binding = *backup; + efree(backup); + } + } + struct zai_interceptor_opline *backup = zai_interceptor_opline_before_binding.prev; if (backup) { zai_interceptor_opline_before_binding = *backup; @@ -154,8 +168,9 @@ static int zai_interceptor_post_declare_handler(zend_execute_data *execute_data) } } // preserve offset + zai_interceptor_pop_opline_before_binding(execute_data); EX(opline) = zai_interceptor_opline_before_binding.op + (EX(opline) - &zai_interceptor_post_declare_ops[0]); - zai_interceptor_pop_opline_before_binding(); + zai_interceptor_pop_opline_before_binding(NULL); return ZEND_USER_OPCODE_CONTINUE; } else if (prev_post_declare_handler) { return prev_post_declare_handler(execute_data); @@ -237,8 +252,9 @@ static user_opcode_handler_t prev_handle_exception_handler; static int zai_interceptor_handle_exception_handler(zend_execute_data *execute_data) { // not everything goes through zend_throw_exception_hook, in particular when zend_rethrow_exception alone is used (e.g. during zend_call_function) if (EG(opline_before_exception) == zai_interceptor_post_declare_ops) { + zai_interceptor_pop_opline_before_binding(execute_data); EG(opline_before_exception) = zai_interceptor_opline_before_binding.op; - zai_interceptor_pop_opline_before_binding(); + zai_interceptor_pop_opline_before_binding(NULL); } return prev_handle_exception_handler ? prev_handle_exception_handler(execute_data) : ZEND_USER_OPCODE_DISPATCH; @@ -249,14 +265,20 @@ static void zai_interceptor_exception_hook(zend_object *ex) { zend_function *func = EG(current_execute_data)->func; if (func && ZEND_USER_CODE(func->type) && EG(current_execute_data)->opline == zai_interceptor_post_declare_ops) { // called right before setting EG(opline_before_exception), reset to original value to ensure correct throw_op handling + zai_interceptor_pop_opline_before_binding(EG(current_execute_data)); EG(current_execute_data)->opline = zai_interceptor_opline_before_binding.op; - zai_interceptor_pop_opline_before_binding(); + zai_interceptor_pop_opline_before_binding(NULL); } if (prev_exception_hook) { prev_exception_hook(ex); } } +void zai_interceptor_reset_resolver() { + // reset in case a prior request had a bailout + zai_interceptor_opline_before_binding = (struct zai_interceptor_opline){0}; +} + static void *opcache_handle; static void zai_interceptor_find_opcache_handle(void *ext) { zend_extension *extension = (zend_extension *)ext; From 39d72be5b27dbf0d5f450234b008ed16b70cd167 Mon Sep 17 00:00:00 2001 From: Bob Weinand Date: Thu, 11 May 2023 14:41:06 +0200 Subject: [PATCH 2/2] Add void to prototypes Signed-off-by: Bob Weinand --- zend_abstract_interface/interceptor/php7/resolver.c | 2 +- zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/zend_abstract_interface/interceptor/php7/resolver.c b/zend_abstract_interface/interceptor/php7/resolver.c index 9168b269d7..4cf0e18261 100644 --- a/zend_abstract_interface/interceptor/php7/resolver.c +++ b/zend_abstract_interface/interceptor/php7/resolver.c @@ -281,7 +281,7 @@ static void zai_interceptor_exception_hook(zval *ex) { } } -void zai_interceptor_reset_resolver() { +void zai_interceptor_reset_resolver(void) { // reset in case a prior request had a bailout zai_interceptor_opline_before_binding = (struct zai_interceptor_opline){0}; } diff --git a/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c b/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c index ff55ecf0bb..9114116893 100644 --- a/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c +++ b/zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c @@ -274,7 +274,7 @@ static void zai_interceptor_exception_hook(zend_object *ex) { } } -void zai_interceptor_reset_resolver() { +void zai_interceptor_reset_resolver(void) { // reset in case a prior request had a bailout zai_interceptor_opline_before_binding = (struct zai_interceptor_opline){0}; }