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);
> 

Reply via email to