After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 762250 - fuse daemon crashes when exiting
fuse daemon crashes when exiting
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: fuse
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gvfs-maint
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2016-02-18 08:52 UTC by Ondrej Holy
Modified: 2016-03-11 07:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
fuse: Do not unref result of g_vfs_get_default (793 bytes, patch)
2016-02-18 08:54 UTC, Ondrej Holy
committed Details | Review
fuse: Avoid crashes when exiting (draft) (2.01 KB, patch)
2016-02-18 09:15 UTC, Ondrej Holy
none Details | Review
fuse: Do not kill itself when exiting (draft) (1.54 KB, patch)
2016-02-18 10:29 UTC, Ondrej Holy
none Details | Review
fuse: Avoid crashes when exiting (2.22 KB, patch)
2016-03-10 12:48 UTC, Ondrej Holy
committed Details | Review
fuse: Ignore signals during exit procedure (2.17 KB, patch)
2016-03-10 12:49 UTC, Ondrej Holy
committed Details | Review

Description Ondrej Holy 2016-02-18 08:52:51 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
Comment 1 Ondrej Holy 2016-02-18 08:54:26 UTC
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.
Comment 2 Ondrej Holy 2016-02-18 09:15:09 UTC
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?
Comment 3 Ondrej Holy 2016-02-18 10:29:39 UTC
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?
Comment 4 Alexander Larsson 2016-02-18 14:29:41 UTC
Review of attachment 321569 [details] [review]:

ack
Comment 5 Alexander Larsson 2016-02-18 14:50:02 UTC
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.
Comment 6 Matthias Clasen 2016-02-18 23:14:58 UTC
putting crashes on the target list for now
Comment 7 Ondrej Holy 2016-02-19 15:37:07 UTC
(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...
Comment 8 Matthias Clasen 2016-03-01 22:56:49 UTC
there's an a-c-n patch here - should that land, or do we need to sort out the race first ?
Comment 9 Ondrej Holy 2016-03-07 13:13:42 UTC
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
Comment 10 Ondrej Holy 2016-03-10 12:46:48 UTC
Fuse doesn't replace/restore custom signal handler, only SIG_DFL.
Comment 11 Ondrej Holy 2016-03-10 12:48:05 UTC
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.
Comment 12 Ondrej Holy 2016-03-10 12:49:09 UTC
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.
Comment 13 Alexander Larsson 2016-03-10 15:55:11 UTC
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?
Comment 14 Alexander Larsson 2016-03-10 15:58:54 UTC
Looks good to me.
Comment 15 Ondrej Holy 2016-03-11 07:32:23 UTC
(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 16 Ondrej Holy 2016-03-11 07:35:02 UTC
Comment on attachment 323618 [details] [review]
fuse: Avoid crashes when exiting

Pushed with minor whitespace fixes.

commit 618c9cbb8621ef0e801cea609a5eaafff88af378
Comment 17 Ondrej Holy 2016-03-11 07:35:25 UTC
Comment on attachment 323619 [details] [review]
fuse: Ignore signals during exit procedure

commit 28a8dc531e99bb597c760f2df5be7d40151a3238