On Fri, 10 Nov 2017 11:55:53 +0000 Helg Bredow <[email protected]> wrote:
> On Fri, 10 Nov 2017 10:13:35 +0100 > Anton Lindqvist <[email protected]> wrote: > > > On Fri, Nov 10, 2017 at 09:36:25AM +0100, Martin Pieuchot wrote: > > > On 09/11/17(Thu) 09:02, Helg Bredow wrote: > > > > The current libfuse signal handling assumes that the file system will > > > > always be unmounted by the child. One obvious case where this is not > > > > true is if the file system is busy. To replicate: > > > > > > > > 1. mount a fuse file system > > > > 2. cd anywhere on the file system > > > > 3. pkill -INT <fuse exe> > > > > > > > > The result is a zombie child process and no more response to signals > > > > even if the file system is no longer busy. > > > > > > > > This patch ensures that the child always exits and that an error is > > > > printed to stdout if the file system cannot be unmounted. Tested with > > > > fuse-exfat and ntfs_3g. Suggestions for improvement are welcome. > > > > > > Nice to see that you're fixing a bug. However I'd suggest you to go > > > much further in your improvement. > > > > > > Signal handlers are hard and instead of doing work inside the signal > > > handler you should toggle a global variable/flag and do this work > > > inside the main loop (fuse_loop()?). > > > > > > For example your code below calls fprintf(3) in the signal handler. This > > > is incorrect, this functions is not signal handler safe. What about > > > fuse_unmount()? Are you sure it can be called from a signal handler? > > > > Some more info on making signal handlers asynchronous-safe: > > > > https://www.securecoding.cert.org/confluence/display/c/SIG30-C.+Call+only+asynchronous-safe+functions+within+signal+handlers > > Thanks for the feedback and info guys. I wasn't too confident with this patch > and you've given me some good pointers to improve it. > > -- > Helg <[email protected]> > I've completely rewritten the patch. Is this better? Index: fuse.c =================================================================== RCS file: /cvs/src/lib/libfuse/fuse.c,v retrieving revision 1.34 diff -u -p -u -p -r1.34 fuse.c --- fuse.c 4 Nov 2017 13:17:18 -0000 1.34 +++ fuse.c 12 Nov 2017 04:28:21 -0000 @@ -31,7 +31,7 @@ #include "fuse_private.h" #include "debug.h" -static struct fuse_session *sigse; +static volatile sig_atomic_t signum = 0; static struct fuse_context *ictx = NULL; static int max_read = FUSEBUFMAXSIZE; @@ -61,6 +61,48 @@ static struct fuse_opt fuse_core_opts[] FUSE_OPT_END }; +static void +ifuse_get_signal(int num) +{ + signum = num; +} + +static void +ifuse_child_exit(const struct fuse *f) +{ + int status; + + signal(SIGCHLD, SIG_DFL); + if (waitpid(WAIT_ANY, &status, WNOHANG) == -1) + fprintf(stderr, "fuse: %s\n", strerror(errno)); + + if (WIFEXITED(status) && (WEXITSTATUS(status) != 0)) + fprintf(stderr, "fuse: %s: %s\n", + f->fc->dir, strerror(WEXITSTATUS(status))); + + return; +} + +static void +ifuse_try_unmount(const struct fuse *f) +{ + pid_t child; + + signal(SIGCHLD, ifuse_get_signal); + child = fork(); + + if (child < 0) { + DPERROR(__func__); + return; + } + + if (child == 0) { + errno = 0; + fuse_unmount(f->fc->dir, f->fc); + _exit(errno); + } +} + int fuse_loop(struct fuse *fuse) { @@ -83,9 +125,24 @@ fuse_loop(struct fuse *fuse) while (!fuse->fc->dead) { ret = kevent(fuse->fc->kq, &fuse->fc->event, 1, &ev, 1, NULL); - if (ret == -1) - DPERROR(__func__); - else if (ret > 0) { + if (ret == -1) { + if (errno == EINTR) { + switch (signum) { + case SIGCHLD: + ifuse_child_exit(fuse); + break; + case SIGHUP: + case SIGINT: + case SIGTERM: + ifuse_try_unmount(fuse); + break; + default: + fprintf(stderr, "%s: %s\n", __func__, + strsignal(signum)); + } + } else + DPERROR(__func__); + } else if (ret > 0) { n = read(fuse->fc->fd, &fbuf, sizeof(fbuf)); if (n != sizeof(fbuf)) { fprintf(stderr, "%s: bad fusebuf read\n", @@ -298,38 +355,9 @@ fuse_destroy(struct fuse *f) free(f); } -static void -ifuse_get_signal(unused int num) -{ - struct fuse *f; - pid_t child; - int status; - - if (sigse != NULL) { - child = fork(); - - if (child < 0) - return; - - f = sigse->args; - if (child == 0) { - fuse_unmount(f->fc->dir, f->fc); - sigse = NULL; - exit(0); - } - - fuse_loop(f); - while (waitpid(child, &status, 0) == -1) { - if (errno != EINTR) - break; - } - } -} - void fuse_remove_signal_handlers(unused struct fuse_session *se) { - sigse = NULL; signal(SIGHUP, SIG_DFL); signal(SIGINT, SIG_DFL); signal(SIGTERM, SIG_DFL); @@ -337,12 +365,8 @@ fuse_remove_signal_handlers(unused struc } int -fuse_set_signal_handlers(struct fuse_session *se) +fuse_set_signal_handlers(unused struct fuse_session *se) { - if (se == NULL) - return -1; - - sigse = se; signal(SIGHUP, ifuse_get_signal); signal(SIGINT, ifuse_get_signal); signal(SIGTERM, ifuse_get_signal);
