From 280fd680c813f5dde2adc5db8ca642058b04c635 Mon Sep 17 00:00:00 2001 From: Levi Morrison Date: Wed, 1 Jun 2022 09:43:25 -0600 Subject: [PATCH] Make vm_interrupt and timed_out atomic (#8327) This is done by adding a new zend_atomic_bool type. The type definition is only available for compiler alignment and size info; it should be treated as opaque and only the zend_atomic_bool_* family of functions should be used. Note that directly using atomic_bool is complicated. All C++ compilers stdlibs that I checked typedef atomic_bool to std::atomic, which can't be used in an extern "C" section, and there's at least one usage of this in core, and probably more outside of it. So, instead use platform specific functions, preferring compiler intrinsics. --- Zend/zend_atomic.c | 47 +++++++++ Zend/zend_atomic.h | 172 +++++++++++++++++++++++++++++++ Zend/zend_execute.c | 4 +- Zend/zend_execute_API.c | 29 +++--- Zend/zend_globals.h | 5 +- Zend/zend_vm_def.h | 4 +- Zend/zend_vm_execute.h | 4 +- configure.ac | 2 +- ext/opcache/jit/zend_jit_trace.c | 2 +- ext/pcntl/pcntl.c | 2 +- sapi/phpdbg/phpdbg_prompt.c | 2 +- win32/build/config.w32 | 2 +- win32/signal.c | 4 +- 13 files changed, 249 insertions(+), 30 deletions(-) create mode 100644 Zend/zend_atomic.c create mode 100644 Zend/zend_atomic.h diff --git a/Zend/zend_atomic.c b/Zend/zend_atomic.c new file mode 100644 index 00000000000..7b16f5e4851 --- /dev/null +++ b/Zend/zend_atomic.c @@ -0,0 +1,47 @@ +/* + +----------------------------------------------------------------------+ + | This source file is subject to version 3.01 of the PHP license, | + | that is bundled with this package in the file LICENSE, and is | + | available through the world-wide-web at the following url: | + | https://www.php.net/license/3_01.txt | + | If you did not receive a copy of the PHP license and are unable to | + | obtain it through the world-wide-web, please send a note to | + | license@php.net so we can mail you a copy immediately. | + +----------------------------------------------------------------------+ + | Authors: Levi Morrison | + +----------------------------------------------------------------------+ + */ + +#include "zend_atomic.h" + +/* This file contains the non-inline copy of atomic functions. This is useful + * for extensions written in languages such as Rust. C and C++ compilers are + * probably going to inline these functions, but in the case they don't, this + * is also where the code will go. + */ + +/* Defined for FFI users; everyone else use ZEND_ATOMIC_BOOL_INIT. + * This is NOT ATOMIC as it is meant for initialization. + */ +ZEND_API void zend_atomic_bool_init(zend_atomic_bool *obj, bool desired) { + ZEND_ATOMIC_BOOL_INIT(obj, desired); +} + +ZEND_API bool zend_atomic_bool_exchange(zend_atomic_bool *obj, bool desired) { + return zend_atomic_bool_exchange_ex(obj, desired); +} + +ZEND_API void zend_atomic_bool_store(zend_atomic_bool *obj, bool desired) { + zend_atomic_bool_store_ex(obj, desired); +} + +#if ZEND_WIN32 || HAVE_SYNC_ATOMICS +/* On these platforms it is non-const due to underlying APIs. */ +ZEND_API bool zend_atomic_bool_load(zend_atomic_bool *obj) { + return zend_atomic_bool_load_ex(obj); +} +#else +ZEND_API bool zend_atomic_bool_load(const zend_atomic_bool *obj) { + return zend_atomic_bool_load_ex(obj); +} +#endif diff --git a/Zend/zend_atomic.h b/Zend/zend_atomic.h new file mode 100644 index 00000000000..5f7f6fbc6e6 --- /dev/null +++ b/Zend/zend_atomic.h @@ -0,0 +1,172 @@ +/* + +----------------------------------------------------------------------+ + | This source file is subject to version 3.01 of the PHP license, | + | that is bundled with this package in the file LICENSE, and is | + | available through the world-wide-web at the following url: | + | https://www.php.net/license/3_01.txt | + | If you did not receive a copy of the PHP license and are unable to | + | obtain it through the world-wide-web, please send a note to | + | license@php.net so we can mail you a copy immediately. | + +----------------------------------------------------------------------+ + | Authors: Levi Morrison | + +----------------------------------------------------------------------+ + */ + +#ifndef ZEND_ATOMIC_H +#define ZEND_ATOMIC_H + +#include "zend_portability.h" + +#include + +#define ZEND_GCC_PREREQ(x, y) \ + ((__GNUC__ == (x) && __GNUC_MINOR__ >= (y)) || (__GNUC__ > (x))) + +/* Builtins are used to avoid library linkage */ +#if __has_feature(c_atomic) +#define HAVE_C11_ATOMICS 1 +#elif ZEND_GCC_PREREQ(4, 7) +#define HAVE_GNUC_ATOMICS 1 +#elif defined(__GNUC__) +#define HAVE_SYNC_ATOMICS 1 +#elif !defined(ZEND_WIN32) +#define HAVE_NO_ATOMICS 1 +#endif + +#undef ZEND_GCC_PREREQ + +/* Treat zend_atomic_* types as opaque. They have definitions only for size + * and alignment purposes. + */ + +#if ZEND_WIN32 || HAVE_SYNC_ATOMICS +typedef struct zend_atomic_bool_s { + volatile char value; +} zend_atomic_bool; +#elif HAVE_C11_ATOMICS +typedef struct zend_atomic_bool_s { + _Atomic(bool) value; +} zend_atomic_bool; +#else +typedef struct zend_atomic_bool_s { + volatile bool value; +} zend_atomic_bool; +#endif + +BEGIN_EXTERN_C() + +#if ZEND_WIN32 + +#define ZEND_ATOMIC_BOOL_INIT(obj, desired) ((obj)->value = (desired)) + +static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *obj, bool desired) { + return InterlockedExchange8(&obj->value, desired); +} + +/* On this platform it is non-const due to Iterlocked API*/ +static zend_always_inline bool zend_atomic_bool_load_ex(zend_atomic_bool *obj) { + /* Or'ing with false won't change the value. */ + return InterlockedOr8(&obj->value, false); +} + +static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj, bool desired) { + (void)InterlockedExchange8(&obj->value, desired); +} + +#elif HAVE_C11_ATOMICS + +#define ZEND_ATOMIC_BOOL_INIT(obj, desired) __c11_atomic_init(&(obj)->value, (desired)) + +static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *obj, bool desired) { + return __c11_atomic_exchange(&obj->value, desired, __ATOMIC_SEQ_CST); +} + +static zend_always_inline bool zend_atomic_bool_load_ex(const zend_atomic_bool *obj) { + return __c11_atomic_load(&obj->value, __ATOMIC_SEQ_CST); +} + +static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj, bool desired) { + __c11_atomic_store(&obj->value, desired, __ATOMIC_SEQ_CST); +} + +#elif HAVE_GNUC_ATOMICS + +#define ZEND_ATOMIC_BOOL_INIT(obj, desired) ((obj)->value = (desired)) + +static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *obj, bool desired) { + bool prev = false; + __atomic_exchange(&obj->value, &desired, &prev, __ATOMIC_SEQ_CST); + return prev; +} + +static zend_always_inline bool zend_atomic_bool_load_ex(const zend_atomic_bool *obj) { + bool prev = false; + __atomic_load(&obj->value, &prev, __ATOMIC_SEQ_CST); + return prev; +} + +static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj, bool desired) { + __atomic_store(&obj->value, &desired, __ATOMIC_SEQ_CST); +} + +#elif HAVE_SYNC_ATOMICS + +#define ZEND_ATOMIC_BOOL_INIT(obj, desired) ((obj)->value = (desired)) + +static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *obj, bool desired) { + bool prev = __sync_lock_test_and_set(&obj->value, desired); + + /* __sync_lock_test_and_set only does an acquire barrier, so sync + * immediately after. + */ + __sync_synchronize(); + return prev; +} + +static zend_always_inline bool zend_atomic_bool_load_ex(zend_atomic_bool *obj) { + /* Or'ing false won't change the value */ + return __sync_fetch_and_or(&obj->value, false); +} + +static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj, bool desired) { + __sync_synchronize(); + obj->value = desired; + __sync_synchronize(); +} + +#elif HAVE_NO_ATOMICS + +#warning No atomics support detected. Please open an issue with platform details. + +#define ZEND_ATOMIC_BOOL_INIT(obj, desired) ((obj)->value = (desired)) + +static zend_always_inline void zend_atomic_bool_store_ex(zend_atomic_bool *obj, bool desired) { + obj->value = desired; +} + +static zend_always_inline bool zend_atomic_bool_load_ex(const zend_atomic_bool *obj) { + return obj->value; +} + +static zend_always_inline bool zend_atomic_bool_exchange_ex(zend_atomic_bool *obj, bool desired) { + bool prev = obj->value; + obj->value = true; + return prev; +} + +#endif + +ZEND_API void zend_atomic_bool_init(zend_atomic_bool *obj, bool desired); +ZEND_API bool zend_atomic_bool_exchange(zend_atomic_bool *obj, bool desired); +ZEND_API void zend_atomic_bool_store(zend_atomic_bool *obj, bool desired); + +#if ZEND_WIN32 || HAVE_SYNC_ATOMICS +/* On these platforms it is non-const due to underlying APIs. */ +ZEND_API bool zend_atomic_bool_load(zend_atomic_bool *obj); +#else +ZEND_API bool zend_atomic_bool_load(const zend_atomic_bool *obj); +#endif + +END_EXTERN_C() + +#endif diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 436a5a2fd2a..7b55fa5e8ac 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -3726,13 +3726,13 @@ ZEND_API void ZEND_FASTCALL zend_free_compiled_variables(zend_execute_data *exec /* }}} */ #define ZEND_VM_INTERRUPT_CHECK() do { \ - if (UNEXPECTED(EG(vm_interrupt))) { \ + if (UNEXPECTED(zend_atomic_bool_load_ex(&EG(vm_interrupt)))) { \ ZEND_VM_INTERRUPT(); \ } \ } while (0) #define ZEND_VM_LOOP_INTERRUPT_CHECK() do { \ - if (UNEXPECTED(EG(vm_interrupt))) { \ + if (UNEXPECTED(zend_atomic_bool_load_ex(&EG(vm_interrupt)))) { \ ZEND_VM_LOOP_INTERRUPT(); \ } \ } while (0) diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c index ddc2f7f4d3d..e203556a8ec 100644 --- a/Zend/zend_execute_API.c +++ b/Zend/zend_execute_API.c @@ -168,8 +168,8 @@ void init_executor(void) /* {{{ */ zend_objects_store_init(&EG(objects_store), 1024); EG(full_tables_cleanup) = 0; - EG(vm_interrupt) = 0; - EG(timed_out) = 0; + ZEND_ATOMIC_BOOL_INIT(&EG(vm_interrupt), false); + ZEND_ATOMIC_BOOL_INIT(&EG(timed_out), false); EG(exception) = NULL; EG(prev_exception) = NULL; @@ -960,9 +960,8 @@ cleanup_args: /* This flag is regularly checked while running user functions, but not internal * So see whether interrupt flag was set while the function was running... */ - if (EG(vm_interrupt)) { - EG(vm_interrupt) = 0; - if (EG(timed_out)) { + if (zend_atomic_bool_exchange_ex(&EG(vm_interrupt), false)) { + if (zend_atomic_bool_load_ex(&EG(timed_out))) { zend_timeout(); } else if (zend_interrupt_function) { zend_interrupt_function(EG(current_execute_data)); @@ -1330,14 +1329,14 @@ ZEND_API ZEND_NORETURN void ZEND_FASTCALL zend_timeout(void) /* {{{ */ timer is not restarted properly, it could hang in the shutdown function. */ if (EG(hard_timeout) > 0) { - EG(timed_out) = 0; + zend_atomic_bool_store_ex(&EG(timed_out), false); zend_set_timeout_ex(EG(hard_timeout), 1); /* XXX Abused, introduce an additional flag if the value needs to be kept. */ EG(hard_timeout) = 0; } # endif #else - EG(timed_out) = 0; + zend_atomic_bool_store_ex(&EG(timed_out), false); zend_set_timeout_ex(0, 1); #endif @@ -1349,7 +1348,7 @@ ZEND_API ZEND_NORETURN void ZEND_FASTCALL zend_timeout(void) /* {{{ */ static void zend_timeout_handler(int dummy) /* {{{ */ { #ifndef ZTS - if (EG(timed_out)) { + if (zend_atomic_bool_load_ex(&EG(timed_out))) { /* Die on hard timeout */ const char *error_filename = NULL; uint32_t error_lineno = 0; @@ -1384,8 +1383,8 @@ static void zend_timeout_handler(int dummy) /* {{{ */ zend_on_timeout(EG(timeout_seconds)); } - EG(timed_out) = 1; - EG(vm_interrupt) = 1; + zend_atomic_bool_store_ex(&EG(timed_out), true); + zend_atomic_bool_store_ex(&EG(vm_interrupt), true); #ifndef ZTS if (EG(hard_timeout) > 0) { @@ -1409,8 +1408,8 @@ VOID CALLBACK tq_timer_cb(PVOID arg, BOOLEAN timed_out) } eg = (zend_executor_globals *)arg; - eg->timed_out = 1; - eg->vm_interrupt = 1; + zend_atomic_bool_store_ex(&eg->timed_out, true); + zend_atomic_bool_store_ex(&eg->vm_interrupt, true); } #endif @@ -1496,7 +1495,7 @@ void zend_set_timeout(zend_long seconds, bool reset_signals) /* {{{ */ EG(timeout_seconds) = seconds; zend_set_timeout_ex(seconds, reset_signals); - EG(timed_out) = 0; + zend_atomic_bool_store_ex(&EG(timed_out), false); } /* }}} */ @@ -1505,7 +1504,7 @@ void zend_unset_timeout(void) /* {{{ */ #ifdef ZEND_WIN32 if (NULL != tq_timer) { if (!DeleteTimerQueueTimer(NULL, tq_timer, INVALID_HANDLE_VALUE)) { - EG(timed_out) = 0; + zend_atomic_bool_store_ex(&EG(timed_out), false); tq_timer = NULL; zend_error_noreturn(E_ERROR, "Could not delete queued timer"); return; @@ -1525,7 +1524,7 @@ void zend_unset_timeout(void) /* {{{ */ # endif } #endif - EG(timed_out) = 0; + zend_atomic_bool_store_ex(&EG(timed_out), false); } /* }}} */ diff --git a/Zend/zend_globals.h b/Zend/zend_globals.h index ccee57b3fb1..17469fab0c1 100644 --- a/Zend/zend_globals.h +++ b/Zend/zend_globals.h @@ -25,6 +25,7 @@ #include "zend_globals_macros.h" +#include "zend_atomic.h" #include "zend_stack.h" #include "zend_ptr_stack.h" #include "zend_hash.h" @@ -189,8 +190,8 @@ struct _zend_executor_globals { /* for extended information support */ bool no_extensions; - bool vm_interrupt; - bool timed_out; + zend_atomic_bool vm_interrupt; + zend_atomic_bool timed_out; zend_long hard_timeout; #ifdef ZEND_WIN32 diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h index bbba2ca0dfc..0dc7f4d5f4d 100644 --- a/Zend/zend_vm_def.h +++ b/Zend/zend_vm_def.h @@ -9892,9 +9892,9 @@ ZEND_VM_DEFINE_OP(137, ZEND_OP_DATA); ZEND_VM_HELPER(zend_interrupt_helper, ANY, ANY) { - EG(vm_interrupt) = 0; + zend_atomic_bool_store_ex(&EG(vm_interrupt), false); SAVE_OPLINE(); - if (EG(timed_out)) { + if (zend_atomic_bool_load_ex(&EG(timed_out))) { zend_timeout(); } else if (zend_interrupt_function) { zend_interrupt_function(execute_data); diff --git a/Zend/zend_vm_execute.h b/Zend/zend_vm_execute.h index 4caf4a2cd71..87480a0c8ec 100644 --- a/Zend/zend_vm_execute.h +++ b/Zend/zend_vm_execute.h @@ -3537,9 +3537,9 @@ static ZEND_VM_HOT ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL ZEND_JMP_FORWARD_SPEC_H static zend_never_inline ZEND_OPCODE_HANDLER_RET ZEND_FASTCALL zend_interrupt_helper_SPEC(ZEND_OPCODE_HANDLER_ARGS) { - EG(vm_interrupt) = 0; + zend_atomic_bool_store_ex(&EG(vm_interrupt), false); SAVE_OPLINE(); - if (EG(timed_out)) { + if (zend_atomic_bool_load_ex(&EG(timed_out))) { zend_timeout(); } else if (zend_interrupt_function) { zend_interrupt_function(execute_data); diff --git a/configure.ac b/configure.ac index 8887c2aea65..bed4b063bcc 100644 --- a/configure.ac +++ b/configure.ac @@ -1640,7 +1640,7 @@ PHP_ADD_SOURCES(Zend, \ zend_closures.c zend_weakrefs.c zend_float.c zend_string.c zend_signal.c zend_generators.c \ zend_virtual_cwd.c zend_ast.c zend_objects.c zend_object_handlers.c zend_objects_API.c \ zend_default_classes.c zend_inheritance.c zend_smart_str.c zend_cpuinfo.c zend_gdb.c \ - zend_observer.c zend_system_id.c zend_enum.c zend_fibers.c \ + zend_observer.c zend_system_id.c zend_enum.c zend_fibers.c zend_atomic.c \ Optimizer/zend_optimizer.c \ Optimizer/pass1.c \ Optimizer/pass3.c \ diff --git a/ext/opcache/jit/zend_jit_trace.c b/ext/opcache/jit/zend_jit_trace.c index e876d2c5f25..09c2ea9dbc0 100644 --- a/ext/opcache/jit/zend_jit_trace.c +++ b/ext/opcache/jit/zend_jit_trace.c @@ -8032,7 +8032,7 @@ int ZEND_FASTCALL zend_jit_trace_exit(uint32_t exit_num, zend_jit_registers_buf EX(opline) = opline; } - if (EG(vm_interrupt) || JIT_G(tracing)) { + if (zend_atomic_bool_load_ex(&EG(vm_interrupt)) || JIT_G(tracing)) { return 1; /* Lock-free check if the side trace was already JIT-ed or blacklist-ed in another process */ } else if (t->exit_info[exit_num].flags & (ZEND_JIT_EXIT_JITED|ZEND_JIT_EXIT_BLACKLISTED)) { diff --git a/ext/pcntl/pcntl.c b/ext/pcntl/pcntl.c index d8aeb1f00e7..fe4fe104a36 100644 --- a/ext/pcntl/pcntl.c +++ b/ext/pcntl/pcntl.c @@ -1385,7 +1385,7 @@ static void pcntl_signal_handler(int signo) PCNTL_G(tail) = psig; PCNTL_G(pending_signals) = 1; if (PCNTL_G(async_signals)) { - EG(vm_interrupt) = 1; + zend_atomic_bool_store_ex(&EG(vm_interrupt), true); } } diff --git a/sapi/phpdbg/phpdbg_prompt.c b/sapi/phpdbg/phpdbg_prompt.c index 0aa13475eab..a7003473727 100644 --- a/sapi/phpdbg/phpdbg_prompt.c +++ b/sapi/phpdbg/phpdbg_prompt.c @@ -1663,7 +1663,7 @@ void phpdbg_execute_ex(zend_execute_data *execute_data) /* {{{ */ } #ifdef ZEND_WIN32 - if (EG(timed_out)) { + if (zend_atomic_bool_load_ex(&EG(timed_out))) { zend_timeout(); } #endif diff --git a/win32/build/config.w32 b/win32/build/config.w32 index ab7c07896c6..c174ebe8541 100644 --- a/win32/build/config.w32 +++ b/win32/build/config.w32 @@ -238,7 +238,7 @@ ADD_SOURCES("Zend", "zend_language_parser.c zend_language_scanner.c \ zend_default_classes.c zend_execute.c zend_strtod.c zend_gc.c zend_closures.c zend_weakrefs.c \ zend_float.c zend_string.c zend_generators.c zend_virtual_cwd.c zend_ast.c \ zend_inheritance.c zend_smart_str.c zend_cpuinfo.c zend_observer.c zend_system_id.c \ - zend_enum.c zend_fibers.c"); + zend_enum.c zend_fibers.c zend_atomic.c"); ADD_SOURCES("Zend\\Optimizer", "zend_optimizer.c pass1.c pass3.c optimize_func_calls.c block_pass.c optimize_temp_vars_5.c nop_removal.c compact_literals.c zend_cfg.c zend_dfg.c dfa_pass.c zend_ssa.c zend_inference.c zend_func_info.c zend_call_graph.c zend_dump.c escape_analysis.c compact_vars.c dce.c sccp.c scdf.c"); var FIBER_ASSEMBLER = X64 ? PATH_PROG('ML64') : PATH_PROG('ML'); diff --git a/win32/signal.c b/win32/signal.c index 088a0044283..88f975ab644 100644 --- a/win32/signal.c +++ b/win32/signal.c @@ -22,7 +22,7 @@ /* true globals; only used from main thread and from kernel callback */ static zval ctrl_handler; static DWORD ctrl_evt = (DWORD)-1; -static bool *vm_interrupt_flag = NULL; +static zend_atomic_bool *vm_interrupt_flag = NULL; static void (*orig_interrupt_function)(zend_execute_data *execute_data); @@ -77,7 +77,7 @@ static BOOL WINAPI php_win32_signal_system_ctrl_handler(DWORD evt) return FALSE; } - (void)InterlockedExchange8(vm_interrupt_flag, 1); + zend_atomic_bool_store_ex(vm_interrupt_flag, true); ctrl_evt = evt;