Skip to content

Commit 0e500d1

Browse files
iliaaliliaal
committed
Fix GH-22480: phpdbg use-after-free re-watching a watched variable
phpdbg_add_watch_element() frees the passed element and returns the existing one when the variable is already watched, but several callers kept using the freed pointer, so re-watching a variable was a use-after-free in more places than the two reported. Report the duplicate back through the parse callback and register nothing, and reject a recursive watch's hashtable element that collides with an existing watch, at creation and recreation, rather than freeing a still-referenced one. A recursive watch over an already-array-watched variable now watches only the root and skips the shared hashtable. Running the tests under ASAN exposed a second, latent use-after-free at shutdown: the watch tables are module-scoped but hold request-pool memory, so under the tracked allocator freeing a watched variable during executor teardown re-queues an element after the last drain, tracked_free_all frees the pool, and phpdbg_destroy_watchpoints reads it at module shutdown. Tear the watch state down in a post_deactivate handler, which runs after the executor frees values but before the memory manager is torn down. Fixes GH-22480
1 parent 8b39b01 commit 0e500d1

7 files changed

Lines changed: 251 additions & 45 deletions

File tree

‎NEWS‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,10 @@ PHP NEWS
184184
. Mark Phar::buildFromIterator() base directory argument as a path.
185185
(ndossche)
186186

187+
- phpdbg:
188+
. Fixed GH-22480 (Use-after-free when re-watching an already-watched
189+
variable). (iliaal)
190+
187191
- Posix:
188192
. Added validity check to the flags argument for posix_access(). (arshidkv12)
189193

‎sapi/phpdbg/phpdbg.c‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,13 @@ static PHP_RSHUTDOWN_FUNCTION(phpdbg) /* {{{ */
239239
return SUCCESS;
240240
} /* }}} */
241241

242+
static ZEND_MODULE_POST_ZEND_DEACTIVATE_D(phpdbg) /* {{{ */
243+
{
244+
phpdbg_release_watch_elements();
245+
246+
return SUCCESS;
247+
} /* }}} */
248+
242249
/* {{{ Attempt to set the execution context for phpdbg
243250
If the execution context was set previously it is returned
244251
If the execution context was not set previously boolean true is returned
@@ -681,7 +688,9 @@ static zend_module_entry sapi_phpdbg_module_entry = {
681688
PHP_RSHUTDOWN(phpdbg),
682689
NULL,
683690
PHPDBG_VERSION,
684-
STANDARD_MODULE_PROPERTIES
691+
NO_MODULE_GLOBALS,
692+
ZEND_MODULE_POST_ZEND_DEACTIVATE_N(phpdbg),
693+
STANDARD_MODULE_PROPERTIES_EX
685694
};
686695

687696
static inline int php_sapi_phpdbg_module_startup(sapi_module_struct *module) /* {{{ */

‎sapi/phpdbg/phpdbg_watch.c‎

Lines changed: 172 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -501,8 +501,39 @@ void phpdbg_free_watch_element(phpdbg_watch_element *element);
501501
void phpdbg_remove_watchpoint(phpdbg_watchpoint_t *watch);
502502
void phpdbg_watch_parent_ht(phpdbg_watch_element *element);
503503

504-
phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdbg_watch_element *element) {
504+
static bool phpdbg_watch_element_collides(void *addr, phpdbg_watchtype type, phpdbg_watch_element *element) {
505+
phpdbg_watch_element *old;
506+
phpdbg_btree_result *res = phpdbg_btree_find(&PHPDBG_G(watchpoint_tree), (zend_ulong) addr);
507+
508+
if (res) {
509+
phpdbg_watchpoint_t *watch = res->ptr;
510+
if (watch->type == type && (old = zend_hash_find_ptr(&watch->elements, element->str)) && old != element) {
511+
return true;
512+
}
513+
}
514+
515+
return (old = zend_hash_find_ptr(&PHPDBG_G(watch_recreation), element->str)) && old != element;
516+
}
517+
518+
static bool phpdbg_ht_watch_element_collides(zval *zv, phpdbg_watch_element *element) {
519+
HashTable *ht = HT_FROM_ZVP(zv);
520+
521+
if (!ht) {
522+
return false;
523+
}
524+
525+
return phpdbg_watch_element_collides(HT_WATCH_OFFSET + (char *) ht, WATCH_ON_HASHTABLE, element);
526+
}
527+
528+
static bool phpdbg_bucket_watch_element_collides(zval *zv, phpdbg_watch_element *element) {
529+
return phpdbg_watch_element_collides((Bucket *) zv, WATCH_ON_BUCKET, element);
530+
}
531+
532+
phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdbg_watch_element *element, bool *is_new) {
505533
phpdbg_btree_result *res;
534+
if (is_new) {
535+
*is_new = true;
536+
}
506537
if ((res = phpdbg_btree_find(&PHPDBG_G(watchpoint_tree), (zend_ulong) watch->addr.ptr)) == NULL) {
507538
phpdbg_watchpoint_t *mem = emalloc(sizeof(*mem));
508539
*mem = *watch;
@@ -520,6 +551,9 @@ phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdb
520551
if (element != old_element) {
521552
phpdbg_free_watch_element(element);
522553
}
554+
if (is_new) {
555+
*is_new = false;
556+
}
523557
return old_element;
524558
}
525559
}
@@ -534,25 +568,35 @@ phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdb
534568
return element;
535569
}
536570

