Skip to content

Commit 6d9d07d

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. Fixes GH-22480
1 parent 8b39b01 commit 6d9d07d

4 files changed

Lines changed: 212 additions & 32 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_watch.c‎

Lines changed: 132 additions & 32 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,14 +1309,23 @@ 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);
1312+
#define PHPDBG_WATCH_DUPLICATE 1
1313+
1314+
static int phpdbg_create_simple_watchpoint(zval *zv, phpdbg_watch_element **element) {
1315+
phpdbg_watch_element *added;
1316+
(*element)->flags = PHPDBG_WATCH_SIMPLE;
1317+
bool added_new;
1318+
added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new);
1319+
if (!added_new) {
1320+
*element = added;
1321+
return PHPDBG_WATCH_DUPLICATE;
1322+
}
12541323
return SUCCESS;
12551324
}
12561325

1257-
static int phpdbg_create_array_watchpoint(zval *zv, phpdbg_watch_element *element) {
1258-
phpdbg_watch_element *new;
1326+
static int phpdbg_create_array_watchpoint(zval *zv, phpdbg_watch_element **element_ptr) {
1327+
phpdbg_watch_element *element = *element_ptr;
1328+
phpdbg_watch_element *new, *added;
12591329
zend_string *str;
12601330
zval *orig_zv = zv;
12611331

@@ -1264,30 +1334,48 @@ static int phpdbg_create_array_watchpoint(zval *zv, phpdbg_watch_element *elemen
12641334
return FAILURE;
12651335
}
12661336

1267-
new = ecalloc(1, sizeof(phpdbg_watch_element));
1268-
12691337
str = strpprintf(0, "%.*s[]", (int) ZSTR_LEN(element->str), ZSTR_VAL(element->str));
12701338
zend_string_release(element->str);
12711339
element->str = str;
12721340
element->flags = PHPDBG_WATCH_IMPLICIT;
1273-
phpdbg_add_bucket_watch_element((Bucket *) orig_zv, element);
1274-
element->child = new;
1341+
bool added_new;
1342+
added = phpdbg_add_bucket_watch_element((Bucket *) orig_zv, element, &added_new);
1343+
if (!added_new) {
1344+
*element_ptr = added;
1345+
return PHPDBG_WATCH_DUPLICATE;
1346+
}
1347+
element = added;
12751348

1349+
new = ecalloc(1, sizeof(phpdbg_watch_element));
12761350
new->flags = PHPDBG_WATCH_SIMPLE;
12771351
new->str = zend_string_copy(str);
12781352
new->parent = element;
1279-
phpdbg_add_ht_watch_element(zv, new);
1353+
added = phpdbg_add_ht_watch_element(zv, new, &added_new);
1354+
if (added != NULL && !added_new) {
1355+
phpdbg_clean_watch_element(element);
1356+
phpdbg_free_watch_element(element);
1357+
*element_ptr = added;
1358+
return PHPDBG_WATCH_DUPLICATE;
1359+
}
1360+
element->child = new;
1361+
*element_ptr = element;
12801362
return SUCCESS;
12811363
}
12821364

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);
1365+
static int phpdbg_create_recursive_watchpoint(zval *zv, phpdbg_watch_element **element) {
1366+
phpdbg_watch_element *added;
1367+
(*element)->flags = PHPDBG_WATCH_RECURSIVE | PHPDBG_WATCH_RECURSIVE_ROOT;
1368+
(*element)->child = NULL;
1369+
bool added_new;
1370+
added = phpdbg_add_bucket_watch_element((Bucket *) zv, *element, &added_new);
1371+
if (!added_new) {
1372+
*element = added;
1373+
return PHPDBG_WATCH_DUPLICATE;
1374+
}
12871375
return SUCCESS;
12881376
}
12891377

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

