Don't unserialize iterator values while holding the cache lock by iliaal · Pull Request #627 · krakjoe/apcu

iliaal · 2026-06-30T21:08:59Z

APCUIterator builds each item, including unserializing the stored value, while holding the cache read lock. Objects are always stored serialized, so this runs __wakeup() / Serializable::unserialize() under the lock. If that userland code re-enters apcu with a write operation, it takes the write lock while the read lock is held and the worker self-deadlocks. On recursive-mutex builds the nested write succeeds instead and can free or relocate the entry mid-iteration, a use-after-free.

Reproduce (hangs the worker):

class C { function __wakeup() { apcu_store('x', 1); } }
apcu_store('obj', new C());
foreach (new APCUIterator(null, APC_ITER_VALUE) as $_) {}

apc_cache_fetch() already avoids this: it increfs the entry, releases the lock, then unserializes. This applies the same pattern to the iterator. Under the lock it pins (increfs) the matching entries, releases the lock, then builds the items and drops the references. Pinning blocks defragmentation and freeing (apc_cache_wlocked_move_entry refuses ref_count > 0), so the entry pointers stay valid across the unlock. Includes a regression test.

madmajestro · 2026-07-01T11:24:21Z

Thanks for your contribution! At first glance, this looks like a problem worth taking a closer look at.

I'm currently on vacation for two weeks. I'll probably have not much time for reviews during this period, but I'll try.

madmajestro · 2026-07-03T21:23:23Z

This file must be added to package.xml

The file could be named iterator_012.phpt to align it's name with the other iterator tests.

Renamed to iterator_012.phpt and added it to package.xml.

madmajestro · 2026-07-03T21:38:18Z

+						if (apc_iterator_search_match(iterator, entry)) {
+							count++;
+							apc_cache_entry_incref(cache, entry);
+							apc_stack_push(entries, entry);


apc_stack_push() can call erealloc() and if my understanding is right this can cause a bailout in oom situations. Therefore, apc_stack_push() should be called before apc_cache_entry_incref(). Otherwise the incremented reference counter of the entry which causes the bailout will not be decremented in the finally block.

Right, with incref first a bailout in the push would leak the pin. Reordered so apc_stack_push() runs before the incref.

madmajestro · 2026-07-03T22:40:08Z

 		return count;
 	}

+	entries = apc_stack_create(iterator->chunk_size);


Creating a new stack (2 x emalloc()) while holding the lock seems suboptimal. It should be created either before acquiring the lock or when the iterator is created (iterator contains two stacks). The latter is probably better, because it avoids the need to create / destroy a new stack for each iterator fetch / chunk.

Looking at this i wonder if we need two stacks at all:

It should be possible to use one stack for both. All we need is to implement a new function apc_stack_set() in apc_stack.[ch] which enables us to set (replace) the value on the stack.

The loop which uses apc_iterator_item_ctor() to construct the items could decrement the reference counter and replace the pointer to the entry by a pointer to an item.

it must be ensured that no reference to an entry remains on the stack.

I would like to keep the performance impact of this change as low as possible. TBH it has the chance to improve the situation in some cases due to reduced lock contention.

Moved the entries stack onto the iterator, created once at construction and cleared (not reallocated) per fetch, so there's no allocation under the lock and no per-chunk create/destroy.

I'd keep it separate from iterator->stack rather than folding both into one via apc_stack_set(): the two have different bailout lifetimes. Entries must be fully released on any bailout; items get handed to the caller. With one stack, a bailout inside apc_iterator_item_ctor() mid-build leaves the finally unable to tell which slots are still entries to release versus finished items. Two stacks encode that boundary structurally.

madmajestro · 2026-07-03T23:36:30Z

