Skip to content

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Jan 30, 2020

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.

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.
@cmb69
Copy link
Member Author

cmb69 commented Jan 30, 2020

Ah, almost forgot: there may be an additional fix needed regarding the context switch:

php-src/ext/soap/php_sdl.c

Lines 3302 to 3319 in 494615f

if (context) {
php_stream_context_to_zval(context, &new_context);
php_libxml_switch_context(&new_context, &orig_context);
}
SOAP_GLOBAL(error_code) = "WSDL";
sdl = load_wsdl(this_ptr, uri);
if (sdl) {
sdl->is_persistent = 0;
}
SOAP_GLOBAL(error_code) = old_error_code;
if (context) {
php_libxml_switch_context(&orig_context, NULL);
zval_ptr_dtor(&new_context);
}

If load_wsd() throws (like in this case), we may have to catch that, and restore the context.

@nikic
Copy link
Member

nikic commented Jan 30, 2020

Can you please link the relevant commits?

@cmb69
Copy link
Member Author

cmb69 commented Jan 30, 2020

Bug #71536 has been fixed with commit 93592a9.
Bug #79029 has been partially fixed with commit b5e0043.

@cmb69
Copy link
Member Author

cmb69 commented Jan 30, 2020

The Travis segfault happens only for PHP 7.3, since the EG(active) check is useless there, because the flag is only changed after the objects have been freed, while PHP 7.4 does it the other way round. Any ideas?

@cmb69
Copy link
Member Author

cmb69 commented Feb 2, 2020

To clarify: this PR is fine for PHP-7.4+, but not for PHP-7.3 where it would cause segfaults. Leaving PHP-7.3 as is, would not only mean that bug #79191 would not be fixed there, but also that variants of bug #71536 (which do not rely on unclean shutdown, but rather on late destruction of XMLWriters) would still segfault.

@nikic nikic self-assigned this Feb 3, 2020
@nikic
Copy link
Member

nikic commented Feb 3, 2020

The current test for bug79029 is borked due to overwritten variables:

index 2e76a4e409..b6b0c84b18 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

@nikic
Copy link
Member

nikic commented Feb 3, 2020

After looking into this a bit, I think the proper fix (for master) for various stream resource destruction issues we've been seeing is to make resource destruction a two phase process. First we destroy all non-stream resources and run object dtors, and then we destroy all stream resources that are still live.

For the purposes of this PR, it should be fine if you move the EG(active) = 0 line before the object store freeing on 7.3. free_obj should not need to execute user code.

cmb69 added 2 commits February 3, 2020 13:36
The variables must not be overwritten, because otherwise these parts
won't trigger the issue which happened during shutdown.
@cmb69
Copy link
Member Author

cmb69 commented Feb 3, 2020

I've made the suggested changes.

First we destroy all non-stream resources and run object dtors, and then we destroy all stream resources that are still live.

Sounds like a sensible improvement.

@nikic
Copy link
Member

nikic commented Feb 3, 2020

After thinking about this a bit more, I think this is on the wrong track... Instead, we should move the xmlwriter_free_resource_ptr call into dtor_obj, rather than free_obj, so it runs prior to resource destruction.

@cmb69
Copy link
Member Author

cmb69 commented Feb 3, 2020

After thinking about this a bit more, I think this is on the wrong track... Instead, we should move the xmlwriter_free_resource_ptr call into dtor_obj, rather than free_obj, so it runs prior to resource destruction.

I've though about that as well, but AIUI dtor_obj isn't guaranteed to be called, in which case we'd "leak".

@nikic
Copy link
Member

nikic commented Feb 3, 2020

@cmb69 Well, the current implementation always leaks if the XmlWriter is destroyed during shutdown, because the EG(active) check will be false there. Doing this in dtor_obj would mean we only leak on unclean shutdown (mainly: exit + fatal error). That's not ideal, but better than what we have now.

cmb69 added 2 commits February 3, 2020 17:27
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.
@cmb69
Copy link
Member Author

cmb69 commented Feb 3, 2020

@nikic, ah, of course! I've done that change now.

@cmb69
Copy link
Member Author

cmb69 commented Feb 3, 2020

Applied as fe1bfb7. Thanks!

@cmb69 cmb69 closed this Feb 3, 2020
@cmb69 cmb69 deleted the cmb/79191 branch February 3, 2020 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants