GNOME Bugzilla – Bug 762250
fuse daemon crashes when exiting
Last modified: 2016-03-11 07:35:37 UTC
I realized that race is possible when fuse daemon is exiting. It might cause segfaults. It happens rarely, but it would be nice to fix it. Fuse daemon may exits in two ways, but only one seems to be handled correctly: - The correct one is when dbus connection is closed, or main daemon disappeared from dbus. Subthread main loop quits, subthread resources are fried, and finally daemon is killed, which invokes vfs_destroy... - Another way is when vfs_destroy is called directly and subthread is still running. It may happen if fuse mountpoint is unmounted, or daemon is killed. There is race with subthread and might cause following crashes: https://retrace.fedoraproject.org/faf/problems/?daterange=2012-02-17%3A2016-02-17&function_names=vfs_destroy
Created attachment 321569 [details] [review] fuse: Do not unref result of g_vfs_get_default Result from g_vfs_get_default shouldn't be freed as per documentation.
Created attachment 321570 [details] [review] fuse: Avoid crashes when exiting (draft) There is attempt to fix the race. Call g_main_loop_quit in subthread and wait for thread termination. I suppose that this patch will fix the crashes, however I realized that there is also another problem in the code... If vfs_destroy is called directly, main loop quits and kill command in subthread_main() terminates the daemon immediately. This is probably better then segfault, however it is not really clean. I can set some flag in vfs_destroy to not call kill from subthread, but still there might be race. Don't you have an idea, how to fix it gracefully?
Created attachment 321575 [details] [review] fuse: Do not kill itself when exiting (draft) It seems to me that this is the best what we can do to reduce the risk of killing itself, however there is still small chance for race... or do you see better approach?
Review of attachment 321569 [details] [review]: ack
I don't think you can realistically fix the race when there are two ways to initiate shutdown like that. Instead i think the proper solution is to always shut down the same way, and since kill is called from the outside that is the one we need to use. So, in name_vanished_handler, and dbus_connection_close we call kill(). And then in vfs_destroy we call g_main_loop_quit (subthread_main_loop), which btw is threadsafe, so no need for the idle stuff. Then we join the threads. There is some chance that the kill signal will be sent twice. For instance if the gvfs daemon dirs, and then the dbus daemon dies, then the subthread calls kill twice. Another way is for the subthread to race with an externally sent kill signal. However, the second time this happen we will be already handling the kill signal on the main thread, so that signal is blocked and will not be delivered until the first signal handler returned.
putting crashes on the target list for now
(In reply to Alexander Larsson from comment #5) > I don't think you can realistically fix the race when there are two ways to > initiate shutdown like that. > > Instead i think the proper solution is to always shut down the same way, and > since kill is called from the outside that is the one we need to use. So, in > name_vanished_handler, and dbus_connection_close we call kill(). And then in > vfs_destroy we call g_main_loop_quit (subthread_main_loop), which btw is > threadsafe, so no need for the idle stuff. Then we join the threads. This is probably better solution, but there is still same problem. Because I realized that fuse removes its signal handlers, before calling vfs_destroy. So any additional kill from name_vanished_handler/dbus_connection_close causes termination immediately... One possible solution might be to set empty signal handler before calling fuse_main. Fuse should restore the previous signal handler (i.e. the empty one) before calling vfs_destroy. However it doesn't work for me, because fuse doesn't replace my handler from some reason and I don't know why... though I am not sure this is right way to fix...
there's an a-c-n patch here - should that land, or do we need to sort out the race first ?
Comment on attachment 321569 [details] [review] fuse: Do not unref result of g_vfs_get_default Sorry, I was sick. Yes, we can push it: commit 70092d7455bb35d560d03aa015d4fef52d441d48
Fuse doesn't replace/restore custom signal handler, only SIG_DFL.
Created attachment 323618 [details] [review] fuse: Avoid crashes when exiting There may be a race when exiting between a fuse main loop and a subthread's main loop. Therefore call g_main_loop_quit always from vfs_destroy on the main thread and wait there for a termination of the subthread to avoid the race.
Created attachment 323619 [details] [review] fuse: Ignore signals during exit procedure Fuse removes its signals handlers in the middle of exit procedure, before vfs_destroy() call. Consequenlty fuse daemon may be killed instead of proper termination if signal is recieved. Reimplement fuse_main helper using lowlevel API in order to ignore new signals during the exit procedure.
Review of attachment 323619 [details] [review]: ::: client/gvfsfusedaemon.c @@ +2565,3 @@ + /* Ignore new signals during exit procedure in order to terminate properly */ + set_custom_signal_handlers (SIG_IGN); + fuse_remove_signal_handlers (se); I guess this works, but can't you just not call fuse_remove_signal_handlers?
Looks good to me.
(In reply to Alexander Larsson from comment #13) > Review of attachment 323619 [details] [review] [review]: > > ::: client/gvfsfusedaemon.c > @@ +2565,3 @@ > + /* Ignore new signals during exit procedure in order to terminate > properly */ > + set_custom_signal_handlers (SIG_IGN); > + fuse_remove_signal_handlers (se); > > I guess this works, but can't you just not call fuse_remove_signal_handlers? oholy because fuse signal handler uses fuse internal structures which are freed later... so another signal might cause segfault... alex Ah, it unsets fuse_instance oholy yep alex and if you don't do that then later signals will crash alex sounds good to me then
Comment on attachment 323618 [details] [review] fuse: Avoid crashes when exiting Pushed with minor whitespace fixes. commit 618c9cbb8621ef0e801cea609a5eaafff88af378
Comment on attachment 323619 [details] [review] fuse: Ignore signals during exit procedure commit 28a8dc531e99bb597c760f2df5be7d40151a3238