From 4a7f69f24bc9aad702fbc22d1c079efd0ee6e6d9 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Tue, 14 Jul 2026 14:23:34 -0400 Subject: [PATCH 1/3] Fix GH-22480: phpdbg use-after-free re-watching a watched variable (#22493) 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. Dropping watch_006's xfail surfaced a separate pre-existing bug on large-page platforms: the fault handler rechecks every watchpoint on the faulting page, so on 16 KB pages a value-only write to an element can fault the page holding a watched hashtable's own struct and run the recursive rescan, which re-reported existing elements as freshly added. Only run that rescan when the element count actually changed. Fixes GH-22480 (cherry picked from commit 207dd1d23f75654f0350d55b246cee40e451645a) --- NEWS | 2 + sapi/phpdbg/phpdbg.c | 11 +- sapi/phpdbg/phpdbg_watch.c | 212 +++++++++++++++++++++++++------ sapi/phpdbg/phpdbg_watch.h | 1 + sapi/phpdbg/tests/gh22480.phpt | 34 +++++ sapi/phpdbg/tests/gh22480_2.phpt | 30 +++++ sapi/phpdbg/tests/watch_006.phpt | 9 -- 7 files changed, 251 insertions(+), 48 deletions(-) create mode 100644 sapi/phpdbg/tests/gh22480.phpt create mode 100644 sapi/phpdbg/tests/gh22480_2.phpt diff --git a/NEWS b/NEWS index fb54e6dd3e6c..5c9720923443 100644 --- a/NEWS +++ b/NEWS @@ -121,6 +121,8 @@ PHP NEWS - Phpdbg: . Fix wrong command names. (ndossche, Weilin Du) + . Fixed bug GH-22480 (Use-after-free when re-watching an already-watched + variable). (Ilia Alshanetsky) - Readline: . Fixed a crash in readline_info() when completion_append_character diff --git a/sapi/phpdbg/phpdbg.c b/sapi/phpdbg/phpdbg.c index 69f14913b4b6..27a716481f9b 100644 --- a/sapi/phpdbg/phpdbg.c +++ b/sapi/phpdbg/phpdbg.c @@ -241,6 +241,13 @@ static PHP_RSHUTDOWN_FUNCTION(phpdbg) /* {{{ */ return SUCCESS; } /* }}} */ +static ZEND_MODULE_POST_ZEND_DEACTIVATE_D(phpdbg) /* {{{ */ +{ + phpdbg_release_watch_elements(); + + return SUCCESS; +} /* }}} */ + /* {{{ Attempt to set the execution context for phpdbg If the execution context was set previously it is returned If the execution context was not set previously boolean true is returned @@ -688,7 +695,9 @@ static zend_module_entry sapi_phpdbg_module_entry = { PHP_RSHUTDOWN(phpdbg), NULL, PHPDBG_VERSION, - STANDARD_MODULE_PROPERTIES + NO_MODULE_GLOBALS, + ZEND_MODULE_POST_ZEND_DEACTIVATE_N(phpdbg), + STANDARD_MODULE_PROPERTIES_EX }; static inline int php_sapi_phpdbg_module_startup(sapi_module_struct *module) /* {{{ */ diff --git a/sapi/phpdbg/phpdbg_watch.c b/sapi/phpdbg/phpdbg_watch.c index 2f546a71d606..848ef6b2cfca 100644 --- a/sapi/phpdbg/phpdbg_watch.c +++ b/sapi/phpdbg/phpdbg_watch.c @@ -503,8 +503,39 @@ void phpdbg_free_watch_element(phpdbg_watch_element *element); void phpdbg_remove_watchpoint(phpdbg_watchpoint_t *watch); void phpdbg_watch_parent_ht(phpdbg_watch_element *element); -phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdbg_watch_element *element) { +static bool phpdbg_watch_element_collides(void *addr, phpdbg_watchtype type, phpdbg_watch_element *element) { + phpdbg_watch_element *old; + phpdbg_btree_result *res = phpdbg_btree_find(&PHPDBG_G(watchpoint_tree), (zend_ulong) addr); + + if (res) { + phpdbg_watchpoint_t *watch = res->ptr; + if (watch->type == type && (old = zend_hash_find_ptr(&watch->elements, element->str)) && old != element) { + return true; + } + } + + return (old = zend_hash_find_ptr(&PHPDBG_G(watch_recreation), element->str)) && old != element; +} + +static bool phpdbg_ht_watch_element_collides(zval *zv, phpdbg_watch_element *element) { + HashTable *ht = HT_FROM_ZVP(zv); + + if (!ht) { + return false; + } + + return phpdbg_watch_element_collides(HT_WATCH_OFFSET + (char *) ht, WATCH_ON_HASHTABLE, element); +} + +static bool phpdbg_bucket_watch_element_collides(zval *zv, phpdbg_watch_element *element) { + return phpdbg_watch_element_collides((Bucket *) zv, WATCH_ON_BUCKET, element); +} + +phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdbg_watch_element *element, bool *is_new) { phpdbg_btree_result *res; + if (is_new) { + *is_new = true; + } if ((res = phpdbg_btree_find(&PHPDBG_G(watchpoint_tree), (zend_ulong) watch->addr.ptr)) == NULL) { phpdbg_watchpoint_t *mem = emalloc(sizeof(*mem)); *mem = *watch; @@ -522,6 +553,9 @@ phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdb if (element != old_element) { phpdbg_free_watch_element(element); } + if (is_new) { + *is_new = false; + } return old_element; } } @@ -536,25 +570,35 @@ phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdb return element; } -phpdbg_watch_element *phpdbg_add_bucket_watch_element(Bucket *bucket, phpdbg_watch_element *element) { +phpdbg_watch_element *phpdbg_add_bucket_watch_element(Bucket *bucket, phpdbg_watch_element *element, bool *is_new) { phpdbg_watchpoint_t watch; phpdbg_set_bucket_watchpoint(bucket, &watch); - element = phpdbg_add_watch_element(&watch, element); - phpdbg_watch_parent_ht(element); - return element; + bool added_new; + phpdbg_watch_element *added = phpdbg_add_watch_element(&watch, element, &added_new); + if (added_new) { + phpdbg_watch_parent_ht(added); + } + if (is_new) { + *is_new = added_new; + } + return added; } -phpdbg_watch_element *phpdbg_add_ht_watch_element(zval *zv, phpdbg_watch_element *element) { +phpdbg_watch_element *phpdbg_add_ht_watch_element(zval *zv, phpdbg_watch_element *element, bool *is_new) { phpdbg_watchpoint_t watch; HashTable *ht = HT_FROM_ZVP(zv); + if (is_new) { + *is_new = false; + } + if (!ht) { return NULL; } element->flags |= Z_TYPE_P(zv) == IS_ARRAY ? PHPDBG_WATCH_ARRAY : PHPDBG_WATCH_OBJECT; phpdbg_set_ht_watchpoint(ht, &watch); - return phpdbg_add_watch_element(&watch, element); + return phpdbg_add_watch_element(&watch, element, is_new); } bool phpdbg_is_recursively_watched(void *ptr, phpdbg_watch_element *element) { @@ -590,8 +634,15 @@ void phpdbg_add_recursive_watch_from_ht(phpdbg_watch_element *element, zend_long child->parent = element; child->child = NULL; child->parent_container = HT_WATCH_HT(element->watch); - zend_hash_add_ptr(&element->child_container, child->str, child); - phpdbg_add_bucket_watch_element((Bucket *) zv, child); + if (phpdbg_bucket_watch_element_collides(zv, child)) { + phpdbg_free_watch_element(child); + return; + } + bool added_new; + phpdbg_add_bucket_watch_element((Bucket *) zv, child, &added_new); + if (added_new) { + zend_hash_add_ptr(&element->child_container, child->str, child); + } } void phpdbg_recurse_watch_element(phpdbg_watch_element *element) { @@ -629,8 +680,13 @@ void phpdbg_recurse_watch_element(phpdbg_watch_element *element) { child->child = NULL; element->child = child; } + if (phpdbg_ht_watch_element_collides(zv, child)) { + phpdbg_free_watch_element(child); + element->child = NULL; + return; + } zend_hash_init(&child->child_container, 8, NULL, NULL, 0); - phpdbg_add_ht_watch_element(zv, child); + phpdbg_add_ht_watch_element(zv, child, NULL); } else if (zend_hash_num_elements(&element->child_container) == 0) { zend_string *str; zend_long idx; @@ -729,7 +785,10 @@ bool phpdbg_try_re_adding_watch_element(zval *parent, phpdbg_watch_element *elem phpdbg_print_watch_diff(WATCH_ON_HASHTABLE, element->str, oldPtr, htPtr); } - phpdbg_add_ht_watch_element(parent, element); + if ((element->flags & PHPDBG_WATCH_RECURSIVE) && phpdbg_ht_watch_element_collides(parent, element)) { + return false; + } + element = phpdbg_add_ht_watch_element(parent, element, NULL); } else if ((zv = zend_symtable_find(ht, element->name_in_parent))) { if (element->flags & PHPDBG_WATCH_IMPLICIT) { zval *next = zv; @@ -748,9 +807,11 @@ bool phpdbg_try_re_adding_watch_element(zval *parent, phpdbg_watch_element *elem phpdbg_print_watch_diff(WATCH_ON_ZVAL, element->str, &element->backup.zv, zv); } + if ((element->flags & PHPDBG_WATCH_RECURSIVE) && phpdbg_bucket_watch_element_collides(zv, element)) { + return false; + } element->parent_container = ht; - phpdbg_add_bucket_watch_element((Bucket *) zv, element); - phpdbg_watch_parent_ht(element); + element = phpdbg_add_bucket_watch_element((Bucket *) zv, element, NULL); } else { return 0; } @@ -1012,7 +1073,8 @@ void phpdbg_check_watchpoint(phpdbg_watchpoint_t *watch) { zend_string *name = NULL; void *comparePtr; - if (watch->type == WATCH_ON_HASHTABLE) { + if (watch->type == WATCH_ON_HASHTABLE + && phpdbg_check_watch_diff(WATCH_ON_HASHTABLE, (char *) &watch->backup.ht + HT_WATCH_OFFSET, watch->addr.ptr)) { phpdbg_watch_element *element; zend_string *str; zend_long idx; @@ -1250,49 +1312,80 @@ void phpdbg_list_watchpoints(void) { } ZEND_HASH_FOREACH_END(); } -static int phpdbg_create_simple_watchpoint(zval *zv, phpdbg_watch_element *element) { - element->flags = PHPDBG_WATCH_SIMPLE; - phpdbg_add_bucket_watch_element((Bucket *) zv, element); - return SUCCESS; +typedef enum { + PHPDBG_WATCH_ADD_OK, + PHPDBG_WATCH_ADD_FAILED, + PHPDBG_WATCH_ADD_DUPLICATE, +} phpdbg_watch_add_result; + +static phpdbg_watch_add_result phpdbg_create_simple_watchpoint(zval *zv, phpdbg_watch_element **element) { + phpdbg_watch_element *added; + (*element)->flags = PHPDBG_WATCH_SIMPLE; + bool added_new; + added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new); + if (!added_new) { + *element = added; + return PHPDBG_WATCH_ADD_DUPLICATE; + } + return PHPDBG_WATCH_ADD_OK; } -static int phpdbg_create_array_watchpoint(zval *zv, phpdbg_watch_element *element) { - phpdbg_watch_element *new; +static phpdbg_watch_add_result phpdbg_create_array_watchpoint(zval *zv, phpdbg_watch_element **element_ptr) { + phpdbg_watch_element *element = *element_ptr; + phpdbg_watch_element *new, *added; zend_string *str; zval *orig_zv = zv; ZVAL_DEREF(zv); if (Z_TYPE_P(zv) != IS_ARRAY && Z_TYPE_P(zv) != IS_OBJECT) { - return FAILURE; + return PHPDBG_WATCH_ADD_FAILED; } - new = ecalloc(1, sizeof(phpdbg_watch_element)); - str = strpprintf(0, "%.*s[]", (int) ZSTR_LEN(element->str), ZSTR_VAL(element->str)); zend_string_release(element->str); element->str = str; element->flags = PHPDBG_WATCH_IMPLICIT; - phpdbg_add_bucket_watch_element((Bucket *) orig_zv, element); - element->child = new; + bool added_new; + added = phpdbg_add_bucket_watch_element((Bucket *) orig_zv, element, &added_new); + if (!added_new) { + *element_ptr = added; + return PHPDBG_WATCH_ADD_DUPLICATE; + } + element = added; + new = ecalloc(1, sizeof(phpdbg_watch_element)); new->flags = PHPDBG_WATCH_SIMPLE; new->str = zend_string_copy(str); new->parent = element; - phpdbg_add_ht_watch_element(zv, new); - return SUCCESS; + added = phpdbg_add_ht_watch_element(zv, new, &added_new); + if (added != NULL && !added_new) { + phpdbg_clean_watch_element(element); + phpdbg_free_watch_element(element); + *element_ptr = added; + return PHPDBG_WATCH_ADD_DUPLICATE; + } + element->child = new; + *element_ptr = element; + return PHPDBG_WATCH_ADD_OK; } -static int phpdbg_create_recursive_watchpoint(zval *zv, phpdbg_watch_element *element) { - element->flags = PHPDBG_WATCH_RECURSIVE | PHPDBG_WATCH_RECURSIVE_ROOT; - element->child = NULL; - phpdbg_add_bucket_watch_element((Bucket *) zv, element); - return SUCCESS; +static phpdbg_watch_add_result phpdbg_create_recursive_watchpoint(zval *zv, phpdbg_watch_element **element) { + phpdbg_watch_element *added; + (*element)->flags = PHPDBG_WATCH_RECURSIVE | PHPDBG_WATCH_RECURSIVE_ROOT; + (*element)->child = NULL; + bool added_new; + added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new); + if (!added_new) { + *element = added; + return PHPDBG_WATCH_ADD_DUPLICATE; + } + return PHPDBG_WATCH_ADD_OK; } -typedef struct { int (*callback)(zval *zv, phpdbg_watch_element *); zend_string *str; } phpdbg_watch_parse_struct; +typedef struct { phpdbg_watch_add_result (*callback)(zval *zv, phpdbg_watch_element **); zend_string *str; } phpdbg_watch_parse_struct; 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) { - int ret; + int ret = SUCCESS; phpdbg_watch_element *element = ecalloc(1, sizeof(phpdbg_watch_element)); element->str = zend_string_init(name, namelen, 0); element->name_in_parent = zend_string_init(key, keylen, 0); @@ -1300,13 +1393,25 @@ static int phpdbg_watchpoint_parse_wrapper(char *name, size_t namelen, char *key element->parent = PHPDBG_G(watch_tmp); element->child = NULL; - ret = info->callback(zv, element); + phpdbg_watch_add_result result = info->callback(zv, &element); efree(name); efree(key); - if (ret != SUCCESS) { + if (result == PHPDBG_WATCH_ADD_FAILED) { phpdbg_remove_watch_element(element); + ret = FAILURE; + } else if (result == PHPDBG_WATCH_ADD_DUPLICATE) { + phpdbg_notice("%.*s is already being watched", (int) ZSTR_LEN(element->str), ZSTR_VAL(element->str)); + while (PHPDBG_G(watch_tmp) && PHPDBG_G(watch_tmp)->child == NULL) { + phpdbg_watch_element *parent = PHPDBG_G(watch_tmp)->parent; + phpdbg_clean_watch_element(PHPDBG_G(watch_tmp)); + phpdbg_free_watch_element(PHPDBG_G(watch_tmp)); + if (parent) { + parent->child = NULL; + } + PHPDBG_G(watch_tmp) = parent; + } } else { if (PHPDBG_G(watch_tmp)) { PHPDBG_G(watch_tmp)->child = element; @@ -1348,7 +1453,7 @@ static int phpdbg_watchpoint_parse_step(char *name, size_t namelen, char *key, s element->name_in_parent = zend_string_init(key, keylen, 0); element->parent_container = parent; element->parent = PHPDBG_G(watch_tmp); - element = phpdbg_add_bucket_watch_element((Bucket *) zv, element); + element = phpdbg_add_bucket_watch_element((Bucket *) zv, element, NULL); efree(name); efree(key); @@ -1361,7 +1466,7 @@ static int phpdbg_watchpoint_parse_step(char *name, size_t namelen, char *key, s return SUCCESS; } -static int phpdbg_watchpoint_parse_symtables(char *input, size_t len, int (*callback)(zval *, phpdbg_watch_element *)) { +static int phpdbg_watchpoint_parse_symtables(char *input, size_t len, phpdbg_watch_add_result (*callback)(zval *, phpdbg_watch_element **)) { zend_class_entry *scope = zend_get_executed_scope(); phpdbg_watch_parse_struct info; int ret; @@ -1531,6 +1636,37 @@ void phpdbg_destroy_watchpoints(void) { free(PHPDBG_G(watchlist_mem_backup)); } +void phpdbg_release_watch_elements(void) { + phpdbg_watch_element *element; + uint32_t guard; + + ZEND_HASH_MAP_FOREACH_PTR(&PHPDBG_G(watch_recreation), element) { + phpdbg_automatic_dequeue_free(element); + } ZEND_HASH_FOREACH_END(); + zend_hash_clean(&PHPDBG_G(watch_recreation)); + + guard = zend_hash_num_elements(&PHPDBG_G(watch_elements)) + 1; + while (zend_hash_num_elements(&PHPDBG_G(watch_elements)) > 0 && guard-- > 0) { + ZEND_HASH_FOREACH_PTR(&PHPDBG_G(watch_elements), element) { + phpdbg_remove_watch_element(element); + break; + } ZEND_HASH_FOREACH_END(); + } + zend_hash_clean(&PHPDBG_G(watch_recreation)); + + phpdbg_btree_clean(&PHPDBG_G(watchpoint_tree)); + phpdbg_btree_clean(&PHPDBG_G(watch_HashTables)); + + zend_hash_destroy(&PHPDBG_G(watch_elements)); + zend_hash_init(&PHPDBG_G(watch_elements), 8, NULL, NULL, 0); + zend_hash_destroy(&PHPDBG_G(watch_recreation)); + zend_hash_init(&PHPDBG_G(watch_recreation), 8, NULL, NULL, 0); + zend_hash_destroy(&PHPDBG_G(watch_free)); + zend_hash_init(&PHPDBG_G(watch_free), 8, NULL, NULL, 0); + zend_hash_destroy(&PHPDBG_G(watch_collisions)); + zend_hash_init(&PHPDBG_G(watch_collisions), 8, NULL, NULL, 0); +} + void phpdbg_purge_watchpoint_tree(void) { phpdbg_btree_position pos; phpdbg_btree_result *res; diff --git a/sapi/phpdbg/phpdbg_watch.h b/sapi/phpdbg/phpdbg_watch.h index 17d9dfac93dd..7a794dd0b2ea 100644 --- a/sapi/phpdbg/phpdbg_watch.h +++ b/sapi/phpdbg/phpdbg_watch.h @@ -112,6 +112,7 @@ typedef struct { void phpdbg_setup_watchpoints(void); void phpdbg_destroy_watchpoints(void); +void phpdbg_release_watch_elements(void); void phpdbg_purge_watchpoint_tree(void); #ifndef _WIN32 diff --git a/sapi/phpdbg/tests/gh22480.phpt b/sapi/phpdbg/tests/gh22480.phpt new file mode 100644 index 000000000000..16559a75f27a --- /dev/null +++ b/sapi/phpdbg/tests/gh22480.phpt @@ -0,0 +1,34 @@ +--TEST-- +GH-22480 (Use-after-free when re-watching an already-watched variable) +--PHPDBG-- +b 6 +r +w a $a +w a $a +c + +q +--EXPECTF-- +[Successful compilation of %s] +prompt> [Breakpoint #0 added at %s:6] +prompt> [Breakpoint #0 at %s:6, hits: 1] +>00006: $a[0] = 2; + 00007: + 00008: $a = [0 => 3, 1 => 4]; +prompt> [Added watchpoint #0 for $a[]] +prompt> [$a[] is already being watched] +prompt> [Breaking on watchpoint $a[]] +1 elements were added to the array +>00009: +prompt> [$a[] has been removed, removing watchpoint] +[Script ended normally] +prompt> +--FILE-- + 3, 1 => 4]; diff --git a/sapi/phpdbg/tests/gh22480_2.phpt b/sapi/phpdbg/tests/gh22480_2.phpt new file mode 100644 index 000000000000..aa6e13dbf8d5 --- /dev/null +++ b/sapi/phpdbg/tests/gh22480_2.phpt @@ -0,0 +1,30 @@ +--TEST-- +GH-22480 (Use-after-free re-watching a sub-path of a recursive watchpoint) +--INI-- +opcache.optimization_level=0 +--PHPDBG-- +b 5 +r +w r $a +w $a[x] +c +q +--EXPECTF-- +[Successful compilation of %s] +prompt> [Breakpoint #0 added at %s:5] +prompt> [Breakpoint #0 at %s:5, hits: 1] +>00005: $a['x'] = 2; + 00006: +prompt> [Added watchpoint #0 for $a[]] +prompt> [$a[x] is already being watched] +prompt> [Breaking on watchpoint $a] +Old value%s +New value: Array ([x] => 2) +>00006: +prompt> [$a has been removed, removing watchpoint recursively] +--FILE-- + 1]; + +$a['x'] = 2; diff --git a/sapi/phpdbg/tests/watch_006.phpt b/sapi/phpdbg/tests/watch_006.phpt index 5b5ca9ee57c0..bf38b8eff1fb 100644 --- a/sapi/phpdbg/tests/watch_006.phpt +++ b/sapi/phpdbg/tests/watch_006.phpt @@ -1,14 +1,5 @@ --TEST-- Test multiple watch elements pointing to the same watchpoint ---SKIPIF-- - --PHPDBG-- b 4 r From ff356c5cb8e96b0f481bca0d6fcf2e028314412a Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Fri, 24 Jul 2026 11:14:15 -0400 Subject: [PATCH 2/3] Fix phpdbg over-read when watching a packed array element (#22756) phpdbg watched every array element as a Bucket, but packed arrays store bare zvals and de-indirected stack variables aren't buckets, so reading Bucket.h/.key over-read the neighbour and a sibling write tripped a phantom break. Watch such elements as WATCH_ON_ZVAL and compare the type info first, reading the value union only for initialised slots so an uninitialised CV is never compared by value. A separate pre-existing bug crashed 32-bit: phpdbg_btree_insert_or_update linked each new node into the tree before initialising its child pointers, so a write to a node on a watched page faulted into the watchpoint handler, which walked the half-built node. Build the node fully, then publish it with a single store. Closes GH-22756 (cherry picked from commit 250f138329e4e255a6361a110a313f3bf3112e80) --- NEWS | 1 + sapi/phpdbg/phpdbg_btree.c | 13 +++++--- sapi/phpdbg/phpdbg_watch.c | 54 +++++++++++++++++++++++++------- sapi/phpdbg/phpdbg_watch.h | 1 + sapi/phpdbg/tests/watch_005.phpt | 9 ------ sapi/phpdbg/tests/watch_006.phpt | 6 ---- 6 files changed, 53 insertions(+), 31 deletions(-) diff --git a/NEWS b/NEWS index 5c9720923443..6b584d083883 100644 --- a/NEWS +++ b/NEWS @@ -123,6 +123,7 @@ PHP NEWS . Fix wrong command names. (ndossche, Weilin Du) . Fixed bug GH-22480 (Use-after-free when re-watching an already-watched variable). (Ilia Alshanetsky) + . Fixed over-read when watching a packed array element. (Ilia Alshanetsky) - Readline: . Fixed a crash in readline_info() when completion_append_character diff --git a/sapi/phpdbg/phpdbg_btree.c b/sapi/phpdbg/phpdbg_btree.c index 81571c17c01a..800039d30427 100644 --- a/sapi/phpdbg/phpdbg_btree.c +++ b/sapi/phpdbg/phpdbg_btree.c @@ -158,13 +158,18 @@ int phpdbg_btree_insert_or_update(phpdbg_btree *tree, zend_ulong idx, void *ptr, } { - phpdbg_btree_branch *memory = *branch = pemalloc((i + 2) * sizeof(phpdbg_btree_branch), tree->persistent); + phpdbg_btree_branch *memory = pemalloc((i + 2) * sizeof(phpdbg_btree_branch), tree->persistent); + phpdbg_btree_branch *node = memory; do { - (*branch)->branches[!((idx >> i) % 2)] = NULL; - branch = &(*branch)->branches[(idx >> i) % 2]; - *branch = ++memory; + node->branches[!((idx >> i) % 2)] = NULL; + node->branches[(idx >> i) % 2] = node + 1; + node++; } while (i--); + node->result.idx = idx; + node->result.ptr = ptr; + *branch = memory; tree->count++; + return SUCCESS; } } else if (!(flags & PHPDBG_BTREE_UPDATE)) { return FAILURE; diff --git a/sapi/phpdbg/phpdbg_watch.c b/sapi/phpdbg/phpdbg_watch.c index 848ef6b2cfca..efd216021032 100644 --- a/sapi/phpdbg/phpdbg_watch.c +++ b/sapi/phpdbg/phpdbg_watch.c @@ -135,6 +135,16 @@ const phpdbg_command_t phpdbg_watch_commands[] = { #define HT_WATCH_HT(watch) HT_PTR_HT((watch)->addr.ptr) /* ### PRINTING POINTER DIFFERENCES ### */ +static bool phpdbg_check_zval_watch_diff(zval *oldPtr, zval *newPtr) { + if (Z_TYPE_INFO_P(oldPtr) != Z_TYPE_INFO_P(newPtr)) { + return true; + } + if (Z_TYPE_P(oldPtr) < IS_LONG) { + return false; + } + return memcmp(oldPtr, newPtr, sizeof(zend_value)) != 0; +} + bool phpdbg_check_watch_diff(phpdbg_watchtype type, void *oldPtr, void *newPtr) { switch (type) { case WATCH_ON_BUCKET: @@ -144,7 +154,7 @@ bool phpdbg_check_watch_diff(phpdbg_watchtype type, void *oldPtr, void *newPtr) /* TODO: Is this intentional? */ ZEND_FALLTHROUGH; case WATCH_ON_ZVAL: - return memcmp(oldPtr, newPtr, sizeof(zend_value) + sizeof(uint32_t) /* value + typeinfo */) != 0; + return phpdbg_check_zval_watch_diff((zval *) oldPtr, (zval *) newPtr); case WATCH_ON_HASHTABLE: return zend_hash_num_elements(HT_PTR_HT(oldPtr)) != zend_hash_num_elements(HT_PTR_HT(newPtr)); case WATCH_ON_REFCOUNTED: @@ -570,9 +580,26 @@ phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdb return element; } -phpdbg_watch_element *phpdbg_add_bucket_watch_element(Bucket *bucket, phpdbg_watch_element *element, bool *is_new) { +static bool phpdbg_zval_is_bucket_of(HashTable *ht, zval *zv) { + if (!ht || HT_IS_PACKED(ht)) { + return false; + } + if ((uintptr_t) zv < (uintptr_t) ht->arData + || (uintptr_t) zv >= (uintptr_t) (ht->arData + ht->nNumUsed)) { + return false; + } + ZEND_ASSERT(((uintptr_t) zv - (uintptr_t) ht->arData) % sizeof(Bucket) == 0); + return true; +} + +phpdbg_watch_element *phpdbg_add_bucket_watch_element(zval *zv, phpdbg_watch_element *element, bool *is_new) { phpdbg_watchpoint_t watch; - phpdbg_set_bucket_watchpoint(bucket, &watch); + + if (phpdbg_zval_is_bucket_of(element->parent_container, zv)) { + phpdbg_set_bucket_watchpoint((Bucket *) zv, &watch); + } else { + phpdbg_set_zval_watchpoint(zv, &watch); + } bool added_new; phpdbg_watch_element *added = phpdbg_add_watch_element(&watch, element, &added_new); if (added_new) { @@ -639,7 +666,7 @@ void phpdbg_add_recursive_watch_from_ht(phpdbg_watch_element *element, zend_long return; } bool added_new; - phpdbg_add_bucket_watch_element((Bucket *) zv, child, &added_new); + phpdbg_add_bucket_watch_element(zv, child, &added_new); if (added_new) { zend_hash_add_ptr(&element->child_container, child->str, child); } @@ -699,7 +726,7 @@ void phpdbg_recurse_watch_element(phpdbg_watch_element *element) { } void phpdbg_watch_parent_ht(phpdbg_watch_element *element) { - if (element->watch->type == WATCH_ON_BUCKET) { + if (element->watch->type == WATCH_ON_BUCKET || element->watch->type == WATCH_ON_ZVAL) { phpdbg_btree_result *res; phpdbg_watch_ht_info *hti; ZEND_ASSERT(element->parent_container); @@ -718,14 +745,17 @@ void phpdbg_watch_parent_ht(phpdbg_watch_element *element) { hti = (phpdbg_watch_ht_info *) res->ptr; } - zend_hash_add_ptr(&hti->watches, element->name_in_parent, element); + if (zend_hash_add_ptr(&hti->watches, element->name_in_parent, element)) { + element->flags |= PHPDBG_WATCH_HT_REGISTERED; + } } } void phpdbg_unwatch_parent_ht(phpdbg_watch_element *element) { - if (element->watch && element->watch->type == WATCH_ON_BUCKET) { + if (element->flags & PHPDBG_WATCH_HT_REGISTERED) { phpdbg_btree_result *res = phpdbg_btree_find(&PHPDBG_G(watch_HashTables), (zend_ulong) element->parent_container); ZEND_ASSERT(element->parent_container); + element->flags &= ~PHPDBG_WATCH_HT_REGISTERED; if (res) { phpdbg_watch_ht_info *hti = res->ptr; @@ -811,7 +841,7 @@ bool phpdbg_try_re_adding_watch_element(zval *parent, phpdbg_watch_element *elem return false; } element->parent_container = ht; - element = phpdbg_add_bucket_watch_element((Bucket *) zv, element, NULL); + element = phpdbg_add_bucket_watch_element(zv, element, NULL); } else { return 0; } @@ -1322,7 +1352,7 @@ static phpdbg_watch_add_result phpdbg_create_simple_watchpoint(zval *zv, phpdbg_ phpdbg_watch_element *added; (*element)->flags = PHPDBG_WATCH_SIMPLE; bool added_new; - added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new); + added = phpdbg_add_bucket_watch_element(zv, *element, &added_new); if (!added_new) { *element = added; return PHPDBG_WATCH_ADD_DUPLICATE; @@ -1346,7 +1376,7 @@ static phpdbg_watch_add_result phpdbg_create_array_watchpoint(zval *zv, phpdbg_w element->str = str; element->flags = PHPDBG_WATCH_IMPLICIT; bool added_new; - added = phpdbg_add_bucket_watch_element((Bucket *) orig_zv, element, &added_new); + added = phpdbg_add_bucket_watch_element(orig_zv, element, &added_new); if (!added_new) { *element_ptr = added; return PHPDBG_WATCH_ADD_DUPLICATE; @@ -1374,7 +1404,7 @@ static phpdbg_watch_add_result phpdbg_create_recursive_watchpoint(zval *zv, phpd (*element)->flags = PHPDBG_WATCH_RECURSIVE | PHPDBG_WATCH_RECURSIVE_ROOT; (*element)->child = NULL; bool added_new; - added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new); + added = phpdbg_add_bucket_watch_element(zv, *element, &added_new); if (!added_new) { *element = added; return PHPDBG_WATCH_ADD_DUPLICATE; @@ -1453,7 +1483,7 @@ static int phpdbg_watchpoint_parse_step(char *name, size_t namelen, char *key, s element->name_in_parent = zend_string_init(key, keylen, 0); element->parent_container = parent; element->parent = PHPDBG_G(watch_tmp); - element = phpdbg_add_bucket_watch_element((Bucket *) zv, element, NULL); + element = phpdbg_add_bucket_watch_element(zv, element, NULL); efree(name); efree(key); diff --git a/sapi/phpdbg/phpdbg_watch.h b/sapi/phpdbg/phpdbg_watch.h index 7a794dd0b2ea..805a4ef190da 100644 --- a/sapi/phpdbg/phpdbg_watch.h +++ b/sapi/phpdbg/phpdbg_watch.h @@ -56,6 +56,7 @@ typedef enum { #define PHPDBG_WATCH_NORMAL (PHPDBG_WATCH_SIMPLE | PHPDBG_WATCH_RECURSIVE) #define PHPDBG_WATCH_IMPLICIT 0x10 #define PHPDBG_WATCH_RECURSIVE_ROOT 0x20 +#define PHPDBG_WATCH_HT_REGISTERED 0x40 typedef struct _phpdbg_watch_collision phpdbg_watch_collision; diff --git a/sapi/phpdbg/tests/watch_005.phpt b/sapi/phpdbg/tests/watch_005.phpt index aacc158f6174..d6bae9d3f3fa 100644 --- a/sapi/phpdbg/tests/watch_005.phpt +++ b/sapi/phpdbg/tests/watch_005.phpt @@ -1,14 +1,5 @@ --TEST-- Test proper watch comparisons when having multiple levels of indirection from a zval to its value ---SKIPIF-- - --PHPDBG-- b 3 r diff --git a/sapi/phpdbg/tests/watch_006.phpt b/sapi/phpdbg/tests/watch_006.phpt index bf38b8eff1fb..21b77a982d63 100644 --- a/sapi/phpdbg/tests/watch_006.phpt +++ b/sapi/phpdbg/tests/watch_006.phpt @@ -11,7 +11,6 @@ c - q --EXPECTF-- [Successful compilation of %s] @@ -48,11 +47,6 @@ prompt> [Element 1 has been added to watchpoint] 00010: prompt> [Breaking on watchpoint $b] Old value inaccessible or destroyed -New value (reference): Array ([0] => 2,[1] => 3) ->00009: $b = &$c; - 00010: -prompt> [Breaking on watchpoint $b] -Old value inaccessible or destroyed New value (reference): Array ([0] => 1) >00010: prompt> [$b has been removed, removing watchpoint recursively] From 1be30f919f2d1a8515d0ac7d66746b4e114a1af9 Mon Sep 17 00:00:00 2001 From: Arnaud Le Blanc Date: Wed, 26 Aug 2026 18:25:39 +0200 Subject: [PATCH 3/3] Fix double free of phpdbg watch element chains PHPDBG_G(watch_recreation) is keyed by element->str, so a watch element and its implicit parent are stored under distinct keys ("$lower[0]" and "$lower[]"). The deduplication in phpdbg_queue_element_for_recreation() only walks down from the entry found under the *same* key, so it never notices that the two belong to the same chain and both get queued. phpdbg_free_watch_element_tree() frees the element together with its entire parent and child chains, so draining watch_recreation freed that chain twice: the first entry frees the whole chain, the second one then walks the already freed links and frees them again. Drop every entry referencing a member of the chain before freeing it. The buckets are nulled out rather than deleted, as all callers are iterating over the hash at that point; they clean it right afterwards. (cherry picked from commit 9d6fd4fd98aa9e7662f7b8082ce328b20c6be7b6) --- NEWS | 1 + sapi/phpdbg/phpdbg_watch.c | 40 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/NEWS b/NEWS index 6b584d083883..24e593dea28b 100644 --- a/NEWS +++ b/NEWS @@ -124,6 +124,7 @@ PHP NEWS . Fixed bug GH-22480 (Use-after-free when re-watching an already-watched variable). (Ilia Alshanetsky) . Fixed over-read when watching a packed array element. (Ilia Alshanetsky) + . Fixed double free of watch element chains. (Arnaud) - Readline: . Fixed a crash in readline_info() when completion_append_character diff --git a/sapi/phpdbg/phpdbg_watch.c b/sapi/phpdbg/phpdbg_watch.c index efd216021032..1c63f26ea70e 100644 --- a/sapi/phpdbg/phpdbg_watch.c +++ b/sapi/phpdbg/phpdbg_watch.c @@ -849,8 +849,36 @@ bool phpdbg_try_re_adding_watch_element(zval *parent, phpdbg_watch_element *elem return 1; } +/* watch_recreation is keyed by element->str, so a parent and its child are stored + * under distinct keys and the deduplication in phpdbg_queue_element_for_recreation() + * cannot notice that they belong to the same chain. As phpdbg_free_watch_element_tree() + * frees the whole chain, any other entry referencing a member of it must be dropped + * first, or the chain gets freed twice. The buckets are only nulled out as the hash is + * being iterated over by the callers; they clean it right after. */ +static void phpdbg_forget_queued_watch_element(phpdbg_watch_element *element) { + zval *zv = zend_hash_find(&PHPDBG_G(watch_recreation), element->str); + if (zv && Z_PTR_P(zv) == element) { + Z_PTR_P(zv) = NULL; + } +} + +static void phpdbg_dequeue_watch_element_tree(phpdbg_watch_element *element) { + phpdbg_watch_element *cur; + + for (cur = element->parent; cur; cur = cur->parent) { + phpdbg_forget_queued_watch_element(cur); + } + for (cur = element->child; cur; cur = cur->child) { + phpdbg_forget_queued_watch_element(cur); + } + phpdbg_forget_queued_watch_element(element); +} + void phpdbg_automatic_dequeue_free(phpdbg_watch_element *element) { phpdbg_watch_element *child = element; + + phpdbg_dequeue_watch_element_tree(element); + while (child->child && !(child->flags & PHPDBG_WATCH_RECURSIVE_ROOT)) { child = child->child; } @@ -865,6 +893,10 @@ void phpdbg_dequeue_elements_for_recreation(void) { phpdbg_watch_element *element; ZEND_HASH_MAP_FOREACH_PTR(&PHPDBG_G(watch_recreation), element) { + if (!element) { + /* freed along with an already dequeued element of the same chain */ + continue; + } ZEND_ASSERT(element->flags & (PHPDBG_WATCH_IMPLICIT | PHPDBG_WATCH_RECURSIVE_ROOT | PHPDBG_WATCH_SIMPLE)); if (element->parent || zend_hash_index_find(&PHPDBG_G(watch_free), (zend_ulong) element->parent_container)) { zval _zv, *zv = &_zv; @@ -1643,7 +1675,9 @@ void phpdbg_destroy_watchpoints(void) { /* unconditionally free all remaining elements to avoid memory leaks */ ZEND_HASH_MAP_FOREACH_PTR(&PHPDBG_G(watch_recreation), element) { - phpdbg_automatic_dequeue_free(element); + if (element) { + phpdbg_automatic_dequeue_free(element); + } } ZEND_HASH_FOREACH_END(); /* upon fatal errors etc. (i.e. CG(unclean_shutdown) == 1), some watchpoints may still be active. Ensure memory is not watched anymore for next run. Do not care about memory freeing here, shutdown is unclean and near anyway. */ @@ -1671,7 +1705,9 @@ void phpdbg_release_watch_elements(void) { uint32_t guard; ZEND_HASH_MAP_FOREACH_PTR(&PHPDBG_G(watch_recreation), element) { - phpdbg_automatic_dequeue_free(element); + if (element) { + phpdbg_automatic_dequeue_free(element); + } } ZEND_HASH_FOREACH_END(); zend_hash_clean(&PHPDBG_G(watch_recreation));