From 3c41999a0cecec0d43d776645061d3a5f9a4d565 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Tue, 30 Jun 2026 17:06:28 -0400 Subject: [PATCH] Don't unserialize iterator values while holding the cache lock APCUIterator built each item, including unserializing the stored value, while still holding the cache read lock. For objects (always stored serialized) this runs userland code via __wakeup() / Serializable:: unserialize() under the lock. If that code re-enters apcu with a write operation it requests the write lock while the read lock is held, which self-deadlocks the worker (and stalls every other worker's writes for the duration); on recursive-mutex builds the nested write instead succeeds and can free or relocate the entry mid-iteration, a use-after-free. apc_cache_fetch() already avoids this by incref'ing the entry, releasing the lock, and only then unserializing. Apply the same discipline to the iterator: under the lock, collect and pin (incref) the matching entries; release the lock; then build the items and drop the references. Pinning blocks defragmentation (apc_cache_wlocked_move_entry refuses ref_count > 0) and freeing, so the entry pointers stay valid across the unlock. The reported ref_count excludes the iterator's own transient reference. --- apc_cache.c | 5 +++ apc_cache.h | 8 ++++ apc_iterator.c | 99 +++++++++++++++++++++++++++-------------- apc_iterator.h | 3 +- package.xml | 3 ++ tests/iterator_012.phpt | 35 +++++++++++++++ 6 files changed, 118 insertions(+), 35 deletions(-) create mode 100644 tests/iterator_012.phpt diff --git a/apc_cache.c b/apc_cache.c index 730b8629..1c5a8cdd 100644 --- a/apc_cache.c +++ b/apc_cache.c @@ -693,6 +693,11 @@ PHP_APCU_API zend_bool apc_cache_preload(apc_cache_t* cache, const char *path) #endif } +PHP_APCU_API void apc_cache_entry_incref(apc_cache_t *cache, apc_cache_entry_t *entry) +{ + ATOMIC_INC_RLOCKED(entry->ref_count); +} + PHP_APCU_API void apc_cache_entry_release(apc_cache_t *cache, apc_cache_entry_t *entry) { ATOMIC_DEC(entry->ref_count); diff --git a/apc_cache.h b/apc_cache.h index c2a833b5..e13fb4fb 100644 --- a/apc_cache.h +++ b/apc_cache.h @@ -200,6 +200,14 @@ PHP_APCU_API zend_bool apc_cache_entry_fetch_zval( */ PHP_APCU_API void apc_cache_entry_release(apc_cache_t *cache, apc_cache_entry_t *entry); +/* + * apc_cache_entry_incref increments the reference count of a cache entry to pin it, + * preventing it from being freed or relocated by defragmentation while a reference is + * held. It must be called while holding the cache read lock; release the reference with + * apc_cache_entry_release once the entry is no longer needed. + */ +PHP_APCU_API void apc_cache_entry_incref(apc_cache_t *cache, apc_cache_entry_t *entry); + /* * fetches information about the cache provided for userland status functions */ diff --git a/apc_iterator.c b/apc_iterator.c index cb7edc3b..b6c8a03a 100644 --- a/apc_iterator.c +++ b/apc_iterator.c @@ -93,7 +93,9 @@ static apc_iterator_item_t* apc_iterator_item_ctor( zend_hash_add_new(ht, apc_str_access_time, &zv); } if (APC_ITER_REFCOUNT & iterator->format) { - ZVAL_LONG(&zv, entry->ref_count); + /* The iterator holds one reference to pin the entry during construction; + * exclude it so the reported count reflects the cache-visible value. */ + ZVAL_LONG(&zv, entry->ref_count - 1); zend_hash_add_new(ht, apc_str_ref_count, &zv); } if (APC_ITER_MEM_SIZE & iterator->format) { @@ -127,6 +129,7 @@ static void apc_iterator_free(zend_object *object) { } apc_stack_destroy(iterator->stack); + apc_stack_destroy(iterator->entries); if (iterator->regex) { zend_string_release(iterator->regex); @@ -153,6 +156,7 @@ zend_object* apc_iterator_create(zend_class_entry *ce) { iterator->initialized = 0; iterator->stack = NULL; + iterator->entries = NULL; iterator->regex = NULL; iterator->search_hash = NULL; iterator->obj.handlers = &apc_iterator_object_handlers; @@ -199,7 +203,7 @@ static int apc_iterator_check_expiry(apc_cache_t* cache, apc_cache_entry_t *entr static size_t apc_iterator_fetch_active(apc_iterator_t *iterator) { apc_cache_t *cache = apc_user_cache; size_t count = 0; - apc_iterator_item_t *item; + int i; time_t t = apc_time(); while (apc_stack_size(iterator->stack) > 0) { @@ -210,27 +214,43 @@ static size_t apc_iterator_fetch_active(apc_iterator_t *iterator) { return count; } + /* The outer try guarantees the references pinned below are dropped (and the entries + * stack cleared) even if a bailout occurs at any point. */ php_apc_try { - while (count <= iterator->chunk_size && iterator->slot_idx < cache->nslots) { - uintptr_t entry_offset = cache->slots[iterator->slot_idx]; - while (entry_offset) { - apc_cache_entry_t *entry = ENTRYAT(entry_offset); - if (apc_iterator_check_expiry(cache, entry, t)) { - if (apc_iterator_search_match(iterator, entry)) { - count++; - item = apc_iterator_item_ctor(iterator, entry); - if (item) { - apc_stack_push(iterator->stack, item); + /* Collect the matching entries under the lock, pinning each with a reference. + * The values must not be unserialized here: unserialization runs userland code + * (__wakeup / Serializable::unserialize) that can re-enter apcu and deadlock + * against the held lock (or, on recursive-mutex builds, free the entry + * mid-iteration). */ + php_apc_try { + while (count <= iterator->chunk_size && iterator->slot_idx < cache->nslots) { + uintptr_t entry_offset = cache->slots[iterator->slot_idx]; + while (entry_offset) { + apc_cache_entry_t *entry = ENTRYAT(entry_offset); + if (apc_iterator_check_expiry(cache, entry, t)) { + if (apc_iterator_search_match(iterator, entry)) { + count++; + apc_stack_push(iterator->entries, entry); + apc_cache_entry_incref(cache, entry); } } + entry_offset = entry->next; } - entry_offset = entry->next; + iterator->slot_idx++; } - iterator->slot_idx++; + } php_apc_finally { + iterator->stack_idx = 0; + apc_cache_runlock(cache); + } php_apc_end_try(); + + /* Build the items with the lock released. */ + for (i = 0; i < apc_stack_size(iterator->entries); i++) { + apc_stack_push(iterator->stack, apc_iterator_item_ctor(iterator, apc_stack_get(iterator->entries, i))); } } php_apc_finally { - iterator->stack_idx = 0; - apc_cache_runlock(cache); + while (apc_stack_size(iterator->entries) > 0) { + apc_cache_entry_release(cache, apc_stack_pop(iterator->entries)); + } } php_apc_end_try(); return count; @@ -239,34 +259,44 @@ static size_t apc_iterator_fetch_active(apc_iterator_t *iterator) { static size_t apc_iterator_fetch_deleted(apc_iterator_t *iterator) { apc_cache_t *cache = apc_user_cache; size_t count = 0; - apc_iterator_item_t *item; + int i; if (!apc_cache_rlock(cache)) { return count; } + /* As in apc_iterator_fetch_active, collect and pin the matching entries under the + * lock and defer value unserialization until the lock is released. */ php_apc_try { - uintptr_t entry_offset = cache->header->gc; - while (entry_offset && count <= iterator->slot_idx) { - count++; - entry_offset = ENTRYAT(entry_offset)->next; - } - count = 0; - while (entry_offset && count < iterator->chunk_size) { - apc_cache_entry_t *entry = ENTRYAT(entry_offset); - if (apc_iterator_search_match(iterator, entry)) { + php_apc_try { + uintptr_t entry_offset = cache->header->gc; + while (entry_offset && count <= iterator->slot_idx) { count++; - item = apc_iterator_item_ctor(iterator, entry); - if (item) { - apc_stack_push(iterator->stack, item); + entry_offset = ENTRYAT(entry_offset)->next; + } + count = 0; + while (entry_offset && count < iterator->chunk_size) { + apc_cache_entry_t *entry = ENTRYAT(entry_offset); + if (apc_iterator_search_match(iterator, entry)) { + count++; + apc_stack_push(iterator->entries, entry); + apc_cache_entry_incref(cache, entry); } + entry_offset = entry->next; } - entry_offset = entry->next; + } php_apc_finally { + iterator->slot_idx += count; + iterator->stack_idx = 0; + apc_cache_runlock(cache); + } php_apc_end_try(); + + for (i = 0; i < apc_stack_size(iterator->entries); i++) { + apc_stack_push(iterator->stack, apc_iterator_item_ctor(iterator, apc_stack_get(iterator->entries, i))); } } php_apc_finally { - iterator->slot_idx += count; - iterator->stack_idx = 0; - apc_cache_runlock(cache); + while (apc_stack_size(iterator->entries) > 0) { + apc_cache_entry_release(cache, apc_stack_pop(iterator->entries)); + } } php_apc_end_try(); return count; @@ -328,7 +358,8 @@ void apc_iterator_obj_init(apc_iterator_t *iterator, zval *search, zend_long for iterator->stack_idx = 0; iterator->key_idx = 0; iterator->chunk_size = chunk_size == 0 ? APC_DEFAULT_CHUNK_SIZE : chunk_size; - iterator->stack = apc_stack_create(chunk_size); + iterator->stack = apc_stack_create(iterator->chunk_size); + iterator->entries = apc_stack_create(iterator->chunk_size); iterator->format = format; iterator->totals_flag = 0; iterator->count = 0; diff --git a/apc_iterator.h b/apc_iterator.h index 362dcb43..94f37942 100644 --- a/apc_iterator.h +++ b/apc_iterator.h @@ -53,7 +53,8 @@ typedef struct _apc_iterator_t { /* fetch callback to fetch items from cache slots or lists */ size_t slot_idx; /* index to the slot array or linked list */ size_t chunk_size; /* number of entries to pull down per fetch */ - apc_stack_t *stack; /* stack of entries pulled from cache */ + apc_stack_t *stack; /* stack of items pulled from cache */ + apc_stack_t *entries; /* stack of pinned entries */ int stack_idx; /* index into the current stack */ pcre_cache_entry *pce; /* regex filter on entry identifiers */ #if PHP_VERSION_ID >= 70300 diff --git a/package.xml b/package.xml index cd122b75..cf99b41a 100644 --- a/package.xml +++ b/package.xml @@ -63,6 +63,8 @@ the PHP serializer, which is currently the default). - Fix build on macOS. If pcre2 was installed via Homebrew, the header file pcre2.h wasn't found during the build process. + - Fixed self-deadlock in APCUIterator when a stored object's __wakeup() re-enters APCu, by + unserializing values after the cache lock is released instead of while it is held. Internal changes: - Added stress test to CI. This test will hopefully help to detect unexpected behavior @@ -150,6 +152,7 @@ + diff --git a/tests/iterator_012.phpt b/tests/iterator_012.phpt new file mode 100644 index 00000000..abaeb472 --- /dev/null +++ b/tests/iterator_012.phpt @@ -0,0 +1,35 @@ +--TEST-- +APCUIterator: re-entrant apcu access from __wakeup during value iteration +--SKIPIF-- + +--INI-- +apc.enabled=1 +apc.enable_cli=1 +apc.serializer=php +--FILE-- + +--EXPECT-- +bool(true) +int(2) +DONE