12921380
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) {
12931381
int ret;
@@ -1298,13 +1386,25 @@ static int phpdbg_watchpoint_parse_wrapper(char *name, size_t namelen, char *key
12981386
element->parent = PHPDBG_G(watch_tmp);
12991387
element->child = NULL;
13001388

1301-
ret = info->callback(zv, element);
1389+
ret = info->callback(zv, &element);
13021390

13031391
efree(name);
13041392
efree(key);
13051393

1306-
if (ret != SUCCESS) {
1394+
if (ret == FAILURE) {
13071395
phpdbg_remove_watch_element(element);
1396+
} else if (ret == PHPDBG_WATCH_DUPLICATE) {
1397+
phpdbg_notice("%.*s is already being watched", (int) ZSTR_LEN(element->str), ZSTR_VAL(element->str));
1398+
while (PHPDBG_G(watch_tmp) && PHPDBG_G(watch_tmp)->child == NULL) {
1399+
phpdbg_watch_element *parent = PHPDBG_G(watch_tmp)->parent;
1400+
phpdbg_clean_watch_element(PHPDBG_G(watch_tmp));
1401+
phpdbg_free_watch_element(PHPDBG_G(watch_tmp));
1402+
if (parent) {
1403+
parent->child = NULL;
1404+
}
1405+
PHPDBG_G(watch_tmp) = parent;
1406+
}
1407+
ret = SUCCESS;
13081408
} else {
13091409
if (PHPDBG_G(watch_tmp)) {
13101410
PHPDBG_G(watch_tmp)->child = element;
@@ -1346,7 +1446,7 @@ static int phpdbg_watchpoint_parse_step(char *name, size_t namelen, char *key, s
13461446
element->name_in_parent = zend_string_init(key, keylen, 0);
13471447
element->parent_container = parent;
13481448
element->parent = PHPDBG_G(watch_tmp);
1349-
element = phpdbg_add_bucket_watch_element((Bucket *) zv, element);
1449+
element = phpdbg_add_bucket_watch_element((Bucket *) zv, element, NULL);
13501450

13511451
efree(name);
13521452
efree(key);
@@ -1359,7 +1459,7 @@ static int phpdbg_watchpoint_parse_step(char *name, size_t namelen, char *key, s
13591459
return SUCCESS;
13601460
}
13611461

1362-
static int phpdbg_watchpoint_parse_symtables(char *input, size_t len, int (*callback)(zval *, phpdbg_watch_element *)) {
1462+
static int phpdbg_watchpoint_parse_symtables(char *input, size_t len, int (*callback)(zval *, phpdbg_watch_element **)) {
13631463
zend_class_entry *scope = zend_get_executed_scope();
13641464
phpdbg_watch_parse_struct info;
13651465
int ret;

‎sapi/phpdbg/tests/gh22480.phpt‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
--TEST--
2+
GH-22480 (Use-after-free when re-watching an already-watched variable)
3+
--SKIPIF--
4+
<?php
5+
if (getenv('SKIP_ASAN')) {
6+
die("skip intentionally causes segfaults");
7+
}
8+
?>
9+
--PHPDBG--
10+
b 6
11+
r
12+
w a $a
13+
w a $a
14+
c
15+
16+
q
17+
--EXPECTF--
18+
[Successful compilation of %s]
19+
prompt> [Breakpoint #0 added at %s:6]
20+
prompt> [Breakpoint #0 at %s:6, hits: 1]
21+
>00006: $a[0] = 2;
22+
00007:
23+
00008: $a = [0 => 3, 1 => 4];
24+
prompt> [Added watchpoint #0 for $a[]]
25+
prompt> [$a[] is already being watched]
26+
prompt> [Breaking on watchpoint $a[]]
27+
1 elements were added to the array
28+
>00009:
29+
prompt> [$a[] has been removed, removing watchpoint]
30+
[Script ended normally]
31+
prompt>
32+
--FILE--
33+
<?php
34+
35+
$a = [];
36+
37+
$a[0] = 1;
38+
$a[0] = 2;
39+
40+
$a = [0 => 3, 1 => 4];

‎sapi/phpdbg/tests/gh22480_2.phpt‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
--TEST--
2+
GH-22480 (Use-after-free re-watching a sub-path of a recursive watchpoint)
3+
--SKIPIF--
4+
<?php
5+
if (getenv('SKIP_ASAN')) {
6+
die("skip intentionally causes segfaults");
7+
}
8+
?>
9+
--INI--
10+
opcache.optimization_level=0
11+
--PHPDBG--
12+
b 5
13+
r
14+
w r $a
15+
w $a[x]
16+
c
17+
q
18+
--EXPECTF--
19+
[Successful compilation of %s]
20+
prompt> [Breakpoint #0 added at %s:5]
21+
prompt> [Breakpoint #0 at %s:5, hits: 1]
22+
>00005: $a['x'] = 2;
23+
00006:
24+
prompt> [Added watchpoint #0 for $a[]]
25+
prompt> [$a[x] is already being watched]
26+
prompt> [Breaking on watchpoint $a]
27+
Old value%s
28+
New value: Array ([x] => 2)
29+
>00006:
30+
prompt> [$a has been removed, removing watchpoint recursively]
31+
--FILE--
32+
<?php
33+
34+
$a = ['x' => 1];
35+
36+
$a['x'] = 2;

0 commit comments

Comments
 (0)