Skip to content

Commit 58eada0

Browse files
committed
Fix SplObjectStorage getHash() guard leaking across a bailout
The getHash() recursion guard increments a request-persistent counter around the userland getHash() call but decrements it only on the normal return path. A bailout inside an overridden getHash() (out-of-memory, timeout, or any fatal) skips the decrement, and the counter is never reset per request, so on a persistent SAPI every later request on the same worker wrongly throws "Modification of SplObjectStorage during getHash() is prohibited". Decrement inside a zend_catch and re-raise the bailout so the counter is balanced on every path.
1 parent 3ed338c commit 58eada0

2 files changed

Lines changed: 51 additions & 1 deletion

File tree

ext/spl/spl_observer.c

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,16 @@ static zend_result spl_object_storage_get_hash(zend_hash_key *key, spl_SplObject
9494
ZVAL_OBJ(&param, obj);
9595
ZVAL_UNDEF(&rv);
9696
spl_object_storage_get_hash_depth++;
97-
zend_call_method_with_1_params(&intern->std, intern->std.ce, &intern->fptr_get_hash, "getHash", &rv, &param);
97+
bool bailout = false;
98+
zend_try {
99+
zend_call_method_with_1_params(&intern->std, intern->std.ce, &intern->fptr_get_hash, "getHash", &rv, &param);
100+
} zend_catch {
101+
bailout = true;
102+
} zend_end_try();
98103
spl_object_storage_get_hash_depth--;
104+
if (UNEXPECTED(bailout)) {
105+
zend_bailout();
106+
}
99107
if (UNEXPECTED(Z_ISUNDEF(rv))) {
100108
/* An exception has occurred */
101109
return FAILURE;
Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
--TEST--
2+
SplObjectStorage getHash() depth counter is reset after a bailout in a user getHash()
3+
--SKIPIF--
4+
<?php
5+
include "skipif.inc";
6+
?>
7+
--INI--
8+
allow_url_fopen=1
9+
--FILE--
10+
<?php
11+
include "php_cli_server.inc";
12+
13+
$code = <<<'PHP'
14+
if ($_SERVER["REQUEST_URI"] === "/poison") {
15+
class Poison extends SplObjectStorage {
16+
public function getHash($o): string {
17+
ini_set("memory_limit", "2M");
18+
str_repeat("a", 100 * 1024 * 1024);
19+
return "x";
20+
}
21+
}
22+
(new Poison())->offsetSet(new stdClass());
23+
echo "poison";
24+
} else {
25+
$s = new SplObjectStorage();
26+
$s->offsetSet(new stdClass());
27+
echo "check-ok count=", count($s);
28+
}
29+
PHP;
30+
31+
php_cli_server_start($code, 'router.php');
32+
33+
$base = 'http://' . PHP_CLI_SERVER_ADDRESS;
34+
// Request 1 bails out (OOM) inside the overridden getHash() mid-offsetSet.
35+
@file_get_contents($base . '/poison');
36+
// A later request on the same worker must not be poisoned by a stuck counter.
37+
echo @file_get_contents($base . '/check'), "\n";
38+
echo @file_get_contents($base . '/check'), "\n";
39+
?>
40+
--EXPECT--
41+
check-ok count=1
42+
check-ok count=1

0 commit comments

Comments
 (0)