Bug 145347 – [GTK] Crashes when SoupSession is destroyed in exit handler

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

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).

[Michael Catanzaro]

(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.

[Michael Catanzaro]

(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.

[Michael Catanzaro]

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.

[Anders Carlsson]

(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?

[Michael Catanzaro]

(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.

[Carlos Garcia Campos]

I think libsoup requires that the session is properly finalized for some reason I don't remember, Dan?

[Dan Winship]

(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.

[Michael Catanzaro]

Created attachment 257757 [details]
Patch

[Carlos Garcia Campos]

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)

[Michael Catanzaro]

(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.

[Michael Catanzaro]

Committed r187586: <http://trac.webkit.org/changeset/187586>

[Michael Catanzaro]

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....