GNOME Bugzilla – Bug 762138
[abrt] Crash under soup_socket_new()
Last modified: 2018-06-20 11:44:57 UTC
Moving this from a downstream bug report: https://bugzilla.redhat.com/show_bug.cgi?id=1306905 Description of problem: Deletet multiple identical contacts in the Owncloud. Version-Release number of selected component: evolution-data-server-3.18.4-1.fc23 Additional info: reporter: libreport-2.6.4 backtrace_rating: 4 cmdline: /usr/libexec/evolution-addressbook-factory-subprocess --factory webdav --bus-name org.gnome.evolution.dataserver.Subprocess.Backend.AddressBookx2702x6 --own-path /org/gnome/evolution/dataserver/Subprocess/Backend/AddressBook/2702/6 crash_function: g_main_context_ref executable: /usr/libexec/evolution-addressbook-factory-subprocess global_pid: 13405 kernel: 4.3.5-300.fc23.x86_64 Core was generated by `/usr/libexec/evolution-addressbook-factory-subprocess --factory webdav --bus-na'. Program terminated with signal SIGSEGV, Segmentation fault.
+ Trace 235976
Thread 19 (Thread 0x7f5fd15be700 (LWP 17315))
Thread 18 (Thread 0x7f5fd1dbf700 (LWP 17314))
Thread 17 (Thread 0x7f5fd4dc5700 (LWP 17308))
Thread 16 (Thread 0x7f5fd45c4700 (LWP 17309))
Thread 15 (Thread 0x7f5fd2dc1700 (LWP 17312))
Thread 14 (Thread 0x7f5fd0dbd700 (LWP 17316))
Thread 13 (Thread 0x7f5fc2ffd700 (LWP 17321))
Thread 12 (Thread 0x7f5fd35c2700 (LWP 17311))
Thread 9 (Thread 0x7f5fc37fe700 (LWP 17319))
Thread 4 (Thread 0x7f5fc3fff700 (LWP 17317))
Thread 3 (Thread 0x7f5fd25c0700 (LWP 17313))
Thread 2 (Thread 0x7f601ee07700 (LWP 17307))
Thread 1 (Thread 0x7f5ff4737700 (LWP 17306))
Created attachment 363687 [details] soup-test.c
The soup-test.c is a reproducer. Save it and then compile it as written at the comment at the very top of the file. Run it multiple times, in case it won't exhibit for the first time. I compiled libsoup and the program with an address sanitizer and this is the output: ==30144==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000029fc at pc 0x7f52c99e2f20 bp 0x7f52c215b540 sp 0x7f52c215b530 READ of size 4 at 0x6060000029fc thread T1 #0 0x7f52c99e2f1f in soup_socket_properties_ref libsoup/soup-socket-properties.c:48 #1 0x7f52c912214b (/lib64/libgobject-2.0.so.0+0xc14b) #2 0x7f52c912c1fa in g_object_new_valist (/lib64/libgobject-2.0.so.0+0x161fa) #3 0x7f52c912c690 in g_object_new (/lib64/libgobject-2.0.so.0+0x16690) #4 0x7f52c99c5e13 in get_connection_for_host libsoup/soup-session.c:1840 #5 0x7f52c99c6d6b in get_connection libsoup/soup-session.c:1897 #6 0x7f52c99c7705 in soup_session_process_queue_item libsoup/soup-session.c:1964 #7 0x7f52c99c85e9 in async_run_queue libsoup/soup-session.c:2064 #8 0x7f52c99c87b1 in idle_run_queue libsoup/soup-session.c:2091 #9 0x7f52c8e49c26 (/lib64/libglib-2.0.so.0+0x46c26) #10 0x7f52c8e4d246 in g_main_context_dispatch (/lib64/libglib-2.0.so.0+0x4a246) #11 0x7f52c8e4d5e7 (/lib64/libglib-2.0.so.0+0x4a5e7) #12 0x7f52c8e4d901 in g_main_loop_run (/lib64/libglib-2.0.so.0+0x4a901) #13 0x400c90 in test_soup_thread soup-test.c:15 #14 0x7f52c8e74535 (/lib64/libglib-2.0.so.0+0x71535) #15 0x7f52c8beb36c in start_thread (/lib64/libpthread.so.0+0x736c) #16 0x7f52c8923bbe in __GI___clone (/lib64/libc.so.6+0x110bbe) 0x6060000029fc is located 60 bytes inside of 64-byte region [0x6060000029c0,0x606000002a00) freed by thread T0 here: #0 0x7f52ca0064b8 in __interceptor_free (/usr/lib64/libasan.so.4+0xde4b8) #1 0x7f52c8e52b4d in g_free (/lib64/libglib-2.0.so.0+0x4fb4d) previously allocated by thread T0 here: #0 0x7f52ca006850 in malloc (/usr/lib64/libasan.so.4+0xde850) #1 0x7f52c8e52a38 in g_malloc (/lib64/libglib-2.0.so.0+0x4fa38) #2 0x7f52c99b9e42 in ensure_socket_props libsoup/soup-session.c:379 #3 0x7f52c99bcc64 in soup_session_set_property libsoup/soup-session.c:776 #4 0x7f52c912c98d in g_object_set_valist (/lib64/libgobject-2.0.so.0+0x1698d) Thread T1 created by T0 here: #0 0x7f52c9f5fa2f in pthread_create (/usr/lib64/libasan.so.4+0x37a2f) #1 0x7f52c8e9228f (/lib64/libglib-2.0.so.0+0x8f28f) SUMMARY: AddressSanitizer: heap-use-after-free libsoup/soup-socket-properties.c:48 in soup_socket_properties_ref
Created attachment 363688 [details] [review] proposed patch The problem is that the SoupSessionPrivate::socket_props can be accessed in multiple threads, especially recreated on some property changes of the SoupSession, while the other thread can still use it. I added conn_lock mutex around these places, which fixed the issue for me.
Do you think it would be worth it to turn the reproducer in a test that can be added to libsoup's tests?
It could be done, but I'm afraid that it might not be part of `make check` or any such, because it tries to reproduce a race condition between threads, which cannot be reproduced reliably (without additional tools like libasan), thus it's supposed to be run several times, which means it's an expensive test. I also saw a case where (without libasan) the test application just deadlocked itself forever, with a runtime warning about failed ref of a main context.
I am not familiar with this much but from all I can see the patch is fine, also taking into account that it seems to (partly) solve the issue in 785110. Dan, would you mind giving it a quick look?
In theory, conn_lock is only for connection/host stuff (see the comment in SoupSessionPrivate). Cheating and reusing conn_lock doesn't really hurt anything here but it's kind of unclean.
Initially, I didn't want to add another mutex to the structure. Afterwards, when I realized that this lock is already held on couple places when the offending private structure member is accessed, I decided to reuse the conn_lock, also to avoid threading issues about proper mutex acquire order. In other words, reuse of the conn_lock simplifies the code and minimizes possible locking issues which would be theoretically introduced when using two locks.
Dan, would updating the SoupSessionPrivate comment and possibly also renaming conn_lock to something less specific address your concerns?
What's the status of this? It would be good to get this solved so that we can safely use the same SoupSession from multiple threads in GStreamer.
I would personally be OK with you updating the patch as per comment 9, if Dan doesn't object.
I can rename the conn_lock, that's a very simple thing to do, but there's also a corresponding 'GCond conn_cond;'. In this case, what would be the right name for it? I would use "property_lock", but then the "property_cond" feels odd. As I mentioned above, the reason to reuse 'conn_lock' is to avoid issues about incorrect mutex acquire order, when two locks are used. Otherwise I really do not care of the actual fix. If this might lead to "add a note in the developer comment above the conn_lock definition", then I think it can be added just before the commit and doesn't worth me to re-add the patch here. I'm also unsure what the changes comment would like like. Say from: /* Must hold the conn_lock before potentially creating a new * SoupSessionHost, adding/removing a connection, * disconnecting a connection, or moving a connection from * IDLE to IN_USE. Must not emit signals or destroy objects * while holding it. conn_cond is signaled when it may be * possible for a previously-blocked message to continue. */ to anything like this? /* Must hold the conn_lock before potentially creating a new * SoupSessionHost, adding/removing a connection, * disconnecting a connection, moving a connection from * IDLE to IN_USE, or when updating socket properties. * Must not emit signals or destroy objects while holding it. * The conn_cond is signaled when it may be possible for * a previously-blocked message to continue. */
I'm fine with the change as long as the comment gets updated
Milan, got time to look at this again, please?
Oh, was this waiting for me? I didn't get an explicit "green" for the comment content, which is basically the only change to be done in the proposed patch. Having this waiting for me just because of it... Okay, okay, I'll re-upload the patch.
Created attachment 369944 [details] [review] proposed patch ][ The same as before, only with changed comment above conn_lock declaration.
Review of attachment 369944 [details] [review]: Let's go for it.
Created commit d28962ff in master (2.62.1+) Created commit caf90b35 in gnome-3-26 (2.60.4+) Created commit 5b80a626 in gnome-3-24 (2.58.3+)
The issue was still persisting even with that patch. It turns out the ref/unref functions weren't MT-safe. I filed the merge-request with the fix : https://gitlab.gnome.org/GNOME/libsoup/merge_requests/7