+		/* Build the items with the lock released. */
+		for (i = 0; i < apc_stack_size(entries); i++) {
+			item = apc_iterator_item_ctor(iterator, apc_stack_get(entries, i));
+			if (item) {


This if should be removed, because item should always be present. A bailout should occur in apc_iterator_item_ctor() otherwise.

Removed. apc_iterator_item_ctor() bails on OOM rather than returning NULL, so the check was dead.

madmajestro

The only change I can see is that the test has been renamed. Did you perhaps not commit or push all changes?

iliaal · 2026-07-07T13:34:48Z

The only change I can see is that the test has been renamed. Did you perhaps not commit or push all changes?

Helps to do proper push, the changes are there now, sorry 🤦

madmajestro

I only noticed a few minor things / nits. I think we're almost there.

madmajestro · 2026-07-07T21:24:09Z

+		for (i = 0; i < apc_stack_size(entries); i++) {
+			apc_cache_entry_release(cache, apc_stack_get(entries, i));
+		}
+		apc_stack_clear(entries);


Suggested change

for (i = 0; i < apc_stack_size(entries); i++) {

apc_cache_entry_release(cache, apc_stack_get(entries, i));

}

apc_stack_clear(entries);

while (apc_stack_size(entries) > 0) {

apc_cache_entry_release(cache, apc_stack_pop(entries));

}

madmajestro · 2026-07-07T21:30:06Z

@@ -329,6 +363,7 @@ void apc_iterator_obj_init(apc_iterator_t *iterator, zval *search, zend_long for
 	iterator->key_idx = 0;
 	iterator->chunk_size = chunk_size == 0 ? APC_DEFAULT_CHUNK_SIZE : chunk_size;
 	iterator->stack = apc_stack_create(chunk_size);


Suggested change

iterator->stack = apc_stack_create(chunk_size);

iterator->stack = apc_stack_create(iterator->chunk_size);

I think it makes sense to init both stacks with the same capacity. In addition, this respects APC_DEFAULT_CHUNK_SIZE here, too.

madmajestro · 2026-07-07T21:40:20Z

 	size_t count = 0;
-	apc_iterator_item_t *item;
+	int i;
+	apc_stack_t *entries = iterator->entries;


I propose to remove this variable. We can use iterator->entries below.

Removed, using iterator->entries directly in both fetch functions.

madmajestro · 2026-07-07T21:52:03Z

 	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;    /* scratch stack of pinned entries, reused per fetch */


Suggested change

apc_stack_t *entries; /* scratch stack of pinned entries, reused per fetch */

apc_stack_t *entries; /* stack of pinned entries */

I propose to keep the comment similar for both stacks to prevent confusion (e.g, both are reused per fetch)

madmajestro · 2026-07-07T22:15:57Z

    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.



Can you add a changelog entry?

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.

madmajestro

LGTM!

Thank you!

madmajestro requested changes Jul 3, 2026

View reviewed changes

madmajestro reviewed Jul 3, 2026

View reviewed changes

iliaal force-pushed the fix/iterator-unserialize-under-lock branch from 9594732 to da07045 Compare July 7, 2026 12:26

iliaal requested a review from madmajestro July 7, 2026 12:28

madmajestro reviewed Jul 7, 2026

View reviewed changes

iliaal force-pushed the fix/iterator-unserialize-under-lock branch from da07045 to 219c04d Compare July 7, 2026 13:33

iliaal requested a review from madmajestro July 7, 2026 13:50

madmajestro reviewed Jul 7, 2026

View reviewed changes

iliaal force-pushed the fix/iterator-unserialize-under-lock branch from 219c04d to 3c41999 Compare July 8, 2026 00:24

iliaal requested a review from madmajestro July 8, 2026 13:13

madmajestro approved these changes Jul 8, 2026

View reviewed changes

madmajestro merged commit bc9e296 into krakjoe:master Jul 8, 2026
1 check passed

	iterator->stack = apc_stack_create(chunk_size);
	iterator->stack = apc_stack_create(iterator->chunk_size);

	apc_stack_t entries; / scratch stack of pinned entries, reused per fetch */
	apc_stack_t entries; / stack of pinned entries */

Conversation

iliaal commented Jun 30, 2026

Uh oh!

madmajestro commented Jul 1, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madmajestro left a comment

Choose a reason for hiding this comment

Uh oh!

iliaal commented Jul 7, 2026

Uh oh!

madmajestro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

madmajestro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants