145347 2015-05-23 10:47:05 -0700 [GTK] Crashes when SoupSession is destroyed in exit handler 2015-08-06 13:12:16 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) PC Linux RESOLVED FIXED https://bugzilla.redhat.com/show_bug.cgi?id=1208872 https://bugzilla.redhat.com/show_bug.cgi?id=1222802 P2 Normal --- 1 mcatanzaro mcatanzaro andersca cgarcia danw mcatanzaro oldest_to_newest 1097103 0 mcatanzaro 2015-05-23 10:47:05 -0700 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). 1097104 1 mcatanzaro 2015-05-23 10:52:28 -0700 (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. 1112930 2 mcatanzaro 2015-07-28 16:49:32 -0700 (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. 1112933 3 mcatanzaro 2015-07-28 16:52:37 -0700 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. 1112939 4 andersca 2015-07-28 17:03:40 -0700 (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? 1113008 5 mcatanzaro 2015-07-28 19:24:30 -0700 (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. 1113039 6 cgarcia 2015-07-28 22:45:33 -0700 I think libsoup requires that the session is properly finalized for some reason I don't remember, Dan? 1113067 7 danw 2015-07-29 03:43:00 -0700 (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. 1113113 8 257757 mcatanzaro 2015-07-29 10:56:27 -0700 Created attachment 257757 Patch 1113316 9 257757 cgarcia 2015-07-29 23:03:13 -0700 Comment on attachment 257757 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) 1113380 10 mcatanzaro 2015-07-30 09:16:49 -0700 (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. 1113382 11 mcatanzaro 2015-07-30 09:21:51 -0700 Committed r187586: <http://trac.webkit.org/changeset/187586> 1115419 12 mcatanzaro 2015-08-06 13:12:16 -0700 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.... 257757 2015-07-29 10:56:27 -0700 2015-07-29 23:03:13 -0700 Patch bug-145347-20150729125504.patch text/plain 2493 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMTg3NTQ4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTEyNmUyMDhiZDAyNzQ4 OWI4MDMzMWU1YjU1MzE3OTNmZWEwZDViZi4uMTlkNjQyZGI2ODQyYjFlNDEyNDc1MjM3NmE0OGU1 NDgyZWE5YTBlMSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4IEBACisyMDE1LTA3LTI5ICBNaWNo YWVsIENhdGFuemFybyAgPG1jYXRhbnphcm9AaWdhbGlhLmNvbT4KKworICAgICAgICBbR1RLXSBD cmFzaGVzIHdoZW4gU291cFNlc3Npb24gaXMgZGVzdHJveWVkIGluIGV4aXQgaGFuZGxlcgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ1MzQ3CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTGVhayB0aGUgZGVm YXVsdCBTb3VwU2Vzc2lvbiB3aXRoIE5ldmVyRGVzdHJveWVkIHRvIGF2b2lkIHJhY2VzIGF0IHBy b2dyYW0gZXhpdC4KKworICAgICAgICAqIHBsYXRmb3JtL25ldHdvcmsvc291cC9Tb3VwTmV0d29y a1Nlc3Npb24uY3BwOgorICAgICAgICAoV2ViQ29yZTo6U291cE5ldHdvcmtTZXNzaW9uOjpkZWZh dWx0U2Vzc2lvbik6CisgICAgICAgICogcGxhdGZvcm0vbmV0d29yay9zb3VwL1NvdXBOZXR3b3Jr U2Vzc2lvbi5oOgorCiAyMDE1LTA3LTI5ICBNaWNoYWVsIENhdGFuemFybyAgPG1jYXRhbnphcm9A aWdhbGlhLmNvbT4KIAogICAgICAgICBDbGVhbiB1cCBSZWZQdHJDYWlyby5jcHAKICAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0NzM4NAogCmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvU291cE5ldHdvcmtTZXNz aW9uLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9Tb3VwTmV0d29y a1Nlc3Npb24uY3BwCmluZGV4IDI1ZWNiZDkxMDc4M2VmZWNkZDFhZGFjMTJkZjFhMDhjMThhMTAw NGQuLjRhM2M1NjhmOTRmNjAwY2ExOTRkYzJlMWMxMTg5YzUwNjRiMmI1YzMgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9Tb3VwTmV0d29ya1Nlc3Npb24u Y3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL25ldHdvcmsvc291cC9Tb3VwTmV0d29y a1Nlc3Npb24uY3BwCkBAIC0zNyw2ICszNyw3IEBACiAjaW5jbHVkZSAiUmVzb3VyY2VIYW5kbGUu aCIKICNpbmNsdWRlIDxnbGliL2dzdGRpby5oPgogI2luY2x1ZGUgPGxpYnNvdXAvc291cC5oPgor I2luY2x1ZGUgPHd0Zi9OZXZlckRlc3Ryb3llZC5oPgogI2luY2x1ZGUgPHd0Zi90ZXh0L0NTdHJp bmcuaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9TdHJpbmdCdWlsZGVyLmg+CiAKQEAgLTUxLDcgKzUy LDggQEAgaW5saW5lIHN0YXRpYyB2b2lkIHNvdXBMb2dQcmludGVyKFNvdXBMb2dnZXIqLCBTb3Vw TG9nZ2VyTG9nTGV2ZWwsIGNoYXIgZGlyZWN0aW8KIAogU291cE5ldHdvcmtTZXNzaW9uJiBTb3Vw TmV0d29ya1Nlc3Npb246OmRlZmF1bHRTZXNzaW9uKCkKIHsKLSAgICBzdGF0aWMgU291cE5ldHdv cmtTZXNzaW9uIG5ldHdvcmtTZXNzaW9uKHNvdXBDb29raWVKYXIoKSk7CisgICAgLy8gVGhlcmUg YXJlIHRocmVhZC1zYWZldHkgaXNzdWVzIGludm9sdmVkIGluIHRoZSBkZXN0cnVjdGlvbiBvZiB0 aGUgU291cFNlc3Npb24sIGJ1ZyAjMTQ1MzQ3LgorICAgIHN0YXRpYyBOZXZlckRlc3Ryb3llZDxT b3VwTmV0d29ya1Nlc3Npb24+IG5ldHdvcmtTZXNzaW9uKHNvdXBDb29raWVKYXIoKSk7CiAgICAg cmV0dXJuIG5ldHdvcmtTZXNzaW9uOwogfQogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9uZXR3b3JrL3NvdXAvU291cE5ldHdvcmtTZXNzaW9uLmggYi9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9uZXR3b3JrL3NvdXAvU291cE5ldHdvcmtTZXNzaW9uLmgKaW5kZXggOWM4Y2Zh YTYxNmE3NGJjNGUwNzM3N2NlY2FiYTgyNTA1NGRiM2Q4ZS4uYWZhNGNkZTFhOTIyMTM3NzdhYTc3 NjA3ODBmNDA3YWI2YTE0ODA3YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0v bmV0d29yay9zb3VwL1NvdXBOZXR3b3JrU2Vzc2lvbi5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL25ldHdvcmsvc291cC9Tb3VwTmV0d29ya1Nlc3Npb24uaApAQCAtNzAsNiArNzAsOCBA QCBwdWJsaWM6CiAgICAgdm9pZCBzZXRBY2NlcHRMYW5ndWFnZXMoY29uc3QgVmVjdG9yPFN0cmlu Zz4mKTsKIAogcHJpdmF0ZToKKyAgICBmcmllbmQgY2xhc3MgTmV2ZXJEZXN0cm95ZWQ8U291cE5l dHdvcmtTZXNzaW9uPjsKKwogICAgIFNvdXBOZXR3b3JrU2Vzc2lvbihTb3VwQ29va2llSmFyKik7 CiAgICAgU291cE5ldHdvcmtTZXNzaW9uKFNvdXBTZXNzaW9uKik7CiAK