From 07d676e26a6bdb7b61ff09d5b787032b3ee5ffe6 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Thu, 30 Jan 2020 12:19:24 +0100 Subject: [PATCH 1/6] Fix #79191: Error in SoapClient ctor disables DOMDocument::save() The culprit is the too restrictive fix for bug #71536, which prevents `php_libxml_streams_IO_write()` from properly executing when unclean shutdown is flagged. However, the fix for bug #79029 already prevents that call during shutdown in the first place, so that it actually fixes bug #71536 as well. Thus, we can just revert the original fix for bug #71536. Thanks to bwoebi and daverandom for helping to investigate this issue. --- ext/libxml/libxml.c | 3 --- ext/libxml/tests/bug79191.phpt | 24 ++++++++++++++++++++++++ ext/xmlwriter/tests/bug71536.phpt | 24 ++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) create mode 100644 ext/libxml/tests/bug79191.phpt create mode 100644 ext/xmlwriter/tests/bug71536.phpt diff --git a/ext/libxml/libxml.c b/ext/libxml/libxml.c index 864e5a36fb72a..2be6a5b47a06d 100644 --- a/ext/libxml/libxml.c +++ b/ext/libxml/libxml.c @@ -385,9 +385,6 @@ static int php_libxml_streams_IO_read(void *context, char *buffer, int len) static int php_libxml_streams_IO_write(void *context, const char *buffer, int len) { - if (CG(unclean_shutdown)) { - return -1; - } return php_stream_write((php_stream*)context, buffer, len); } diff --git a/ext/libxml/tests/bug79191.phpt b/ext/libxml/tests/bug79191.phpt new file mode 100644 index 0000000000000..1074d01f498fc --- /dev/null +++ b/ext/libxml/tests/bug79191.phpt @@ -0,0 +1,24 @@ +--TEST-- +Bug #79191 (Error in SoapClient ctor disables DOMDocument::save()) +--SKIPIF-- + +--FILE-- +loadxml(''); +var_dump($dom->save(__DIR__ . '/bug79191.xml')); +?> +--EXPECTF-- +int(%d) +--CLEAN-- + diff --git a/ext/xmlwriter/tests/bug71536.phpt b/ext/xmlwriter/tests/bug71536.phpt new file mode 100644 index 0000000000000..4ce2f2e404ac3 --- /dev/null +++ b/ext/xmlwriter/tests/bug71536.phpt @@ -0,0 +1,24 @@ +--TEST-- +Bug #71536 (Access Violation crashes php-cgi.exe) +--SKIPIF-- + +--FILE-- +openUri('php://memory'); + $xml->setIndent(false); + $xml->startDocument('1.0', 'UTF-8'); + $xml->startElement('response'); + die('now'); // crashed with die() + } +} + +Test::init(); +?> +--EXPECT-- +now From 0e80c83149c99964c85e5ac8175e8b50cfd77941 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Thu, 30 Jan 2020 13:49:10 +0100 Subject: [PATCH 2/6] Fix minor issues in test case --- ext/libxml/tests/bug79191.phpt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ext/libxml/tests/bug79191.phpt b/ext/libxml/tests/bug79191.phpt index 1074d01f498fc..7d0dc83f232a0 100644 --- a/ext/libxml/tests/bug79191.phpt +++ b/ext/libxml/tests/bug79191.phpt @@ -10,15 +10,15 @@ if (!extension_loaded('dom')) die('dom extension not available'); try { new \SoapClient('does-not-exist.wsdl'); } catch (Throwable $t) { -}# +} $dom = new DOMDocument; $dom->loadxml(''); var_dump($dom->save(__DIR__ . '/bug79191.xml')); ?> ---EXPECTF-- -int(%d) --CLEAN-- +--EXPECTF-- +int(%d) From 44363ff06ad8634d7b35d6c342ba7e310f658220 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 3 Feb 2020 13:36:21 +0100 Subject: [PATCH 3/6] Unset EG(active) flag before freeing the object storage --- Zend/zend_execute_API.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c index 6eb9dfacee27c..c7384217295cc 100644 --- a/Zend/zend_execute_API.c +++ b/Zend/zend_execute_API.c @@ -265,12 +265,12 @@ void shutdown_executor(void) /* {{{ */ zend_close_rsrc_list(&EG(regular_list)); } zend_end_try(); - zend_objects_store_free_object_storage(&EG(objects_store), fast_shutdown); - - /* All resources and objects are destroyed. */ + /* All resources are destroyed. */ /* No PHP callback functions may be called after this point. */ EG(active) = 0; + zend_objects_store_free_object_storage(&EG(objects_store), fast_shutdown); + zend_try { zend_llist_apply(&zend_extensions, (llist_apply_func_t) zend_extension_deactivator); } zend_end_try(); From e345d1c47be329dbb62497cfdff4f6688b1440a5 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 3 Feb 2020 13:40:19 +0100 Subject: [PATCH 4/6] Fix test case The variables must not be overwritten, because otherwise these parts won't trigger the issue which happened during shutdown. --- ext/xmlwriter/tests/bug79029.phpt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/xmlwriter/tests/bug79029.phpt b/ext/xmlwriter/tests/bug79029.phpt index 2e76a4e409574..b6b0c84b182d0 100644 --- a/ext/xmlwriter/tests/bug79029.phpt +++ b/ext/xmlwriter/tests/bug79029.phpt @@ -11,13 +11,13 @@ $x = array( new XMLWriter() ); $x[0]->openUri("bug79029_1.txt"); $x[0]->startComment(); -$x = new XMLWriter(); -$x->openUri("bug79029_2.txt"); +$y = new XMLWriter(); +$y->openUri("bug79029_2.txt"); fclose(@end(get_resources())); file_put_contents("bug79029_3.txt", "a"); -$x = new XMLReader(); -$x->open("bug79029_3.txt"); +$z = new XMLReader(); +$z->open("bug79029_3.txt"); fclose(@end(get_resources())); ?> okey From 55699ab53a048e26a9ab04ae256d8d5fa3d7f68b Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 3 Feb 2020 17:27:43 +0100 Subject: [PATCH 5/6] Use `dtor_obj` handler instead of fiddling with EG(active) We're moving the `xmlwriter_free_resource_ptr()` call from the `free_obj` handler to an added `dtor_obj` handler, to avoid to write to a closed stream in case of late object freeing. Since some of the earlier changes are no longer needed now, we revert them. --- Zend/zend_execute_API.c | 6 +++--- ext/xmlwriter/php_xmlwriter.c | 32 +++++++++++++++++++++----------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/Zend/zend_execute_API.c b/Zend/zend_execute_API.c index c7384217295cc..6eb9dfacee27c 100644 --- a/Zend/zend_execute_API.c +++ b/Zend/zend_execute_API.c @@ -265,12 +265,12 @@ void shutdown_executor(void) /* {{{ */ zend_close_rsrc_list(&EG(regular_list)); } zend_end_try(); - /* All resources are destroyed. */ + zend_objects_store_free_object_storage(&EG(objects_store), fast_shutdown); + + /* All resources and objects are destroyed. */ /* No PHP callback functions may be called after this point. */ EG(active) = 0; - zend_objects_store_free_object_storage(&EG(objects_store), fast_shutdown); - zend_try { zend_llist_apply(&zend_extensions, (llist_apply_func_t) zend_extension_deactivator); } zend_end_try(); diff --git a/ext/xmlwriter/php_xmlwriter.c b/ext/xmlwriter/php_xmlwriter.c index 24bb9dd1829dc..fa6f9513df6d8 100644 --- a/ext/xmlwriter/php_xmlwriter.c +++ b/ext/xmlwriter/php_xmlwriter.c @@ -91,15 +91,13 @@ typedef int (*xmlwriter_read_int_t)(xmlTextWriterPtr writer); static void xmlwriter_free_resource_ptr(xmlwriter_object *intern) { if (intern) { - if (EG(active)) { - if (intern->ptr) { - xmlFreeTextWriter(intern->ptr); - intern->ptr = NULL; - } - if (intern->output) { - xmlBufferFree(intern->output); - intern->output = NULL; - } + if (intern->ptr) { + xmlFreeTextWriter(intern->ptr); + intern->ptr = NULL; + } + if (intern->output) { + xmlBufferFree(intern->output); + intern->output = NULL; } efree(intern); } @@ -120,17 +118,28 @@ static void xmlwriter_free_resource_ptr(xmlwriter_object *intern) static zend_object_handlers xmlwriter_object_handlers; -/* {{{ xmlwriter_object_free_storage */ -static void xmlwriter_object_free_storage(zend_object *object) +/* {{{{ xmlwriter_object_dtor */ +static void xmlwriter_object_dtor(zend_object *object) { ze_xmlwriter_object *intern = php_xmlwriter_fetch_object(object); if (!intern) { return; } + /* freeing the resource here may leak, but otherwise we may use it after it has been freed */ if (intern->xmlwriter_ptr) { xmlwriter_free_resource_ptr(intern->xmlwriter_ptr); } intern->xmlwriter_ptr = NULL; +} +/* }}} */ + +/* {{{ xmlwriter_object_free_storage */ +static void xmlwriter_object_free_storage(zend_object *object) +{ + ze_xmlwriter_object *intern = php_xmlwriter_fetch_object(object); + if (!intern) { + return; + } zend_object_std_dtor(&intern->std); } /* }}} */ @@ -1844,6 +1853,7 @@ static PHP_MINIT_FUNCTION(xmlwriter) memcpy(&xmlwriter_object_handlers, &std_object_handlers, sizeof(zend_object_handlers)); xmlwriter_object_handlers.offset = XtOffsetOf(ze_xmlwriter_object, std); + xmlwriter_object_handlers.dtor_obj = xmlwriter_object_dtor; xmlwriter_object_handlers.free_obj = xmlwriter_object_free_storage; xmlwriter_object_handlers.clone_obj = NULL; INIT_CLASS_ENTRY(ce, "XMLWriter", xmlwriter_class_functions); From aa57fe9fa6ad374bf02e4688c964ff54b2858455 Mon Sep 17 00:00:00 2001 From: "Christoph M. Becker" Date: Mon, 3 Feb 2020 17:31:38 +0100 Subject: [PATCH 6/6] Actually destroy the object --- ext/xmlwriter/php_xmlwriter.c | 1 + 1 file changed, 1 insertion(+) diff --git a/ext/xmlwriter/php_xmlwriter.c b/ext/xmlwriter/php_xmlwriter.c index fa6f9513df6d8..0fb823199d2bf 100644 --- a/ext/xmlwriter/php_xmlwriter.c +++ b/ext/xmlwriter/php_xmlwriter.c @@ -130,6 +130,7 @@ static void xmlwriter_object_dtor(zend_object *object) xmlwriter_free_resource_ptr(intern->xmlwriter_ptr); } intern->xmlwriter_ptr = NULL; + zend_objects_destroy_object(object); } /* }}} */