Merge branch 'PHP-8.3' into PHP-8.4

nielsdos · nielsdos · commit 575ee23bd9aa · 2024-12-27T15:54:37.000+01:00
* PHP-8.3: Fix GH-17187: unreachable program point in zend_hash
diff --git a/NEWS b/NEWS
@@ -120,6 +120,9 @@ PHP                                                                        NEWS
 - Windows:
   . Hardened proc_open() against cmd.exe hijacking. (cmb)
 
+- XML:
+  . Fixed bug GH-1718 (unreachable program point in zend_hash). (nielsdos)
+
 05 Dec 2024, PHP 8.4.2
 
 - BcMath:
diff --git a/ext/xml/tests/gh17187_1.phpt b/ext/xml/tests/gh17187_1.phpt
@@ -0,0 +1,93 @@
+--TEST--
+GH-17187 (unreachable program point in zend_hash)
+--EXTENSIONS--
+xml
+--CREDITS--
+chongwick
+--FILE--
+<?php
+class ImmutableParser {
+    private $parser;
+    private $immutableData;
+    private $arrayCopy;
+
+    public function __construct() {
+        $this->parser = xml_parser_create();
+        xml_set_element_handler($this->parser, function ($parser, $name, $attrs) {
+            echo "open\n";
+            var_dump($name, $attrs);
+            $this->arrayCopy = [$this]; // Create cycle intentionally
+            $this->immutableData = $this->arrayCopy;
+        }, function ($parser, $name) {
+            echo "close\n";
+            var_dump($name);
+        });
+    }
+
+    public function parseXml($xml) {
+        $this->immutableData = array();
+        xml_parse_into_struct($this->parser, $xml, $this->immutableData, $this->immutableData);
+        return $this->immutableData;
+    }
+}
+$immutableParser = new ImmutableParser();
+$xml = "<container><child/></container>";
+$immutableData = $immutableParser->parseXml($xml);
+var_dump($immutableData);
+?>
+--EXPECT--
+open
+string(9) "CONTAINER"
+array(0) {
+}
+open
+string(5) "CHILD"
+array(0) {
+}
+close
+string(5) "CHILD"
+close
+string(9) "CONTAINER"
+array(5) {
+  [0]=>
+  object(ImmutableParser)#1 (3) {
+    ["parser":"ImmutableParser":private]=>
+    object(XMLParser)#2 (0) {
+    }
+    ["immutableData":"ImmutableParser":private]=>
+    *RECURSION*
+    ["arrayCopy":"ImmutableParser":private]=>
+    array(1) {
+      [0]=>
+      *RECURSION*
+    }
+  }
+  ["CHILD"]=>
+  array(1) {
+    [0]=>
+    int(1)
+  }
+  [1]=>
+  array(3) {
+    ["tag"]=>
+    string(5) "CHILD"
+    ["type"]=>
+    string(8) "complete"
+    ["level"]=>
+    int(2)
+  }
+  ["CONTAINER"]=>
+  array(1) {
+    [0]=>
+    int(2)
+  }
+  [2]=>
+  array(3) {
+    ["tag"]=>
+    string(9) "CONTAINER"
+    ["type"]=>
+    string(5) "close"
+    ["level"]=>
+    int(1)
+  }
+}
diff --git a/ext/xml/tests/gh17187_2.phpt b/ext/xml/tests/gh17187_2.phpt
@@ -0,0 +1,53 @@
+--TEST--
+GH-17187 (unreachable program point in zend_hash)
+--EXTENSIONS--
+xml
+--CREDITS--
+chongwick
+--FILE--
+<?php
+class ImmutableParser {
+    public $parser;
+    public $immutableData1;
+    public $immutableData2;
+
+    public function __construct() {
+        $this->parser = xml_parser_create();
+        xml_set_element_handler($this->parser, function ($parser, $name, $attrs) {
+            echo "open\n";
+            var_dump($name, $attrs);
+            $this->immutableData1 = 0xdead;
+            $this->immutableData2 = 0xbeef;
+        }, function ($parser, $name) {
+            echo "close\n";
+            var_dump($name);
+        });
+    }
+
+    public function parseXml($xml) {
+        $this->immutableData1 = array();
+        $this->immutableData2 = array();
+        xml_parse_into_struct($this->parser, $xml, $this->immutableData1, $this->immutableData2);
+    }
+}
+$immutableParser = new ImmutableParser();
+$xml = "<container><child/></container>";
+$immutableParser->parseXml($xml);
+var_dump($immutableParser->immutableData1);
+var_dump($immutableParser->immutableData2);
+?>
+--EXPECT--
+open
+string(9) "CONTAINER"
+array(0) {
+}
+open
+string(5) "CHILD"
+array(0) {
+}
+close
+string(5) "CHILD"
+close
+string(9) "CONTAINER"
+int(57005)
+int(48879)
diff --git a/ext/xml/xml.c b/ext/xml/xml.c
@@ -79,13 +79,13 @@ typedef struct {
 	zend_fcall_info_cache externalEntityRefHandler;
 	zend_fcall_info_cache startNamespaceDeclHandler;
 	zend_fcall_info_cache endNamespaceDeclHandler;
-
 	zval data;
 	zval info;
+
 	int level;
 	int toffset;
 	int curtag;
-	zval *ctag;
+	zend_long ctag_index;
 	char **ltags;
 	bool lastwasopen;
 	bool skipwhite;
@@ -333,6 +333,8 @@ static void xml_parser_free_obj(zend_object *object)
 {
 	xml_parser *parser = xml_parser_from_obj(object);
 
+	zval_ptr_dtor(&parser->info);
+	zval_ptr_dtor(&parser->data);
 	if (parser->parser) {
 		XML_ParserFree(parser->parser);
 	}
@@ -425,6 +427,8 @@ static HashTable *xml_parser_get_gc(zend_object *object, zval **table, int *n)
 	if (ZEND_FCC_INITIALIZED(parser->endNamespaceDeclHandler)) {
 		zend_get_gc_buffer_add_fcc(gc_buffer, &parser->endNamespaceDeclHandler);
 	}
+	zend_get_gc_buffer_add_zval(gc_buffer, &parser->data);
+	zend_get_gc_buffer_add_zval(gc_buffer, &parser->info);
 
 	zend_get_gc_buffer_use(gc_buffer, table, n);
 
@@ -553,15 +557,18 @@ static void xml_add_to_info(xml_parser *parser, const char *name)
 {
 	zval *element;
 
-	if (Z_ISUNDEF(parser->info)) {
+	if (Z_ISUNDEF(parser->info) || UNEXPECTED(Z_TYPE_P(Z_REFVAL(parser->info)) != IS_ARRAY)) {
 		return;
 	}
 
+	SEPARATE_ARRAY(Z_REFVAL(parser->info));
+	zend_array *arr = Z_ARRVAL_P(Z_REFVAL(parser->info));
+
 	size_t name_len = strlen(name);
-	if ((element = zend_hash_str_find(Z_ARRVAL(parser->info), name, name_len)) == NULL) {
+	if ((element = zend_hash_str_find(arr, name, name_len)) == NULL) {
 		zval values;
 		array_init(&values);
-		element = zend_hash_str_update(Z_ARRVAL(parser->info), name, name_len, &values);
+		element = zend_hash_str_update(arr, name, name_len, &values);
 	}
 
 	add_next_index_long(element, parser->curtag);
@@ -585,6 +592,28 @@ static zend_string *xml_decode_tag(xml_parser *parser, const XML_Char *tag)
 }
 /* }}} */
 
+static zval *xml_get_separated_data(xml_parser *parser)
+{
+	if (EXPECTED(Z_TYPE_P(Z_REFVAL(parser->data)) == IS_ARRAY)) {
+		SEPARATE_ARRAY(Z_REFVAL(parser->data));
+		return Z_REFVAL(parser->data);
+	}
+	return NULL;
+}
+
+static zval *xml_get_ctag(xml_parser *parser)
+{
+	zval *data = xml_get_separated_data(parser);
+	if (EXPECTED(data)) {
+		zval *zv = zend_hash_index_find_deref(Z_ARRVAL_P(data), parser->ctag_index);
+		if (EXPECTED(zv && Z_TYPE_P(zv) == IS_ARRAY)) {
+			SEPARATE_ARRAY(zv);
+			return zv;
+		}
+	}
+	return NULL;
+}
+
 /* {{{ xml_startElementHandler() */
 void xml_startElementHandler(void *userData, const XML_Char *name, const XML_Char **attributes)
 {
@@ -666,7 +695,19 @@ void xml_startElementHandler(void *userData, const XML_Char *name, const XML_Cha
 				zval_ptr_dtor(&atr);
 			}
 
-			parser->ctag = zend_hash_next_index_insert(Z_ARRVAL(parser->data), &tag);
+			zval *data = xml_get_separated_data(parser);
+			if (EXPECTED(data)) {
+				/* Note: due to array resizes or user interference,
+				 * we have to store an index instead of a zval into the array's memory. */
+				zend_array *arr = Z_ARRVAL_P(data);
+				if (EXPECTED(zend_hash_next_index_insert(arr, &tag))) {
+					parser->ctag_index = arr->nNextFreeElement - 1;
+				} else {
+					zval_ptr_dtor(&tag);
+				}
+			} else {
+				zval_ptr_dtor(&tag);
+			}
 		} else if (parser->level == (XML_MAXLEVEL + 1)) {
 						php_error_docref(NULL, E_WARNING, "Maximum depth exceeded - Results truncated");
 		}
@@ -701,17 +742,21 @@ void xml_endElementHandler(void *userData, const XML_Char *name)
 		zval tag;
 
 		if (parser->lastwasopen) {
-			add_assoc_string(parser->ctag, "type", "complete");
+			zval *zv = xml_get_ctag(parser);
+			if (EXPECTED(zv)) {
+				add_assoc_string(zv, "type", "complete");
+			}
 		} else {
-			array_init(&tag);
-
 			xml_add_to_info(parser, ZSTR_VAL(tag_name) + parser->toffset);
 
-			add_assoc_string(&tag, "tag", SKIP_TAGSTART(ZSTR_VAL(tag_name))); /* cast to avoid gcc-warning */
-			add_assoc_string(&tag, "type", "close");
-			add_assoc_long(&tag, "level", parser->level);
-
-			zend_hash_next_index_insert(Z_ARRVAL(parser->data), &tag);
+			zval *data = xml_get_separated_data(parser);
+			if (EXPECTED(data)) {
+				array_init(&tag);
+				add_assoc_string(&tag, "tag", SKIP_TAGSTART(ZSTR_VAL(tag_name))); /* cast to avoid gcc-warning */
+				add_assoc_string(&tag, "type", "close");
+				add_assoc_long(&tag, "level", parser->level);
+				zend_hash_next_index_insert(Z_ARRVAL_P(data), &tag);
+			}
 		}
 
 		parser->lastwasopen = 0;
@@ -770,27 +815,41 @@ void xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 		}
 	}
 	if (parser->lastwasopen) {
+		zval *ctag = xml_get_ctag(parser);
+		if (UNEXPECTED(!ctag)) {
+			zend_string_release_ex(decoded_value, false);
+			return;
+		}
+
 		zval *myval;
 		/* check if the current tag already has a value - if yes append to that! */
-		if ((myval = zend_hash_find(Z_ARRVAL_P(parser->ctag), ZSTR_KNOWN(ZEND_STR_VALUE)))) {
+		if ((myval = zend_hash_find(Z_ARRVAL_P(ctag), ZSTR_KNOWN(ZEND_STR_VALUE))) && Z_TYPE_P(myval) == IS_STRING) {
 			size_t newlen = Z_STRLEN_P(myval) + ZSTR_LEN(decoded_value);
 			Z_STR_P(myval) = zend_string_extend(Z_STR_P(myval), newlen, 0);
 			strncpy(Z_STRVAL_P(myval) + Z_STRLEN_P(myval) - ZSTR_LEN(decoded_value),
 					ZSTR_VAL(decoded_value), ZSTR_LEN(decoded_value) + 1);
 			zend_string_release_ex(decoded_value, 0);
 		} else {
 			if (doprint || (! parser->skipwhite)) {
-				add_assoc_str(parser->ctag, "value", decoded_value);
+				add_assoc_str(ctag, "value", decoded_value);
 			} else {
 				zend_string_release_ex(decoded_value, 0);
 			}
 		}
 	} else {
 		zval tag;
 		zval *curtag, *mytype, *myval;
-		ZEND_HASH_REVERSE_FOREACH_VAL(Z_ARRVAL(parser->data), curtag) {
-			if ((mytype = zend_hash_str_find(Z_ARRVAL_P(curtag),"type", sizeof("type") - 1))) {
-				if (zend_string_equals_literal(Z_STR_P(mytype), "cdata")) {
+
+		zval *data = xml_get_separated_data(parser);
+		if (UNEXPECTED(!data)) {
+			zend_string_release_ex(decoded_value, false);
+			return;
+		}
+
+		ZEND_HASH_REVERSE_FOREACH_VAL(Z_ARRVAL_P(data), curtag) {
+			if (EXPECTED(Z_TYPE_P(curtag) == IS_ARRAY) && (mytype = zend_hash_str_find(Z_ARRVAL_P(curtag),"type", sizeof("type") - 1))) {
+				if (EXPECTED(Z_TYPE_P(mytype) == IS_STRING) && zend_string_equals_literal(Z_STR_P(mytype), "cdata")) {
+					SEPARATE_ARRAY(curtag);
 					if ((myval = zend_hash_find(Z_ARRVAL_P(curtag), ZSTR_KNOWN(ZEND_STR_VALUE)))) {
 						size_t newlen = Z_STRLEN_P(myval) + ZSTR_LEN(decoded_value);
 						Z_STR_P(myval) = zend_string_extend(Z_STR_P(myval), newlen, 0);
@@ -810,7 +869,7 @@ void xml_characterDataHandler(void *userData, const XML_Char *s, int len)
 			add_assoc_str(&tag, "value", decoded_value);
 			add_assoc_string(&tag, "type", "cdata");
 			add_assoc_long(&tag, "level", parser->level);
-			zend_hash_next_index_insert(Z_ARRVAL(parser->data), &tag);
+			zend_hash_next_index_insert(Z_ARRVAL_P(data), &tag);
 		} else if (parser->level == (XML_MAXLEVEL + 1)) {
 								php_error_docref(NULL, E_WARNING, "Maximum depth exceeded - Results truncated");
 		} else {
@@ -1366,21 +1425,21 @@ PHP_FUNCTION(xml_parse_into_struct)
 	}
 
 	if (info) {
-		info = zend_try_array_init(info);
-		if (!info) {
+		if (!zend_try_array_init(info)) {
 			RETURN_THROWS();
 		}
 	}
 
-	xdata = zend_try_array_init(xdata);
-	if (!xdata) {
+	if (!zend_try_array_init(xdata)) {
 		RETURN_THROWS();
 	}
 
-	ZVAL_COPY_VALUE(&parser->data, xdata);
+	zval_ptr_dtor(&parser->data);
+	ZVAL_COPY(&parser->data, xdata);
 
 	if (info) {
-		ZVAL_COPY_VALUE(&parser->info, info);
+		zval_ptr_dtor(&parser->info);
+		ZVAL_COPY(&parser->info, info);
 	}
 
 	parser->level = 0;
