On 12/11/17(Sun) 04:36, Helg Bredow wrote: > On Fri, 10 Nov 2017 11:55:53 +0000 > Helg Bredow <xx...@msn.com> wrote: > > > On Fri, 10 Nov 2017 10:13:35 +0100 > > Anton Lindqvist <an...@openbsd.org> 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 <xx...@msn.com> > > > > I've completely rewritten the patch. Is this better?
Much better! However I'd suggest using two variables instead of the single `signum'. Otherwise you might fail to handle the first signal delivered. > 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); >