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

Author: iliaal

sapi/phpdbg/phpdbg_btree.c

@@ -156,13 +156,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;

sapi/phpdbg/phpdbg_watch.c

@@ -133,6 +133,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:
@@ -142,7 +152,7 @@ bool phpdbg_check_watch_diff(phpdbg_watchtype type, void *oldPtr, void *newPtr)
 			/* Fall through to also compare the value from the bucket. */
 			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:
@@ -568,9 +578,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) {
@@ -637,7 +664,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);
 	}
@@ -697,7 +724,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);
@@ -716,14 +743,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;
 
@@ -809,7 +839,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 false;
 	}
@@ -1320,7 +1350,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;
@@ -1344,7 +1374,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;
@@ -1372,7 +1402,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;
@@ -1451,7 +1481,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);

sapi/phpdbg/phpdbg_watch.h

@@ -54,6 +54,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;
 

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--
-<?php
-if (PHP_INT_SIZE == 4) {
-    die("xfail There may be flaws in the implementation of watchpoints that cause failures");
-}
-if (getenv('SKIP_ASAN')) {
-    die("skip intentionally causes segfaults");
-}
-?>
 --PHPDBG--
 b 3
 r

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]