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).
(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.
(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.
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.
(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?
(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.
I think libsoup requires that the session is properly finalized for some reason I don't remember, Dan?
(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.
Created attachment 257757 [details] Patch
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)
(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.
Committed r187586: <http://trac.webkit.org/changeset/187586>
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....