Fix phpdbg over-read watching an element of a packed array

phpdbg watched every array element as a Bucket, but a packed array
stores bare zvals, so reading Bucket.h/.key over-read the neighbouring
element and a sibling write tripped a phantom break. De-indirected stack
variables are not buckets either. Watch such elements as WATCH_ON_ZVAL,
re-resolve them in the parent on relocation, drop the watch when the
stack frame releases (IS_UNDEF), and compare the type info before the
value so uninitialised CV bytes are never read.

phpdbg_btree_insert_or_update published each freshly allocated node into
the tree before initialising its child pointers. When the node landed on
a watched page the write that initialised it faulted into the watchpoint
handler, which walked the half-built node and dereferenced a wild
pointer. Build the node fully, then link it in with a single store. This
crashed only on 32-bit, where the compact heap places the node on the
same page as the watched zval.

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 = node + 1;
 			} 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,28 @@ phpdbg_watch_element *phpdbg_add_watch_element(phpdbg_watchpoint_t *watch, phpdb
 	return element;
 }
 
+static bool phpdbg_zval_is_bucket_of(HashTable *ht, zval *zv) {
+	uintptr_t off;
+
+	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;
+	}
+	off = (uintptr_t) zv - (uintptr_t) ht->arData;
+	return off % sizeof(Bucket) == 0;
+}
+
 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);
+
+	if (phpdbg_zval_is_bucket_of(element->parent_container, (zval *) bucket)) {
+		phpdbg_set_bucket_watchpoint(bucket, &watch);
+	} else {
+		phpdbg_set_zval_watchpoint((zval *) bucket, &watch);
+	}
 	bool added_new;
 	phpdbg_watch_element *added = phpdbg_add_watch_element(&watch, element, &added_new);
 	if (added_new) {
@@ -697,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);
@@ -716,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;
 
@@ -1120,6 +1152,32 @@ void phpdbg_check_watchpoint(phpdbg_watchpoint_t *watch) {
 		default:
 			comparePtr = &watch->backup;
 	}
+	if (watch->type == WATCH_ON_BUCKET || watch->type == WATCH_ON_ZVAL) {
+		phpdbg_watch_element *first = NULL;
+		ZEND_HASH_MAP_FOREACH_PTR(&watch->elements, first) {
+			break;
+		} ZEND_HASH_FOREACH_END();
+		if (first && first->parent_container && HT_IS_PACKED(first->parent_container)) {
+			zval *new = zend_symtable_find(first->parent_container, first->name_in_parent);
+			if (!new || Z_TYPE_P(new) == IS_UNDEF) {
+				phpdbg_remove_watchpoint(watch);
+				return;
+			}
+			if (new != watch->addr.zv) {
+				phpdbg_remove_watchpoint_btree(watch);
+				phpdbg_deactivate_watchpoint(watch);
+				watch->addr.zv = new;
+				phpdbg_store_watchpoint_btree(watch);
+				phpdbg_activate_watchpoint(watch);
+			}
+			if (!phpdbg_check_watch_diff(WATCH_ON_ZVAL, &watch->backup.bucket.val, watch->addr.ptr)) {
+				phpdbg_watch_backup_data(watch);
+				return;
+			}
+			goto changed;
+		}
+	}
+
 	if (!phpdbg_check_watch_diff(watch->type, comparePtr, watch->addr.ptr)) {
 		return;
 	}
@@ -1160,8 +1218,14 @@ void phpdbg_check_watchpoint(phpdbg_watchpoint_t *watch) {
 			phpdbg_remove_watchpoint(watch);
 			return;
 		}
+	} else if (watch->type == WATCH_ON_ZVAL && !watch->coll
+	 && zend_hash_num_elements(&watch->elements) > 0
+	 && Z_TYPE_P(watch->addr.zv) == IS_UNDEF) {
+		phpdbg_remove_watchpoint(watch);
+		return;
 	}
 
+changed:
 	name = phpdbg_watchpoint_change_collision_name(watch);
 
 	if (name) {

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]