537-
phpdbg_watch_element *phpdbg_add_bucket_watch_element(Bucket *bucket, phpdbg_watch_element *element) {
571+
phpdbg_watch_element *phpdbg_add_bucket_watch_element(Bucket *bucket, phpdbg_watch_element *element, bool *is_new) {
538572
phpdbg_watchpoint_t watch;
539573
phpdbg_set_bucket_watchpoint(bucket, &watch);
540-
element = phpdbg_add_watch_element(&watch, element);
541-
phpdbg_watch_parent_ht(element);
542-
return element;
574+
bool added_new;
575+
phpdbg_watch_element *added = phpdbg_add_watch_element(&watch, element, &added_new);
576+
if (added_new) {
577+
phpdbg_watch_parent_ht(added);
578+
}
579+
if (is_new) {
580+
*is_new = added_new;
581+
}
582+
return added;
543583
}
544584

545-
phpdbg_watch_element *phpdbg_add_ht_watch_element(zval *zv, phpdbg_watch_element *element) {
585+
phpdbg_watch_element *phpdbg_add_ht_watch_element(zval *zv, phpdbg_watch_element *element, bool *is_new) {
546586
phpdbg_watchpoint_t watch;
547587
HashTable *ht = HT_FROM_ZVP(zv);
548588

589+
if (is_new) {
590+
*is_new = false;
591+
}
592+
549593
if (!ht) {
550594
return NULL;
551595
}
552596

553597
element->flags |= Z_TYPE_P(zv) == IS_ARRAY ? PHPDBG_WATCH_ARRAY : PHPDBG_WATCH_OBJECT;
554598
phpdbg_set_ht_watchpoint(ht, &watch);
555-
return phpdbg_add_watch_element(&watch, element);
599+
return phpdbg_add_watch_element(&watch, element, is_new);
556600
}
557601

558602
bool phpdbg_is_recursively_watched(void *ptr, phpdbg_watch_element *element) {
@@ -588,8 +632,15 @@ void phpdbg_add_recursive_watch_from_ht(phpdbg_watch_element *element, zend_long
588632
child->parent = element;
589633
child->child = NULL;
590634
child->parent_container = HT_WATCH_HT(element->watch);
591-
zend_hash_add_ptr(&element->child_container, child->str, child);
592-
phpdbg_add_bucket_watch_element((Bucket *) zv, child);
635+
if (phpdbg_bucket_watch_element_collides(zv, child)) {
636+
phpdbg_free_watch_element(child);
637+
return;
638+
}
639+
bool added_new;
640+
phpdbg_add_bucket_watch_element((Bucket *) zv, child, &added_new);
641+
if (added_new) {
642+
zend_hash_add_ptr(&element->child_container, child->str, child);
643+
}
593644
}
594645

595646
void phpdbg_recurse_watch_element(phpdbg_watch_element *element) {
@@ -627,8 +678,13 @@ void phpdbg_recurse_watch_element(phpdbg_watch_element *element) {
627678
child->child = NULL;
628679
element->child = child;
629680
}
681+
if (phpdbg_ht_watch_element_collides(zv, child)) {
682+
phpdbg_free_watch_element(child);
683+
element->child = NULL;
684+
return;
685+
}
630686
zend_hash_init(&child->child_container, 8, NULL, NULL, 0);
631-
phpdbg_add_ht_watch_element(zv, child);
687+
phpdbg_add_ht_watch_element(zv, child, NULL);
632688
} else if (zend_hash_num_elements(&element->child_container) == 0) {
633689
zend_string *str;
634690
zend_long idx;
@@ -727,7 +783,10 @@ bool phpdbg_try_re_adding_watch_element(zval *parent, phpdbg_watch_element *elem
727783
phpdbg_print_watch_diff(WATCH_ON_HASHTABLE, element->str, oldPtr, htPtr);
728784
}
729785

730-
phpdbg_add_ht_watch_element(parent, element);
786+
if ((element->flags & PHPDBG_WATCH_RECURSIVE) && phpdbg_ht_watch_element_collides(parent, element)) {
787+
return false;
788+
}
789+
element = phpdbg_add_ht_watch_element(parent, element, NULL);
731790
} else if ((zv = zend_symtable_find(ht, element->name_in_parent))) {
732791
if (element->flags & PHPDBG_WATCH_IMPLICIT) {
733792
zval *next = zv;
@@ -746,9 +805,11 @@ bool phpdbg_try_re_adding_watch_element(zval *parent, phpdbg_watch_element *elem
746805
phpdbg_print_watch_diff(WATCH_ON_ZVAL, element->str, &element->backup.zv, zv);
747806
}
748807

808+
if ((element->flags & PHPDBG_WATCH_RECURSIVE) && phpdbg_bucket_watch_element_collides(zv, element)) {
809+
return false;
810+
}
749811
element->parent_container = ht;
750-
phpdbg_add_bucket_watch_element((Bucket *) zv, element);
751-
phpdbg_watch_parent_ht(element);
812+
element = phpdbg_add_bucket_watch_element((Bucket *) zv, element, NULL);
752813
} else {
753814
return false;
754815
}
@@ -1248,63 +1309,106 @@ void phpdbg_list_watchpoints(void) {
12481309
} ZEND_HASH_FOREACH_END();
12491310
}
12501311

1251-
static int phpdbg_create_simple_watchpoint(zval *zv, phpdbg_watch_element *element) {
1252-
element->flags = PHPDBG_WATCH_SIMPLE;
1253-
phpdbg_add_bucket_watch_element((Bucket *) zv, element);
1254-
return SUCCESS;
1312+
typedef enum {
1313+
PHPDBG_WATCH_ADD_OK,
1314+
PHPDBG_WATCH_ADD_FAILED,
1315+
PHPDBG_WATCH_ADD_DUPLICATE,
1316+
} phpdbg_watch_add_result;
1317+
1318+
static phpdbg_watch_add_result phpdbg_create_simple_watchpoint(zval *zv, phpdbg_watch_element **element) {
1319+
phpdbg_watch_element *added;
1320+
(*element)->flags = PHPDBG_WATCH_SIMPLE;
1321+
bool added_new;
1322+
added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new);
1323+
if (!added_new) {
1324+
*element = added;
1325+
return PHPDBG_WATCH_ADD_DUPLICATE;
1326+
}
1327+
return PHPDBG_WATCH_ADD_OK;
12551328
}
12561329

1257-
static int phpdbg_create_array_watchpoint(zval *zv, phpdbg_watch_element *element) {
1258-
phpdbg_watch_element *new;
1330+
static phpdbg_watch_add_result phpdbg_create_array_watchpoint(zval *zv, phpdbg_watch_element **element_ptr) {
1331+
phpdbg_watch_element *element = *element_ptr;
1332+
phpdbg_watch_element *new, *added;
12591333
zend_string *str;
12601334
zval *orig_zv = zv;
12611335

12621336
ZVAL_DEREF(zv);
12631337
if (Z_TYPE_P(zv) != IS_ARRAY && Z_TYPE_P(zv) != IS_OBJECT) {
1264-
return FAILURE;
1338+
return PHPDBG_WATCH_ADD_FAILED;
12651339
}
12661340

1267-
new = ecalloc(1, sizeof(phpdbg_watch_element));
1268-
12691341
str = strpprintf(0, "%.*s[]", (int) ZSTR_LEN(element->str), ZSTR_VAL(element->str));
12701342
zend_string_release(element->str);
12711343
element->str = str;
12721344
element->flags = PHPDBG_WATCH_IMPLICIT;
1273-
phpdbg_add_bucket_watch_element((Bucket *) orig_zv, element);
1274-
element->child = new;
1345+
bool added_new;
1346+
added = phpdbg_add_bucket_watch_element((Bucket *) orig_zv, element, &added_new);
1347+
if (!added_new) {
1348+
*element_ptr = added;
1349+
return PHPDBG_WATCH_ADD_DUPLICATE;
1350+
}
1351+
element = added;
12751352

1353+
new = ecalloc(1, sizeof(phpdbg_watch_element));
12761354
new->flags = PHPDBG_WATCH_SIMPLE;
12771355
new->str = zend_string_copy(str);
12781356
new->parent = element;
1279-
phpdbg_add_ht_watch_element(zv, new);
1280-
return SUCCESS;
1357+
added = phpdbg_add_ht_watch_element(zv, new, &added_new);
1358+
if (added != NULL && !added_new) {
1359+
phpdbg_clean_watch_element(element);
1360+
phpdbg_free_watch_element(element);
1361+
*element_ptr = added;
1362+
return PHPDBG_WATCH_ADD_DUPLICATE;
1363+
}
1364+
element->child = new;
1365+
*element_ptr = element;
1366+
return PHPDBG_WATCH_ADD_OK;
12811367
}
12821368

1283-
static int phpdbg_create_recursive_watchpoint(zval *zv, phpdbg_watch_element *element) {
1284-
element->flags = PHPDBG_WATCH_RECURSIVE | PHPDBG_WATCH_RECURSIVE_ROOT;
1285-
element->child = NULL;
1286-
phpdbg_add_bucket_watch_element((Bucket *) zv, element);
1287-
return SUCCESS;
1369+
static phpdbg_watch_add_result phpdbg_create_recursive_watchpoint(zval *zv, phpdbg_watch_element **element) {
1370+
phpdbg_watch_element *added;
1371+
(*element)->flags = PHPDBG_WATCH_RECURSIVE | PHPDBG_WATCH_RECURSIVE_ROOT;
1372+
(*element)->child = NULL;
1373+
bool added_new;
1374+
added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new);
1375+
if (!added_new) {
1376+
*element = added;
1377+
return PHPDBG_WATCH_ADD_DUPLICATE;
1378+
}
1379+
return PHPDBG_WATCH_ADD_OK;
12881380
}
12891381

1290-
typedef struct { int (*callback)(zval *zv, phpdbg_watch_element *); zend_string *str; } phpdbg_watch_parse_struct;
1382+
typedef struct { phpdbg_watch_add_result (*callback)(zval *zv, phpdbg_watch_element **); zend_string *str; } phpdbg_watch_parse_struct;
12911383

12921384
static int phpdbg_watchpoint_parse_wrapper(char *name, size_t namelen, char *key, size_t keylen, HashTable *parent, zval *zv, phpdbg_watch_parse_struct *info) {
1293-
int ret;
1385+
int ret = SUCCESS;
12941386
phpdbg_watch_element *element = ecalloc(1, sizeof(phpdbg_watch_element));
12951387
element->str = zend_string_init(name, namelen, 0);
12961388
element->name_in_parent = zend_string_init(key, keylen, 0);
12971389
element->parent_container = parent;
12981390
element->parent = PHPDBG_G(watch_tmp);
12991391
element->child = NULL;
13001392

1301-
ret = info->callback(zv, element);
1393+
phpdbg_watch_add_result result = info->callback(zv, &element);
13021394

13031395
efree(name);
13041396
efree(key);
13051397

1306-
if (ret != SUCCESS) {
1398+
if (result == PHPDBG_WATCH_ADD_FAILED) {
13071399
phpdbg_remove_watch_element(element);
1400+
ret = FAILURE;
1401+
} else if (result == PHPDBG_WATCH_ADD_DUPLICATE) {
1402+
phpdbg_notice("%.*s is already being watched", (int) ZSTR_LEN(element->str), ZSTR_VAL(element->str));
1403+
while (PHPDBG_G(watch_tmp) && PHPDBG_G(watch_tmp)->child == NULL) {
1404+
phpdbg_watch_element *parent = PHPDBG_G(watch_tmp)->parent;
1405+
phpdbg_clean_watch_element(PHPDBG_G(watch_tmp));
1406+
phpdbg_free_watch_element(PHPDBG_G(watch_tmp));
1407+
if (parent) {
1408+
parent->child = NULL;
1409+
}
1410+
PHPDBG_G(watch_tmp) = parent;
1411+
}
13081412
} else {
13091413
if (PHPDBG_G(watch_tmp)) {
13101414
PHPDBG_G(watch_tmp)->child = element;
@@ -1346,7 +1450,7 @@ static int phpdbg_watchpoint_parse_step(char *name, size_t namelen, char *key, s
13461450
element->name_in_parent = zend_string_init(key, keylen, 0);
13471451
element->parent_container = parent;
13481452
element->parent = PHPDBG_G(watch_tmp);
1349-
element = phpdbg_add_bucket_watch_element((Bucket *) zv, element);
1453+
element = phpdbg_add_bucket_watch_element((Bucket *) zv, element, NULL);
13501454

13511455
efree(name);
13521456
efree(key);
@@ -1359,7 +1463,7 @@ static int phpdbg_watchpoint_parse_step(char *name, size_t namelen, char *key, s
13591463
return SUCCESS;
13601464
}
13611465

1362-
static int phpdbg_watchpoint_parse_symtables(char *input, size_t len, int (*callback)(zval *, phpdbg_watch_element *)) {
1466+
static int phpdbg_watchpoint_parse_symtables(char *input, size_t len, phpdbg_watch_add_result (*callback)(zval *, phpdbg_watch_element **)) {
13631467
zend_class_entry *scope = zend_get_executed_scope();
13641468
phpdbg_watch_parse_struct info;
13651469
int ret;
@@ -1529,6 +1633,37 @@ void phpdbg_destroy_watchpoints(void) {
15291633
free(PHPDBG_G(watchlist_mem_backup));
15301634
}
15311635

1636+
void phpdbg_release_watch_elements(void) {
1637+
phpdbg_watch_element *element;
1638+
uint32_t guard;
1639+
1640+
ZEND_HASH_MAP_FOREACH_PTR(&PHPDBG_G(watch_recreation), element) {
1641+
phpdbg_automatic_dequeue_free(element);
1642+
} ZEND_HASH_FOREACH_END();
1643+
zend_hash_clean(&PHPDBG_G(watch_recreation));
1644+
1645+
guard = zend_hash_num_elements(&PHPDBG_G(watch_elements)) + 1;
1646+
while (zend_hash_num_elements(&PHPDBG_G(watch_elements)) > 0 && guard-- > 0) {
1647+
ZEND_HASH_FOREACH_PTR(&PHPDBG_G(watch_elements), element) {
1648+
phpdbg_remove_watch_element(element);
1649+
break;
1650+
} ZEND_HASH_FOREACH_END();
1651+
}
1652+
zend_hash_clean(&PHPDBG_G(watch_recreation));
1653+
1654+
phpdbg_btree_clean(&PHPDBG_G(watchpoint_tree));
1655+
phpdbg_btree_clean(&PHPDBG_G(watch_HashTables));
1656+
1657+
zend_hash_destroy(&PHPDBG_G(watch_elements));
1658+
zend_hash_init(&PHPDBG_G(watch_elements), 8, NULL, NULL, 0);
1659+
zend_hash_destroy(&PHPDBG_G(watch_recreation));
1660+
zend_hash_init(&PHPDBG_G(watch_recreation), 8, NULL, NULL, 0);
1661+
zend_hash_destroy(&PHPDBG_G(watch_free));
1662+
zend_hash_init(&PHPDBG_G(watch_free), 8, NULL, NULL, 0);
1663+
zend_hash_destroy(&PHPDBG_G(watch_collisions));
1664+
zend_hash_init(&PHPDBG_G(watch_collisions), 8, NULL, NULL, 0);
1665+
}
1666+
15321667
void phpdbg_purge_watchpoint_tree(void) {
15331668
phpdbg_btree_position pos;
15341669
phpdbg_btree_result *res;

‎sapi/phpdbg/phpdbg_watch.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,7 @@ typedef struct {
110110

111111
void phpdbg_setup_watchpoints(void);
112112
void phpdbg_destroy_watchpoints(void);
113+
void phpdbg_release_watch_elements(void);
113114
void phpdbg_purge_watchpoint_tree(void);
114115

115116
#ifndef _WIN32

0 commit comments

Comments
 (0)