Skip to content

Commit 3fc40d5

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". Reset the counter in the SPL request init so each request starts at zero regardless of how the previous one exited.
1 parent 3ed338c commit 3fc40d5

4 files changed

Lines changed: 50 additions & 0 deletions

File tree

ext/spl/php_spl.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,7 @@ PHP_MINIT_FUNCTION(spl)
563563
PHP_RINIT_FUNCTION(spl) /* {{{ */
564564
{
565565
spl_autoload_extensions = NULL;
566+
spl_object_storage_reset_get_hash_depth();
566567
return SUCCESS;
567568
} /* }}} */
568569

ext/spl/spl_observer.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,11 @@ static zend_object_handlers spl_handler_MultipleIterator;
4747

4848
ZEND_TLS uint32_t spl_object_storage_get_hash_depth;
4949

50+
void spl_object_storage_reset_get_hash_depth(void)
51+
{
52+
spl_object_storage_get_hash_depth = 0;
53+
}
54+
5055
typedef struct _spl_SplObjectStorage { /* {{{ */
5156
HashTable storage;
5257
zend_long index;

ext/spl/spl_observer.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,4 +31,6 @@ extern PHPAPI zend_class_entry *spl_ce_MultipleIterator;
3131

3232
PHP_MINIT_FUNCTION(spl_observer);
3333

34+
void spl_object_storage_reset_get_hash_depth(void);
35+
3436
#endif /* SPL_OBSERVER_H */
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)