Bug 145347 - [GTK] Crashes when SoupSession is destroyed in exit handler
Summary: [GTK] Crashes when SoupSession is destroyed in exit handler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-23 10:47 PDT by Michael Catanzaro
Modified: 2015-08-06 13:12 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2015-07-29 10:56 PDT, Michael Catanzaro
cgarcia: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-05-23 10:47:05 PDT
We have a couple downstream reports of crashes that occur when g_object_unref destroys a SoupSession in an exit handler. E.g.

Truncated backtrace:
Thread no. 1 (10 frames)
 #0 free at /lib64/libc.so.6
 #1 _gnutls_free_dh_info at /lib64/libgnutls.so.28
 #2 _gnutls_free_auth_info at /lib64/libgnutls.so.28
 #3 gnutls_deinit at /lib64/libgnutls.so.28
 #4 g_tls_connection_gnutls_finalize at /usr/lib64/gio/modules/libgiognutls.so
 #6 soup_io_stream_finalize at /lib64/libsoup-2.4.so.1
 #8 soup_socket_finalize at /lib64/libsoup-2.4.so.1
 #10 soup_connection_disconnect at /lib64/libsoup-2.4.so.1
 #11 soup_session_abort at /lib64/libsoup-2.4.so.1
 #12 soup_session_dispose at /lib64/libsoup-2.4.so.1

(Truncated, but soup_session_dispose is called by g_object_unref is called by __run_exit_handlers.)

I spent some time checking libsoup, WebKit, Epiphany, and even GLib for all uses of atexit and on_exit, but didn't find anything suspicious. So my suspicion falls on SoupNetworkSession::defaultSession(). I guess that will be destroyed in an exit handler because it is static? Maybe we should leak it using NeverDestroyed<SoupNetworkSession> instead? This is kind of wild speculation, but it seems reasonable....

We have another backtrace where __run_exit_handlers calls g_object_unref calls soup_session_dispose (as in the backtrace above), then soup_session_dispose calls g_source_destroy calls g_source_destroy_internal calls g_mutex_lock, then boom. That surely is happening because the mutex is already locked, so it's some thread-safety issue for sure.

Full backtraces downstream (in the See Also field).
Comment 1 Michael Catanzaro 2015-05-23 10:52:28 PDT
(In reply to comment #0)
> Maybe we should leak
> it using NeverDestroyed<SoupNetworkSession> instead?

Maybe that is a terrible idea, since I didn't check to see what all SoupSession does when it's destroyed, since we'd be relying on it never doing anything important even if it doesn't currently, and since I haven't even established what the issue is.
Comment 2 Michael Catanzaro 2015-07-28 16:49:32 PDT
(In reply to comment #0) 
> I spent some time checking libsoup, WebKit, Epiphany, and even GLib for all
> uses of atexit and on_exit, but didn't find anything suspicious. So my
> suspicion falls on SoupNetworkSession::defaultSession(). I guess that will
> be destroyed in an exit handler because it is static? Maybe we should leak
> it using NeverDestroyed<SoupNetworkSession> instead? This is kind of wild
> speculation, but it seems reasonable....

Yes, static variables are destroyed in exit handlers. So that must be where this crash is coming from.
Comment 3 Michael Catanzaro 2015-07-28 16:52:37 PDT
So if we don't destroy the SoupSession, surely that could result in us leaving a TLS session open forever. That would annoy the server.
Comment 4 Anders Carlsson 2015-07-28 17:03:40 PDT
(In reply to comment #3)
> So if we don't destroy the SoupSession, surely that could result in us
> leaving a TLS session open forever. That would annoy the server.

Doesn't Linux clean up sockets when the process exits?
Comment 5 Michael Catanzaro 2015-07-28 19:24:30 PDT
(In reply to comment #4)
> Doesn't Linux clean up sockets when the process exits?

Of course. So it shouldn't matter, you're right.
Comment 6 Carlos Garcia Campos 2015-07-28 22:45:33 PDT
I think libsoup requires that the session is properly finalized for some reason I don't remember, Dan?
Comment 7 Dan Winship 2015-07-29 03:43:00 PDT
(In reply to comment #6)
> I think libsoup requires that the session is properly finalized for some
> reason I don't remember, Dan?

Nope. If you're trying to find leaks with valgrind you need to destroy everything, but otherwise it doesn't matter more for SoupSession than for anything else.
Comment 8 Michael Catanzaro 2015-07-29 10:56:27 PDT
Created attachment 257757 [details]
Patch
Comment 9 Carlos Garcia Campos 2015-07-29 23:03:13 PDT
Comment on attachment 257757 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=257757&action=review

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:40
> +#include <wtf/NeverDestroyed.h>

I'm surprised you don't need to include this in the header for the friend class NeverDestroyed<SoupNetworkSession>;

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:55
> +    // There are thread-safety issues involved in the destruction of the SoupSession, bug #145347.

Do not add this comment, using NeverDestroyed is the common thing for a singleton implementation like this in WebKit, the current code was the exception, because I thought we had to properly cleanup the soup session, as we also used to do in wk1 (using an ugly atexit handler)
Comment 10 Michael Catanzaro 2015-07-30 09:16:49 PDT
(In reply to comment #9)
> I'm surprised you don't need to include this in the header for the friend
> class NeverDestroyed<SoupNetworkSession>;

You can friend classes that don't exist, so it's pointless to include the header. I am not sure if that is a language feature or a language defect, but it is what it is....

> > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:55
> > +    // There are thread-safety issues involved in the destruction of the SoupSession, bug #145347.
> 
> Do not add this comment, using NeverDestroyed is the common thing for a
> singleton implementation like this in WebKit, the current code was the
> exception, because I thought we had to properly cleanup the soup session, as
> we also used to do in wk1 (using an ugly atexit handler)

OK.
Comment 11 Michael Catanzaro 2015-07-30 09:21:51 PDT
Committed r187586: <http://trac.webkit.org/changeset/187586>
Comment 12 Michael Catanzaro 2015-08-06 13:12:16 PDT
Note that if the TLS session is not closed cleanly, a well-behaved server would detect a truncation attack. I think in practice, this should not be a problem for any real server, but it's not ideal....