Skip to content

Commit d694135

Browse files
prateekbhujelprateekbhujel
authored
Fix GH-21831: Disallow SplObjectStorage mutation during getHash() (#21835)
Fixes GH-21831. `getHash()` runs while `SplObjectStorage` is looking up an object. Letting it mutate a storage from there is unsafe, because the caller may still be iterating the table that just changed underneath it. This makes that fail immediately with an `Error` instead of trying to handle it one caller at a time. The existing concurrent deletion tests now cover the new rule, and GH-21831 has a direct regression test. This targets master, so the behavior change is also noted in `UPGRADING`.
1 parent 87258eb commit d694135

7 files changed

Lines changed: 198 additions & 62 deletions

File tree

‎NEWS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,8 @@ PHP NEWS
160160
- SPL:
161161
. DirectoryIterator key can now work better with filesystem supporting larger
162162
directory indexing. (David Carlier)
163+
. Fixed bug GH-21831 (SplObjectStorage::removeAllExcept() use-after-free
164+
with re-entrant getHash()). (Pratik Bhujel)
163165
. Fix bugs GH-8561, GH-8562, GH-8563, and GH-8564 (Fixing various
164166
SplFileObject iterator desync bugs). (iliaal)
165167

‎UPGRADING‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,8 @@ PHP 8.6 UPGRADE NOTES
7777
logic in their updateTimestamp() method.
7878

7979
- SPL:
80+
. SplObjectStorage::getHash() implementations may no longer mutate any
81+
SplObjectStorage instance. Attempting to do so now throws an Error.
8082
. SplFileObject::next() now advances the stream when no prior current()
8183
call has cached a line. A subsequent current() call returns the new
8284
line rather than the previous one.

‎ext/spl/spl_observer.c‎

Lines changed: 101 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ static zend_object_handlers spl_handler_MultipleIterator;
4545
#define SOS_OVERRIDDEN_WRITE_DIMENSION 2
4646
#define SOS_OVERRIDDEN_UNSET_DIMENSION 4
4747

48+
ZEND_TLS uint32_t spl_object_storage_get_hash_depth;
49+
4850
typedef struct _spl_SplObjectStorage { /* {{{ */
4951
HashTable storage;
5052
zend_long index;
@@ -69,6 +71,16 @@ static inline spl_SplObjectStorage *spl_object_storage_from_obj(zend_object *obj
6971

7072
#define Z_SPLOBJSTORAGE_P(zv) spl_object_storage_from_obj(Z_OBJ_P((zv)))
7173

74+
static zend_always_inline bool spl_object_storage_is_mutating_within_get_hash_call(void)
75+
{
76+
if (UNEXPECTED(spl_object_storage_get_hash_depth)) {
77+
zend_throw_error(NULL, "Modification of SplObjectStorage during getHash() is prohibited");
78+
return true;
79+
}
80+
81+
return false;
82+
}
83+
7284
static void spl_SplObjectStorage_free_storage(zend_object *object) /* {{{ */
7385
{
7486
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
@@ -83,7 +95,10 @@ static zend_result spl_object_storage_get_hash(zend_hash_key *key, spl_SplObject
8395
zval param;
8496
zval rv;
8597
ZVAL_OBJ(&param, obj);
98+
ZVAL_UNDEF(&rv);
99+
spl_object_storage_get_hash_depth++;
86100
zend_call_method_with_1_params(&intern->std, intern->std.ce, &intern->fptr_get_hash, "getHash", &rv, &param);
101+
spl_object_storage_get_hash_depth--;
87102
if (UNEXPECTED(Z_ISUNDEF(rv))) {
88103
/* An exception has occurred */
89104
return FAILURE;
@@ -176,6 +191,10 @@ static spl_SplObjectStorageElement *spl_object_storage_attach_handle(spl_SplObje
176191

177192
static spl_SplObjectStorageElement *spl_object_storage_attach(spl_SplObjectStorage *intern, zend_object *obj, zval *inf) /* {{{ */
178193
{
194+
if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
195+
return NULL;
196+
}
197+
179198
if (EXPECTED(!(intern->flags & SOS_OVERRIDDEN_WRITE_DIMENSION))) {
180199
return spl_object_storage_attach_handle(intern, obj, inf);
181200
}
@@ -221,6 +240,10 @@ static spl_SplObjectStorageElement *spl_object_storage_attach(spl_SplObjectStora
221240

222241
static zend_result spl_object_storage_detach(spl_SplObjectStorage *intern, zend_object *obj) /* {{{ */
223242
{
243+
if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
244+
return FAILURE;
245+
}
246+
224247
if (EXPECTED(!(intern->flags & SOS_OVERRIDDEN_UNSET_DIMENSION))) {
225248
return zend_hash_index_del(&intern->storage, obj->handle);
226249
}
@@ -247,20 +270,24 @@ static zend_result spl_object_storage_detach(spl_SplObjectStorage *intern, zend_
247270
if (UNEXPECTED(Z_ISUNDEF_P(_z))) continue; \
248271
_ptr = Z_PTR_P(_z);
249272

250-
static void spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjectStorage *other) { /* {{{ */
273+
static zend_result spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjectStorage *other) { /* {{{ */
251274
spl_SplObjectStorageElement *element;
252275

253276
SPL_SAFE_HASH_FOREACH_PTR(&other->storage, element) {
254277
zval zv;
255278
zend_object *obj = element->obj;
256279
GC_ADDREF(obj);
257280
ZVAL_COPY(&zv, &element->inf);
258-
spl_object_storage_attach(intern, obj, &zv);
281+
spl_SplObjectStorageElement *attached = spl_object_storage_attach(intern, obj, &zv);
259282
zval_ptr_dtor(&zv);
260283
OBJ_RELEASE(obj);
284+
if (UNEXPECTED(!attached)) {
285+
return FAILURE;
286+
}
261287
} ZEND_HASH_FOREACH_END();
262288

263289
intern->index = 0;
290+
return SUCCESS;
264291
} /* }}} */
265292

266293
#define SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zstr_method) \
@@ -446,6 +473,9 @@ PHP_METHOD(SplObjectStorage, attach)
446473
Z_PARAM_ZVAL(inf)
447474
ZEND_PARSE_PARAMETERS_END();
448475
spl_object_storage_attach(intern, obj, inf);
476+
if (UNEXPECTED(EG(exception))) {
477+
RETURN_THROWS();
478+
}
449479
} /* }}} */
450480

451481
// todo: make spl_object_storage_has_dimension return bool as well
@@ -494,6 +524,10 @@ static zval *spl_object_storage_read_dimension(zend_object *object, zval *offset
494524
static void spl_object_storage_write_dimension(zend_object *object, zval *offset, zval *inf)
495525
{
496526
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
527+
if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
528+
return;
529+
}
530+
497531
if (UNEXPECTED(offset == NULL || Z_TYPE_P(offset) != IS_OBJECT || (intern->flags & SOS_OVERRIDDEN_WRITE_DIMENSION))) {
498532
zend_std_write_dimension(object, offset, inf);
499533
return;
@@ -504,6 +538,10 @@ static void spl_object_storage_write_dimension(zend_object *object, zval *offset
504538
static void spl_multiple_iterator_write_dimension(zend_object *object, zval *offset, zval *inf)
505539
{
506540
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
541+
if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
542+
return;
543+
}
544+
507545
if (UNEXPECTED(offset == NULL || Z_TYPE_P(offset) != IS_OBJECT || (intern->flags & SOS_OVERRIDDEN_WRITE_DIMENSION))) {
508546
zend_std_write_dimension(object, offset, inf);
509547
return;
@@ -518,6 +556,10 @@ static void spl_multiple_iterator_write_dimension(zend_object *object, zval *off
518556
static void spl_object_storage_unset_dimension(zend_object *object, zval *offset)
519557
{
520558
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
559+
if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
560+
return;
561+
}
562+
521563
if (UNEXPECTED(Z_TYPE_P(offset) != IS_OBJECT || (intern->flags & SOS_OVERRIDDEN_UNSET_DIMENSION))) {
522564
zend_std_unset_dimension(object, offset);
523565
return;
@@ -535,6 +577,9 @@ PHP_METHOD(SplObjectStorage, detach)
535577
Z_PARAM_OBJ(obj)
536578
ZEND_PARSE_PARAMETERS_END();
537579
spl_object_storage_detach(intern, obj);
580+
if (UNEXPECTED(EG(exception))) {
581+
RETURN_THROWS();
582+
}
538583

539584
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
540585
intern->index = 0;
@@ -566,6 +611,7 @@ PHP_METHOD(SplObjectStorage, offsetGet)
566611
ZEND_PARSE_PARAMETERS_END();
567612

568613
if (spl_object_storage_get_hash(&key, intern, obj) == FAILURE) {
614+
/* This may be the old NULL fallback, or an exception thrown by getHash(). */
569615
RETURN_NULL();
570616
}
571617

@@ -592,7 +638,9 @@ PHP_METHOD(SplObjectStorage, addAll)
592638

593639
other = Z_SPLOBJSTORAGE_P(obj);
594640

595-
spl_object_storage_addall(intern, other);
641+
if (UNEXPECTED(spl_object_storage_addall(intern, other) == FAILURE)) {
642+
RETURN_THROWS();
643+
}
596644

597645
RETURN_LONG(zend_hash_num_elements(&intern->storage));
598646
} /* }}} */
@@ -614,6 +662,9 @@ PHP_METHOD(SplObjectStorage, removeAll)
614662
zend_hash_internal_pointer_reset(&other->storage);
615663
while ((element = zend_hash_get_current_data_ptr(&other->storage)) != NULL) {
616664
if (spl_object_storage_detach(intern, element->obj) == FAILURE) {
665+
if (UNEXPECTED(EG(exception))) {
666+
RETURN_THROWS();
667+
}
617668
zend_hash_move_forward(&other->storage);
618669
}
619670
}
@@ -641,8 +692,19 @@ PHP_METHOD(SplObjectStorage, removeAllExcept)
641692
SPL_SAFE_HASH_FOREACH_PTR(&intern->storage, element) {
642693
zend_object *elem_obj = element->obj;
643694
GC_ADDREF(elem_obj);
644-
if (!spl_object_storage_contains(other, elem_obj)) {
645-
spl_object_storage_detach(intern, elem_obj);
695+
bool contains = spl_object_storage_contains(other, elem_obj);
696+
if (UNEXPECTED(EG(exception))) {
697+
OBJ_RELEASE(elem_obj);
698+
RETURN_THROWS();
699+
}
700+
if (!contains) {
701+
if (spl_object_storage_detach(intern, elem_obj) == FAILURE) {
702+
OBJ_RELEASE(elem_obj);
703+
if (UNEXPECTED(EG(exception))) {
704+
RETURN_THROWS();
705+
}
706+
continue;
707+
}
646708
}
647709
OBJ_RELEASE(elem_obj);
648710
} ZEND_HASH_FOREACH_END();
@@ -663,7 +725,13 @@ PHP_METHOD(SplObjectStorage, contains)
663725
ZEND_PARSE_PARAMETERS_START(1, 1)
664726
Z_PARAM_OBJ(obj)
665727
ZEND_PARSE_PARAMETERS_END();
666-
RETURN_BOOL(spl_object_storage_contains(intern, obj));
728+
729+
bool contains = spl_object_storage_contains(intern, obj);
730+
if (UNEXPECTED(EG(exception))) {
731+
RETURN_THROWS();
732+
}
733+
734+
RETURN_BOOL(contains);
667735
} /* }}} */
668736

669737
/* {{{ Determine number of objects in storage */
@@ -753,6 +821,9 @@ PHP_METHOD(SplObjectStorage, setInfo)
753821
if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &inf) == FAILURE) {
754822
RETURN_THROWS();
755823
}
824+
if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
825+
RETURN_THROWS();
826+
}
756827

757828
if ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) == NULL) {
758829
RETURN_NULL();
@@ -947,6 +1018,10 @@ PHP_METHOD(SplObjectStorage, unserialize)
9471018

9481019
if (spl_object_storage_get_hash(&key, intern, Z_OBJ_P(entry)) == FAILURE) {
9491020
zval_ptr_dtor(&inf);
1021+
if (EG(exception)) {
1022+
PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
1023+
RETURN_THROWS();
1024+
}
9501025
goto outexcept;
9511026
}
9521027
pelement = spl_object_storage_get(intern, &key);
@@ -960,6 +1035,14 @@ PHP_METHOD(SplObjectStorage, unserialize)
9601035
var_push_dtor(&var_hash, &obj);
9611036
}
9621037
element = spl_object_storage_attach(intern, Z_OBJ_P(entry), Z_ISUNDEF(inf)?NULL:&inf);
1038+
if (UNEXPECTED(!element)) {
1039+
zval_ptr_dtor(&inf);
1040+
if (EG(exception)) {
1041+
PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
1042+
RETURN_THROWS();
1043+
}
1044+
goto outexcept;
1045+
}
9631046
var_replace(&var_hash, &inf, &element->inf);
9641047
zval_ptr_dtor(&inf);
9651048
}
@@ -1055,7 +1138,9 @@ PHP_METHOD(SplObjectStorage, __unserialize)
10551138
}
10561139

10571140
ZVAL_DEREF(val);
1058-
spl_object_storage_attach(intern, Z_OBJ_P(key), val);
1141+
if (UNEXPECTED(!spl_object_storage_attach(intern, Z_OBJ_P(key), val))) {
1142+
RETURN_THROWS();
1143+
}
10591144
key = NULL;
10601145
} else {
10611146
key = val;
@@ -1151,8 +1236,14 @@ PHP_METHOD(MultipleIterator, attachIterator)
11511236
}
11521237

11531238
spl_object_storage_attach(intern, iterator, &zinfo);
1239+
if (UNEXPECTED(EG(exception))) {
1240+
RETURN_THROWS();
1241+
}
11541242
} else {
11551243
spl_object_storage_attach(intern, iterator, NULL);
1244+
if (UNEXPECTED(EG(exception))) {
1245+
RETURN_THROWS();
1246+
}
11561247
}
11571248
}
11581249
/* }}} */
@@ -1167,6 +1258,9 @@ PHP_METHOD(MultipleIterator, detachIterator)
11671258
RETURN_THROWS();
11681259
}
11691260
spl_object_storage_detach(intern, Z_OBJ_P(iterator));
1261+
if (UNEXPECTED(EG(exception))) {
1262+
RETURN_THROWS();
1263+
}
11701264

11711265
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
11721266
intern->index = 0;

‎ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt‎

Lines changed: 35 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,62 @@
11
--TEST--
2-
SplObjectStorage: Concurrent deletion during iteration
2+
SplObjectStorage: Mutation during getHash is prohibited
33
--CREDITS--
44
cnitlrt
55
--FILE--
66
<?php
77
class EvilStorage extends SplObjectStorage {
8+
public bool $mutate = false;
9+
810
public function getHash($obj): string {
911
global $victim;
10-
static $counter = 0;
11-
if ($counter++ < 1024*2) {
12-
// Re-entrant mutation during removeAllExcept iteration
13-
for ($i = 0; $i < 5; $i++) {
14-
$victim[new stdClass()] = null;
15-
}
12+
if ($this->mutate) {
13+
$victim[new stdClass()] = null;
1614
}
1715
return spl_object_hash($obj);
1816
}
1917
}
2018

19+
function populate(SplObjectStorage $victim, SplObjectStorage $other): void {
20+
for ($i = 0; $i < 1024; $i++) {
21+
$o = new stdClass();
22+
$victim[$o] = null;
23+
$other[$o] = null;
24+
}
25+
}
26+
2127
$victim = new SplObjectStorage();
2228
$other = new EvilStorage();
2329

24-
for ($i = 0; $i < 1024; $i++) {
25-
$o = new stdClass();
26-
$victim[$o] = null;
27-
$other[$o] = null;
30+
populate($victim, $other);
31+
$other->mutate = true;
32+
33+
try {
34+
$victim->removeAllExcept($other);
35+
} catch (Error $e) {
36+
echo $e->getMessage(), "\n";
2837
}
2938

30-
var_dump($victim->removeAllExcept($other));
39+
var_dump(count($victim), count($other));
3140

3241
unset($victim, $other);
3342
$victim = new SplObjectStorage();
3443
$other = new EvilStorage();
3544

36-
for ($i = 0; $i < 1024; $i++) {
37-
$o = new stdClass();
38-
$victim[$o] = null;
39-
$other[$o] = null;
45+
populate($victim, $other);
46+
$other->mutate = true;
47+
48+
try {
49+
$other->addAll($victim);
50+
} catch (Error $e) {
51+
echo $e->getMessage(), "\n";
4052
}
4153

42-
var_dump($other->addAll($victim));
54+
var_dump(count($victim), count($other));
4355
?>
44-
--EXPECTF--
45-
int(%d)
56+
--EXPECT--
57+
Modification of SplObjectStorage during getHash() is prohibited
58+
int(1024)
59+
int(1024)
60+
Modification of SplObjectStorage during getHash() is prohibited
61+
int(1024)
4662
int(1024)

0 commit comments

Comments
 (0)