GNOME Bugzilla – Bug 722723
Infinite recursion when calling g_io_stream_close_async() from libsoup
Last modified: 2014-08-05 17:58:10 UTC
While working on adding WebSocket support to libsoup I stumbled upon this very interesting backtrace : Program received signal SIGSEGV, Segmentation fault. g_type_check_instance_cast (type_instance=type_instance@entry=0x7fffec00e280, iface_type=6362112) at gtype.c:4000 4000 { (gdb) bt
+ Trace 233064
It turns out that there is a little problem with the current implementation of GIOStream. Libsoup abstracts SSL through its SoupConnection/SoupSocket/SoupIOStream classes. The interesting thing about SoupIOStream is that it doesn't write on any file descriptor itself, it uses a GTcpConnection internally to do that. Now when you call g_io_stream_close_async() on SoupIOStream, it proceeds to call g_io_stream_close_async() on the internal GTcpConnection, and when you do that, you basically give the async_ready_close_callback_wrapper() pointer as callback argument to g_io_stream_close_async() for the GTcpConnection. When doing so, you create the infinite loop above.
Created attachment 266918 [details] [review] gio: iostream: use GTask instead of internal pointer
I should mentioned that I checked I didn't break any test in gio.
Comment on attachment 266918 [details] [review] gio: iostream: use GTask instead of internal pointer ok, right idea, but wrong in some details >@@ -416,12 +415,22 @@ async_ready_close_callback_wrapper (GObject *source_object, >+ success = klass->close_finish (stream, res, &error); You need to move the g_async_result_legacy_propagate_error() check from g_io_stream_close_async() here (because legacy GSimpleAsyncResult-based GIOStream implementations may expect that their close_finish() is only called for successful results, not error results). >@@ -454,13 +463,13 @@ g_io_stream_close_async (GIOStream *stream, > if (stream->priv->closed) > { >- GTask *task; >- > task = g_task_new (stream, cancellable, callback, user_data); > g_task_set_source_tag (task, g_io_stream_close_async); You can remove the set_source_tag() here... >@@ -511,8 +519,7 @@ g_io_stream_close_finish (GIOStream *stream, > else if (g_async_result_is_tagged (result, g_io_stream_close_async)) > return g_task_propagate_boolean (G_TASK (result), error); and the is_tagged() check here; with your changes, g_io_stream_close_finish() will always be receiving a GTask that was created by g_io_stream_close_async(), so all it needs to do is return g_task_propagate_boolean(). (You can also reorganize g_io_stream_close_async() to just always create the GTask at the beginning, rather than creating it in two different places, and use g_task_return_error() rather than g_task_report_error() in that error case.)
Created attachment 266988 [details] [review] gio: iostream: use GTask instead of internal pointer
I can't see any reason to even use g_async_result_legacy_propagate_error() given that the AsyncResult should always be GIOStream's own GTask now.
There are two GAsyncResults. g_io_stream_close_async() creates a GTask, and then passes that as the user_data to the subclass's ->close_async() method. The subclass ->close_async() then creates its own GAsyncResult, does an async close, and then passes that GAsyncResult to its callback (which in this case is async_ready_close_callback_wrapper()). So async_ready_close_callback_wrapper() receives the subclass's GAsyncResult as "res" (and g_io_stream_close_async's GTask as "user_data"). It passes "res" to the subclass's ->close_finish(), which extracts and returns the result, and then async_ready_close_callback_wrapper() copies that result to the GTask. EXCEPT... that if "res" is a GSimpleAsyncResult, then we have to handle it differently, because in the old days, people wrote finish() implementations that expected that the wrapper function would handle errors for them. (eg, https://git.gnome.org/browse/glib/tree/gio/gsocketconnection.c?h=glib-2-32#n507, which depends on the fact that g_io_stream_close_finish() will not call it if its close_async() returned an error). So, we have to check for this legacy case, and not call ->close_finish() if so.
Just hit that bug again with a different implementation of WebSockets. I'm not sure how you actually want this bug to be fixed, any comment?
Created attachment 275693 [details] [review] gio: iostream: use GTask instead of internal pointer does this still fix things for you?
Yes, it does. Thanks a lot.
Attachment 275693 [details] pushed as 1565a49 - gio: iostream: use GTask instead of internal pointer
Any plan to cherry-pick this in the 2.40 branch?