diff --git a/Zend/zend_atomic.h b/Zend/zend_atomic.h index 677d2ed0ccb2..048249784d72 100644 --- a/Zend/zend_atomic.h +++ b/Zend/zend_atomic.h @@ -18,6 +18,7 @@ #include "zend_portability.h" #include +#include #define ZEND_GCC_PREREQ(x, y) \ ((__GNUC__ == (x) && __GNUC_MINOR__ >= (y)) || (__GNUC__ > (x))) @@ -415,6 +416,29 @@ static zend_always_inline uint32_t zend_atomic_load_acquire_32(const uint32_t *p #define ZEND_ATOMIC_LOAD_ACQUIRE_32(ptr) zend_atomic_load_acquire_32(ptr) +/* Naturally aligned machine-word atomics. Loads must not use a + * read-modify-write operation: the storage may be mapped read-only. */ +static zend_always_inline uintptr_t zend_atomic_load_acquire_uintptr(const uintptr_t *ptr) +{ +#if defined(HAVE_GNUC_ATOMICS) || defined(HAVE_C11_ATOMICS) + return __atomic_load_n(ptr, __ATOMIC_ACQUIRE); +#else + uintptr_t value = *(const volatile uintptr_t *)ptr; + ZEND_ATOMIC_FENCE_ACQUIRE(); + return value; +#endif +} + +static zend_always_inline void zend_atomic_store_release_uintptr(uintptr_t *ptr, uintptr_t value) +{ +#if defined(HAVE_GNUC_ATOMICS) || defined(HAVE_C11_ATOMICS) + __atomic_store_n(ptr, value, __ATOMIC_RELEASE); +#else + ZEND_ATOMIC_FENCE_RELEASE(); + *(volatile uintptr_t *)ptr = value; +#endif +} + END_EXTERN_C() #endif diff --git a/ext/opcache/ZendAccelerator.c b/ext/opcache/ZendAccelerator.c index e56904d7efb8..6ad10b644324 100644 --- a/ext/opcache/ZendAccelerator.c +++ b/ext/opcache/ZendAccelerator.c @@ -1607,7 +1607,7 @@ static zend_persistent_script *cache_script_in_shared_memory(zend_persistent_scr * exclusive lock) */ bucket = zend_accel_hash_find_entry(&ZCSG(hash), new_persistent_script->script.filename); if (bucket) { - zend_persistent_script *existing_persistent_script = (zend_persistent_script *)bucket->data; + zend_persistent_script *existing_persistent_script = zend_accel_hash_entry_data(bucket); if (!existing_persistent_script->corrupted) { if (key && @@ -2098,7 +2098,7 @@ zend_op_array *persistent_compile_file(zend_file_handle *file_handle, int type) bucket = zend_accel_hash_find_entry(&ZCSG(hash), file_handle->opened_path); if (bucket) { - persistent_script = (zend_persistent_script *)bucket->data; + persistent_script = zend_accel_hash_entry_data(bucket); if (key && !persistent_script->corrupted) { HANDLE_BLOCK_INTERRUPTIONS(); @@ -2586,7 +2586,7 @@ static zend_string* persistent_zend_resolve_path(zend_string *filename) if (key) { zend_accel_hash_entry *bucket = zend_accel_hash_find_entry(&ZCSG(hash), key); if (bucket != NULL) { - zend_persistent_script *persistent_script = (zend_persistent_script *)bucket->data; + zend_persistent_script *persistent_script = zend_accel_hash_entry_data(bucket); if (!persistent_script->corrupted) { ZCG(cache_opline) = EG(current_execute_data) ? EG(current_execute_data)->opline : NULL; ZCG(cache_persistent_script) = persistent_script; @@ -2607,7 +2607,7 @@ static zend_string* persistent_zend_resolve_path(zend_string *filename) /* lookup by real path */ zend_accel_hash_entry *bucket = zend_accel_hash_find_entry(&ZCSG(hash), resolved_path); if (bucket) { - zend_persistent_script *persistent_script = (zend_persistent_script *)bucket->data; + zend_persistent_script *persistent_script = zend_accel_hash_entry_data(bucket); if (!persistent_script->corrupted) { if (key) { /* add another "key" for the same bucket */ diff --git a/ext/opcache/jit/zend_jit_trace.c b/ext/opcache/jit/zend_jit_trace.c index 9c54767fc226..f4d1adb1dbc4 100644 --- a/ext/opcache/jit/zend_jit_trace.c +++ b/ext/opcache/jit/zend_jit_trace.c @@ -7671,10 +7671,10 @@ static void zend_jit_stop_counter_handlers(void) ZCSG(jit_counters_stopped) = true; for (uint32_t i = 0; i < ZCSG(hash).max_num_entries; i++) { zend_accel_hash_entry *cache_entry; - for (cache_entry = ZCSG(hash).hash_table[i]; cache_entry; cache_entry = cache_entry->next) { + for (cache_entry = zend_accel_hash_load_entry(&ZCSG(hash).hash_table[i]); cache_entry; cache_entry = cache_entry->next) { zend_persistent_script *script; - if (cache_entry->indirect) continue; - script = (zend_persistent_script *)cache_entry->data; + script = zend_accel_hash_direct_data(cache_entry); + if (!script) continue; zend_jit_stop_persistent_script(script); } } diff --git a/ext/opcache/tests/gh24319_001.phpt b/ext/opcache/tests/gh24319_001.phpt new file mode 100644 index 000000000000..452210ceb050 --- /dev/null +++ b/ext/opcache/tests/gh24319_001.phpt @@ -0,0 +1,139 @@ +--TEST-- +GH-24319: Concurrent includes while a direct OPcache key becomes an indirect alias +--DESCRIPTION-- +Cache files under their original names, invalidate them, and replace them with +symlinks to uncached files. Concurrent includes then race OPcache's conversion +of direct entries into aliases. Readers must not observe a mismatched data +pointer and indirect flag, or mistake an entry converted after lookup for a +compiled script. + +This stress test disables opcache.protect_memory: the test runner normally +enables it, but repeated mprotect() calls can dominate this workload and make +its watchdog fail on slower systems. The small sequential companion test +covers protected shared-memory reads. +--EXTENSIONS-- +opcache +pcntl +--SKIPIF-- + +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +opcache.jit=disable +opcache.jit_buffer_size=0 +opcache.file_update_protection=0 +opcache.validate_timestamps=0 +opcache.revalidate_path=0 +opcache.protect_memory=0 +opcache.memory_consumption=256 +opcache.interned_strings_buffer=32 +opcache.max_accelerated_files=100000 +opcache.max_wasted_percentage=50 +--FILE-- + $pid) { + do { + $waited = pcntl_waitpid($pid, $status); + } while ($waited === -1 && pcntl_get_last_error() === PCNTL_EINTR); + if ($waited !== $pid) { + echo "Reader $reader: wait failed\n"; + $failed = true; + } elseif (pcntl_wifsignaled($status)) { + $signal = pcntl_wtermsig($status); + echo "Reader $reader: signal $signal", $signal === SIGALRM ? " (watchdog)\n" : "\n"; + $failed = true; + } elseif (!pcntl_wifexited($status) || pcntl_wexitstatus($status) !== 0) { + echo "Reader $reader: exit ", pcntl_wexitstatus($status), "\n"; + $failed = true; + } +} + +foreach ($paths as $i => $old) { + unlink($old); + unlink("$dir/new$i.php"); +} +rmdir($dir); +echo $failed ? "Failed\n" : "Done\n"; +?> +--CLEAN-- + +--EXPECT-- +Done diff --git a/ext/opcache/tests/gh24319_002.phpt b/ext/opcache/tests/gh24319_002.phpt new file mode 100644 index 000000000000..70cc0e60c88b --- /dev/null +++ b/ext/opcache/tests/gh24319_002.phpt @@ -0,0 +1,99 @@ +--TEST-- +GH-24319: Existing OPcache aliases follow later direct-to-indirect conversions +--DESCRIPTION-- +An alias can already point to a direct cache entry when that entry is converted +into another alias. Lookups must resolve the resulting chain, including after +the final target is recompiled. Script enumeration must distinguish direct +entries from aliases using a consistent snapshot. A conversion whose normalized +target is the entry itself must not create an alias cycle. Protected shared +memory is enabled to ensure that read-side atomic operations do not write. +--EXTENSIONS-- +opcache +--SKIPIF-- + +--INI-- +opcache.enable=1 +opcache.enable_cli=1 +opcache.jit=disable +opcache.jit_buffer_size=0 +opcache.file_update_protection=0 +opcache.validate_timestamps=0 +opcache.revalidate_path=0 +opcache.protect_memory=1 +--FILE-- + 1, 'b' => 2, 'c' => 3] as $name => $value) { + file_put_contents("$dir/$name.php", " 'b', 'b' => 'c'] as $old => $new) { + opcache_invalidate("$dir/$old.php", true); + symlink("$dir/$new.php", "$dir/link"); + rename("$dir/link", "$dir/$old.php"); + clearstatcache(true); + var_dump(require "$dir/$old.php"); + var_dump(require "$dir/alias.php"); +} +$scripts = opcache_get_status()['scripts']; +var_dump(isset($scripts["$dir/c.php"])); +var_dump(isset($scripts["$dir/a.php"]), isset($scripts["$dir/b.php"]), isset($scripts["$dir/alias.php"])); + +// Replacing the final script must also update every existing alias chain. +file_put_contents("$dir/c.php", " +--CLEAN-- + +--EXPECT-- +int(1) +int(1) +int(2) +int(2) +int(3) +int(3) +bool(true) +bool(false) +bool(false) +bool(false) +int(6) +int(6) +int(6) +int(6) +int(4) +int(4) +int(5) +int(5) diff --git a/ext/opcache/zend_accelerator_hash.c b/ext/opcache/zend_accelerator_hash.c index 2fd3dfb5ed56..f414d7df72b1 100644 --- a/ext/opcache/zend_accelerator_hash.c +++ b/ext/opcache/zend_accelerator_hash.c @@ -33,7 +33,7 @@ void zend_accel_hash_clean(zend_accel_hash *accel_hash) { accel_hash->num_entries = 0; accel_hash->num_direct_entries = 0; - memset(accel_hash->hash_table, 0, sizeof(zend_accel_hash_entry *)*accel_hash->max_num_entries); + memset(accel_hash->hash_table, 0, sizeof(*accel_hash->hash_table)*accel_hash->max_num_entries); } void zend_accel_hash_init(zend_accel_hash *accel_hash, uint32_t hash_size) @@ -52,7 +52,7 @@ void zend_accel_hash_init(zend_accel_hash *accel_hash, uint32_t hash_size) accel_hash->max_num_entries = hash_size; /* set up hash pointers table */ - accel_hash->hash_table = zend_shared_alloc(sizeof(zend_accel_hash_entry *)*accel_hash->max_num_entries); + accel_hash->hash_table = zend_shared_alloc(sizeof(*accel_hash->hash_table)*accel_hash->max_num_entries); if (!accel_hash->hash_table) { zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Insufficient shared memory!"); return; @@ -64,7 +64,7 @@ void zend_accel_hash_init(zend_accel_hash *accel_hash, uint32_t hash_size) zend_accel_error_noreturn(ACCEL_LOG_FATAL, "Insufficient shared memory!"); return; } - memset(accel_hash->hash_table, 0, sizeof(zend_accel_hash_entry *)*accel_hash->max_num_entries); + memset(accel_hash->hash_table, 0, sizeof(*accel_hash->hash_table)*accel_hash->max_num_entries); } /* Returns NULL if hash is full @@ -78,10 +78,12 @@ zend_accel_hash_entry* zend_accel_hash_update(zend_accel_hash *accel_hash, zend_ zend_accel_hash_entry *entry; zend_accel_hash_entry *indirect_bucket = NULL; + ZEND_ASSERT(data != NULL && !((uintptr_t)data & ZEND_ACCEL_HASH_INDIRECT)); if (indirect) { + uintptr_t target; indirect_bucket = (zend_accel_hash_entry*)data; - while (indirect_bucket->indirect) { - indirect_bucket = (zend_accel_hash_entry*)indirect_bucket->data; + while ((target = zend_accel_hash_load_data(indirect_bucket)) & ZEND_ACCEL_HASH_INDIRECT) { + indirect_bucket = zend_accel_hash_indirect_target(target); } } @@ -92,25 +94,29 @@ zend_accel_hash_entry* zend_accel_hash_update(zend_accel_hash *accel_hash, zend_ index = hash_value % accel_hash->max_num_entries; /* try to see if the element already exists in the hash */ - entry = accel_hash->hash_table[index]; + entry = zend_accel_hash_load_entry(&accel_hash->hash_table[index]); while (entry) { if (entry->hash_value == hash_value && zend_string_equals(entry->key, key)) { - - if (entry->indirect) { - if (indirect_bucket) { - entry->data = indirect_bucket; - } else { - ((zend_accel_hash_entry*)entry->data)->data = data; + uintptr_t old_data = zend_accel_hash_load_data(entry); + + if (indirect_bucket) { + /* The target was resolved under the writer lock. Do not create + * a self-reference when an existing alias resolves back to us. */ + if (indirect_bucket != entry) { + if (!(old_data & ZEND_ACCEL_HASH_INDIRECT)) { + accel_hash->num_direct_entries--; + } + zend_accel_hash_store_data(entry, + (uintptr_t)indirect_bucket | ZEND_ACCEL_HASH_INDIRECT); } } else { - if (indirect_bucket) { - accel_hash->num_direct_entries--; - entry->data = indirect_bucket; - entry->indirect = 1; - } else { - entry->data = data; + zend_accel_hash_entry *target = entry; + while (old_data & ZEND_ACCEL_HASH_INDIRECT) { + target = zend_accel_hash_indirect_target(old_data); + old_data = zend_accel_hash_load_data(target); } + zend_accel_hash_store_data(target, (uintptr_t)data); } return entry; } @@ -124,17 +130,16 @@ zend_accel_hash_entry* zend_accel_hash_update(zend_accel_hash *accel_hash, zend_ entry = &accel_hash->hash_entries[accel_hash->num_entries++]; if (indirect) { - entry->data = indirect_bucket; - entry->indirect = 1; + entry->data = (uintptr_t)indirect_bucket | ZEND_ACCEL_HASH_INDIRECT; } else { accel_hash->num_direct_entries++; - entry->data = data; - entry->indirect = 0; + entry->data = (uintptr_t)data; } entry->hash_value = hash_value; entry->key = key; - entry->next = accel_hash->hash_table[index]; - accel_hash->hash_table[index] = entry; + entry->next = zend_accel_hash_load_entry(&accel_hash->hash_table[index]); + /* Publish only after both the entry and its referenced data are ready. */ + zend_accel_hash_store_entry(&accel_hash->hash_table[index], entry); return entry; } @@ -150,23 +155,15 @@ static zend_always_inline void* zend_accel_hash_find_ex(zend_accel_hash *accel_h #endif index = hash_value % accel_hash->max_num_entries; - entry = accel_hash->hash_table[index]; + entry = zend_accel_hash_load_entry(&accel_hash->hash_table[index]); while (entry) { if (entry->hash_value == hash_value && zend_string_equals(entry->key, key)) { - if (entry->indirect) { - if (data) { - return ((zend_accel_hash_entry*)entry->data)->data; - } else { - return entry->data; - } - } else { - if (data) { - return entry->data; - } else { - return entry; - } + uintptr_t value; + while ((value = zend_accel_hash_load_data(entry)) & ZEND_ACCEL_HASH_INDIRECT) { + entry = zend_accel_hash_indirect_target(value); } + return data ? (void *)value : entry; } entry = entry->next; } @@ -188,35 +185,3 @@ zend_accel_hash_entry* zend_accel_hash_find_entry(zend_accel_hash *accel_hash, z { return (zend_accel_hash_entry *)zend_accel_hash_find_ex(accel_hash, key, 0); } - -int zend_accel_hash_unlink(zend_accel_hash *accel_hash, zend_string *key) -{ - zend_ulong hash_value; - zend_ulong index; - zend_accel_hash_entry *entry, *last_entry=NULL; - - hash_value = zend_string_hash_val(key); -#ifndef ZEND_WIN32 - hash_value ^= ZCG(root_hash); -#endif - index = hash_value % accel_hash->max_num_entries; - - entry = accel_hash->hash_table[index]; - while (entry) { - if (entry->hash_value == hash_value - && zend_string_equals(entry->key, key)) { - if (!entry->indirect) { - accel_hash->num_direct_entries--; - } - if (last_entry) { - last_entry->next = entry->next; - } else { - accel_hash->hash_table[index] = entry->next; - } - return SUCCESS; - } - last_entry = entry; - entry = entry->next; - } - return FAILURE; -} diff --git a/ext/opcache/zend_accelerator_hash.h b/ext/opcache/zend_accelerator_hash.h index 755d3f13ec51..66e34b4d1554 100644 --- a/ext/opcache/zend_accelerator_hash.h +++ b/ext/opcache/zend_accelerator_hash.h @@ -23,6 +23,7 @@ #define ZEND_ACCELERATOR_HASH_H #include "zend.h" +#include "zend_atomic.h" /* zend_accel_hash - is a hash table allocated in shared memory and @@ -36,8 +37,9 @@ however, entries are still taken from the same 'hash_entries' array. 'key' and 'data' passed to zend_accel_hash_update() must be already allocated in shared memory. Few keys may be resolved to the same data. - using 'indirect' entries, that point to other entries ('data' is actually - a pointer to another zend_accel_hash_entry). + using indirect entries, whose data is a tagged pointer to another entry. + The pointer and its kind are published together because readers do not + take the writer lock. Entries and scripts remain allocated until restart. zend_accel_hash_update() requires exclusive lock, however, zend_accel_hash_find() does not. */ @@ -47,13 +49,64 @@ typedef struct _zend_accel_hash_entry zend_accel_hash_entry; struct _zend_accel_hash_entry { zend_ulong hash_value; zend_string *key; + /* Immutable after the entry is published, until zend_accel_hash_clean() + * reuses all entries during an OPcache restart (when no readers remain). */ zend_accel_hash_entry *next; - void *data; - bool indirect; + /* Tagged pointer: bit 0 selects an indirect entry; otherwise a script. */ + uintptr_t data; }; +#define ZEND_ACCEL_HASH_INDIRECT ((uintptr_t) 1) + +static zend_always_inline zend_accel_hash_entry *zend_accel_hash_load_entry(const uintptr_t *ptr) +{ + return (zend_accel_hash_entry *)zend_atomic_load_acquire_uintptr(ptr); +} + +static zend_always_inline void zend_accel_hash_store_entry(uintptr_t *ptr, zend_accel_hash_entry *value) +{ + zend_atomic_store_release_uintptr(ptr, (uintptr_t)value); +} + +static zend_always_inline uintptr_t zend_accel_hash_load_data(const zend_accel_hash_entry *entry) +{ + return zend_atomic_load_acquire_uintptr(&entry->data); +} + +static zend_always_inline void zend_accel_hash_store_data(zend_accel_hash_entry *entry, uintptr_t value) +{ + zend_atomic_store_release_uintptr(&entry->data, value); +} + +static zend_always_inline zend_accel_hash_entry *zend_accel_hash_indirect_target(uintptr_t data) +{ + ZEND_ASSERT(data & ZEND_ACCEL_HASH_INDIRECT); + return (zend_accel_hash_entry *)(data & ~ZEND_ACCEL_HASH_INDIRECT); +} + +/* An entry returned by find_entry() can become indirect before the caller + * uses it. Resolve each atomically loaded value, including newly formed chains. */ +static zend_always_inline void *zend_accel_hash_entry_data(const zend_accel_hash_entry *entry) +{ + while (1) { + uintptr_t data = zend_accel_hash_load_data(entry); + if (!(data & ZEND_ACCEL_HASH_INDIRECT)) { + return (void *)data; + } + entry = zend_accel_hash_indirect_target(data); + } +} + +/* Enumerators need the kind and data from the same snapshot. */ +static zend_always_inline void *zend_accel_hash_direct_data(const zend_accel_hash_entry *entry) +{ + uintptr_t data = zend_accel_hash_load_data(entry); + return (data & ZEND_ACCEL_HASH_INDIRECT) ? NULL : (void *)data; +} + typedef struct _zend_accel_hash { - zend_accel_hash_entry **hash_table; + /* Bucket heads are untagged entry addresses. */ + uintptr_t *hash_table; zend_accel_hash_entry *hash_entries; uint32_t num_entries; uint32_t max_num_entries; @@ -79,10 +132,6 @@ zend_accel_hash_entry* zend_accel_hash_find_entry( zend_accel_hash *accel_hash, zend_string *key); -int zend_accel_hash_unlink( - zend_accel_hash *accel_hash, - zend_string *key); - static inline bool zend_accel_hash_is_full(zend_accel_hash *accel_hash) { if (accel_hash->num_entries == accel_hash->max_num_entries) { diff --git a/ext/opcache/zend_accelerator_module.c b/ext/opcache/zend_accelerator_module.c index 902d69374bb7..562291a47ca8 100644 --- a/ext/opcache/zend_accelerator_module.c +++ b/ext/opcache/zend_accelerator_module.c @@ -616,14 +616,13 @@ static int accelerator_get_scripts(zval *return_value) array_init(return_value); for (i = 0; inext) { + for (cache_entry = zend_accel_hash_load_entry(&ZCSG(hash).hash_table[i]); cache_entry; cache_entry = cache_entry->next) { zend_persistent_script *script; char *str; size_t len; - if (cache_entry->indirect) continue; - - script = (zend_persistent_script *)cache_entry->data; + script = zend_accel_hash_direct_data(cache_entry); + if (!script) continue; array_init(&persistent_script_report); add_assoc_str(&persistent_script_report, "full_path", zend_string_dup(script->script.filename, 0)); diff --git a/ext/opcache/zend_file_cache.c b/ext/opcache/zend_file_cache.c index a8463e4252e6..53c25bb3d367 100644 --- a/ext/opcache/zend_file_cache.c +++ b/ext/opcache/zend_file_cache.c @@ -1922,7 +1922,7 @@ zend_persistent_script *zend_file_cache_script_load(zend_file_handle *file_handl * exclusive lock) */ bucket = zend_accel_hash_find_entry(&ZCSG(hash), full_path); if (bucket) { - script = (zend_persistent_script *)bucket->data; + script = zend_accel_hash_entry_data(bucket); if (!script->corrupted) { zend_shared_alloc_unlock(); zend_arena_release(&CG(arena), checkpoint);