summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorChristoph M. Becker <cmbecker69@gmx.de>2020-08-10 10:40:31 +0200
committerChristoph M. Becker <cmbecker69@gmx.de>2020-08-10 12:23:44 +0200
commit0af3f493121f8eb5f71051303c5ee1a42a81fc6f (patch)
treeefd307aa7d82397faf96300a748b6ba3e6f6671c
parent5be670265b261f2c0162ba89827f5652ba584b05 (diff)
downloadphp-git-0af3f493121f8eb5f71051303c5ee1a42a81fc6f.tar.gz
Fix #79922: Crash after multiple calls to xml_parser_free()
We must not call `zend_list_delete()` in resource closer functions exposed to userland, because decreasing the refcount there leads to use-after-free scenarios. In this case, commit 4a42fbb worked for typical use-cases where `xml_parser_free()` has been called exactly once for the resource, because there is an internal zval (`->index`) referencing the same resource which already increased the refcount by one. However, when `xml_parser_free()` is called multiple times on the same XML parser resource, the resource would be freed prematurely. Instead we forcefully close the resource in `xml_parser_free()`. We also could decrease the refcount of the resource there, but that would require to call `xml_parser_free()` which is somewhat uncommon, and would be particularly bad wrt. PHP 8 where that function is a NOP, and as such doesn't have to be called. So we do no longer increase the refcount of the resource when copying it to the internal zval, and let the usualy refcounting semantics take care of the resource destruction. [1] <http://git.php.net/?p=php-src.git;a=commit;h=4a42fbbbc73aad7427aef5c89974d1833636e082>
-rw-r--r--NEWS3
-rw-r--r--ext/xml/tests/bug79922.phpt17
-rw-r--r--ext/xml/xml.c4
3 files changed, 22 insertions, 2 deletions
diff --git a/NEWS b/NEWS
index 23d95ac78c..ec597b4153 100644
--- a/NEWS
+++ b/NEWS
@@ -27,6 +27,9 @@ PHP NEWS
. Fixed bug #79930 (array_merge_recursive() crashes when called with array
with single reference). (Nikita)
+- XML:
+ . Fixed bug #79922 (Crash after multiple calls to xml_parser_free()). (cmb)
+
06 Aug 2020, PHP 7.3.21
- Apache:
diff --git a/ext/xml/tests/bug79922.phpt b/ext/xml/tests/bug79922.phpt
new file mode 100644
index 0000000000..e578a5d2c4
--- /dev/null
+++ b/ext/xml/tests/bug79922.phpt
@@ -0,0 +1,17 @@
+--TEST--
+Bug #79922 (Crash after multiple calls to xml_parser_free())
+--SKIPIF--
+<?php
+if (!extension_loaded('xml')) die('skip xml extension not available');
+?>
+--FILE--
+<?php
+$c=xml_parser_create_ns();
+$a=xml_parser_free($c);
+$a=xml_parser_free($c);
+$c=0;
+var_dump($a);
+?>
+--EXPECTF--
+Warning: xml_parser_free(): supplied resource is not a valid XML Parser resource in %s on line %d
+bool(false)
diff --git a/ext/xml/xml.c b/ext/xml/xml.c
index 8a5f7797ce..7ca0275595 100644
--- a/ext/xml/xml.c
+++ b/ext/xml/xml.c
@@ -1132,7 +1132,7 @@ static void php_xml_parser_create_impl(INTERNAL_FUNCTION_PARAMETERS, int ns_supp
XML_SetUserData(parser->parser, parser);
RETVAL_RES(zend_register_resource(parser, le_xml_parser));
- ZVAL_COPY(&parser->index, return_value);
+ ZVAL_COPY_VALUE(&parser->index, return_value);
}
/* }}} */
@@ -1559,7 +1559,7 @@ PHP_FUNCTION(xml_parser_free)
RETURN_FALSE;
}
- if (zend_list_delete(Z_RES(parser->index)) == FAILURE) {
+ if (zend_list_close(Z_RES(parser->index)) == FAILURE) {
RETURN_FALSE;
}