After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 722723 - Infinite recursion when calling g_io_stream_close_async() from libsoup
Infinite recursion when calling g_io_stream_close_async() from libsoup
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
2.39.x
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2014-01-21 19:36 UTC by Lionel Landwerlin
Modified: 2014-08-05 17:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gio: iostream: use GTask instead of internal pointer (3.07 KB, patch)
2014-01-21 19:37 UTC, Lionel Landwerlin
needs-work Details | Review
gio: iostream: use GTask instead of internal pointer (3.13 KB, patch)
2014-01-22 17:14 UTC, Lionel Landwerlin
none Details | Review
gio: iostream: use GTask instead of internal pointer (3.57 KB, patch)
2014-05-02 19:50 UTC, Dan Winship
committed Details | Review

Description Lionel Landwerlin 2014-01-21 19:36:08 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
  • #0 g_type_check_instance_cast
    at gtype.c line 4000
  • #1 async_ready_close_callback_wrapper
    at giostream.c line 418
  • #2 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #3 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #4 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #5 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #6 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #7 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #8 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #9 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #10 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #11 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #12 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #13 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #14 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #15 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #16 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #17 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #18 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #19 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #20 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #21 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #22 async_ready_close_callback_wrapper
    at giostream.c line 423
  • #23 async_ready_close_callback_wrapper
    at giostream.c line 423



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.
Comment 1 Lionel Landwerlin 2014-01-21 19:37:44 UTC
Created attachment 266918 [details] [review]
gio: iostream: use GTask instead of internal pointer
Comment 2 Lionel Landwerlin 2014-01-21 19:55:49 UTC
I should mentioned that I checked I didn't break any test in gio.
Comment 3 Dan Winship 2014-01-22 16:45:05 UTC
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.)
Comment 4 Lionel Landwerlin 2014-01-22 17:14:29 UTC
Created attachment 266988 [details] [review]
gio: iostream: use GTask instead of internal pointer
Comment 5 Lionel Landwerlin 2014-01-22 17:17:45 UTC
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.
Comment 6 Dan Winship 2014-01-22 17:57:27 UTC
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.
Comment 7 Lionel Landwerlin 2014-05-02 16:32:43 UTC
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?
Comment 8 Dan Winship 2014-05-02 19:50:45 UTC
Created attachment 275693 [details] [review]
gio: iostream: use GTask instead of internal pointer

does this still fix things for you?
Comment 9 Lionel Landwerlin 2014-05-04 15:08:01 UTC
Yes, it does. Thanks a lot.
Comment 10 Dan Winship 2014-05-05 15:10:24 UTC
Attachment 275693 [details] pushed as 1565a49 - gio: iostream: use GTask instead of internal pointer
Comment 11 Lionel Landwerlin 2014-08-05 17:58:10 UTC
Any plan to cherry-pick this in the 2.40 branch?