GNOME Bugzilla – Bug 739575
backend crashed with SIGABRT when locked mutex is cleared
Last modified: 2014-11-06 10:46:58 UTC
g_mutex_clear in g_vfs_backend_mtp_finalize causes SIGABRT after g_vfs_backend_unmount_force is called and there is already running job (with locked mutex). See: https://bugzilla.redhat.com/show_bug.cgi?id=1159610
Hard to say how to fix it gracefully, maybe we should just leak it for the case of force unmount to avoid those crashes... Philip what are you thinking about?
Just to capture the discussion: 1) The problem is that there's an outstanding job running at the time the plug is pulled. 2) This triggers the forced unmount, which asynchronously requests the backend be terminated. 3) The job continues to hang around, probably making no forward progress 4) The backend termination reaches the finalize call while the job still hold the mutex. 5) Boom. I agree with Ondrej that there's no clean way to avoid this, except to leak the mutex in finalize - which is fine in practice.
Created attachment 289926 [details] [review] mtp: do not crash when device is unplugged
Comment on attachment 289926 [details] [review] mtp: do not crash when device is unplugged Thanks!
It's also possible that a backend is force unmounted by the user. In that case, I suppose the unmount method would hang trying to lock the mutex... but I'm not sure if the finalize method would still run eventually when (if) the backend exits.
Created attachment 289974 [details] [review] mtp: avoid crash when backend is force unmounted This could probably happens, because mtp backend doesn't limit number of threads, however I'm unable to reproduce it currently. But attached patch should fix it I hope... Mtp backend doesn't limit number of threads, however all methods are executed with locked mutex thus multiple threads isn't needed. Do not waste resources and limit maximal number of threads to one. This change should also avoid executing finalize with locked mutex (which leads to crash) when backend is force unmounted by user.
Comment on attachment 289926 [details] [review] mtp: do not crash when device is unplugged commit 37727a10ee377d5675413c9d72fee4b9273c60b2
Comment on attachment 289926 [details] [review] mtp: do not crash when device is unplugged Also pushed for gnome-3-14: commit b34875f78f7cb314197e1deef85b426eb89e2bbe
So I've made some more testing. Finalize method is called after unmount method is finished and unmount method is executed last one thanks to the mutexes (and blocking new jobs when mount operation started), so the second patch isn't needed, however it could be useful from other reasons (e.g. wasting resources)...
Review of attachment 289974 [details] [review]: Seems reasonable to do as it reflects reality. I'd have done it to begin with if I knew it was a setting :-)
Review of attachment 289974 [details] [review]: If you do this, then you may as well save some more resources and remove the mutex, the unmount_started flag, and the recently added force_unmounted flag since all methods execute sequentially.
MAX_JOB_THREADS affects only maximal number of threads in a thread pool with jobs. So only jobs are executed sequentially. I don't think we can remove the unmount_started flag and the mutex, because check_event is running in a separate thread, see: GThread *event_thread = g_thread_new ("events", check_event, event_ref); I don't think we can remove the recently added force_unmounted flag either, because on_uevent is also called on thread and finalize could be executed before current job is finished...
(In reply to comment #12) > MAX_JOB_THREADS affects only maximal number of threads in a thread pool with > jobs. So only jobs are executed sequentially. > > I don't think we can remove the unmount_started flag and the mutex, because > check_event is running in a separate thread, see: > > GThread *event_thread = g_thread_new ("events", check_event, event_ref); Ah, I hadn't seen this extra thread. > > I don't think we can remove the recently added force_unmounted flag either, > because on_uevent is also called on thread and finalize could be executed > before current job is finished... Yeah, I suppose on_uevent is executed from the daemon's main main loop which could run in parallel with a job. Go ahead and commit it :-)
Comment on attachment 289974 [details] [review] mtp: avoid crash when backend is force unmounted Committed to master with modified commit message... commit 14032ee48bab618e9e7c75134f1c653e996bc4dd
Thanks for cooperation to fix this bug :-)