[Marti2203]

php_array_data_shuffle() first compacts the holes of a packed array by moving the remaining values down with ZVAL_COPY_VALUE(), and only sets nNumUsed after the shuffle loop:

		if (hash->nNumUsed != hash->nNumOfElements) {
			for (j = 0, idx = 0; idx < hash->nNumUsed; idx++) {
				...
					ZVAL_COPY_VALUE(&hash->arPacked[j], zv);
				...
			}
		}
		while (--n_left) {
			rnd_idx = algo->range(state, 0, n_left);
			if (EG(exception)) {
				return false;      /* nNumUsed still counts the old tail */
			}
			...
		}
	...
	hash->nNumUsed = n_elems;

If the engine throws during the loop, the function returns with the old nNumUsed. The tail slots still hold copies of values that were moved down, so those values are counted twice. Randomizer::shuffleArray() then returns with RETURN_THROWS() and its copy of the array is destroyed, which releases the moved values twice. The caller's original array is left pointing at a freed value.

This needs a user-supplied Random\Engine that throws (or whose generate() returns an empty string, which also throws), and an array with holes. shuffle() uses the default engine, which does not throw, and arrays without holes are not affected.

Reproducer

<?php
final class ThrowingEngine implements Random\Engine {
    public function generate(): string { throw new Exception("boom"); }
}
$a = [new stdClass, new stdClass, new stdClass];
unset($a[1]);
$r = new Random\Randomizer(new ThrowingEngine);
try {
    $r->shuffleArray($a);
} catch (Exception $e) {
    echo "caught: ", $e->getMessage(), "\n";
}
var_dump($a);
echo "done\n";

• Before: release build segfaults in var_dump($a) while printing $a[2]. An ASan build with USE_ZEND_ALLOC=0 reports heap-use-after-free ... READ of size 8 in php_var_dump(); the object was freed by zend_objects_store_del() from zend_array_destroy() of the copy.
• After: prints both objects and done, no ASan report.

Fix

Set nNumUsed to the number of elements right after the compaction, in both the regular and the iterator branch, so the table is consistent before the engine is called. The success path is unchanged. It still sets nNumUsed, nInternalPointer and nNextFreeElement at the end.

Test

ext/random/tests/03_randomizer/methods/shuffleArray_engine_throws_with_holes.phpt shuffles an array with a hole, using an engine that throws on the first call and one that throws on the second call. It checks that the original array is intact afterwards.

• Without the fix: the test crashes (Termsig=11) in the release build, and ASan reports a heap-use-after-free.
• With the fix: passes in the release build and with ASan (USE_ZEND_ALLOC=0).
• ext/random/tests and ext/standard/tests/array: 909 passed, 14 skipped (release build). Under ASan, the same set passes except 4 stack-limit tests (count_recursive_stack_limit.phpt, gh23111.phpt, gh23113.phpt, gh23115.phpt). They fail the same way without this change.

This issue was found by an LLM-Enhanced Static Analysis Engine and verified manually.

🤖 Generated with Claude Code

[TimWolla]

As a ext/random maintainer, the test case LGTM. I defer the review of the C code to someone else.

[ndossche]

I think it's better to update nNumUsed (and in my opinion also nInternalPointer and nNextFreeElement) to after the compaction happens. That means that lines 3346-3348 (pre-patch line numbers) will disappear from the end and will be duplicated 2 times: once after each compaction loop.

[ndossche]

This comment is a bit misleading, it's not "dropping" it's rather "truncating" the stale slots

[Marti2203]

Thanks, done: the counters are now reset right after compaction in both branches (before the engine can throw), the trailing reset is gone, and the comment says "truncate". ext/random and ext/standard/tests/array pass.

[ndossche]

This is good, thanks. Leaving open a bit to give other maintainers/codeowners some time to review too if they want