On 12/11/17(Sun) 04:36, Helg Bredow wrote:
> 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?
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);
>
