libfuse: signal handler doesn't cater for "Device busy" and other errors

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

libfuse: signal handler doesn't cater for "Device busy" and other errors

Helg Bredow
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.

Index: fuse.c
===================================================================
RCS file: /cvs/src/lib/libfuse/fuse.c,v
retrieving revision 1.34
diff -u -p -r1.34 fuse.c
--- fuse.c 4 Nov 2017 13:17:18 -0000 1.34
+++ fuse.c 9 Nov 2017 08:41:11 -0000
@@ -303,26 +303,40 @@ ifuse_get_signal(unused int num)
 {
  struct fuse *f;
  pid_t child;
- int status;
+ int status, err;
+
+ if (num == SIGCHLD) {
+ /* child fuse_unmount() completed, check error */
+ while (waitpid(WAIT_ANY, &status, WNOHANG) == -1) {
+ if (errno != EINTR)
+ fprintf(stderr, "fuse: %s\n", strerror(errno));
+ }
+
+ if (WIFEXITED(status)) {
+ err = WEXITSTATUS(status);
+ if (err)
+ fprintf(stderr, "fuse: %s\n", strerror(err));
+ }
+
+ signal(SIGCHLD, SIG_DFL);
+ return;
+ }
 
  if (sigse != NULL) {
+ signal(SIGCHLD, ifuse_get_signal);
  child = fork();
 
  if (child < 0)
- return;
+ return ;
 
- f = sigse->args;
  if (child == 0) {
+ /* try to unmount, file system may be busy */
+ f = sigse->args;
+ errno = 0;
  fuse_unmount(f->fc->dir, f->fc);
- sigse = NULL;
- exit(0);
+ _exit(errno);
  }
 
- fuse_loop(f);
- while (waitpid(child, &status, 0) == -1) {
- if (errno != EINTR)
- break;
- }
  }
 }
 

Reply | Threaded
Open this post in threaded view
|

Re: libfuse: signal handler doesn't cater for "Device busy" and other errors

Martin Pieuchot
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?

I'd recommend you to read signal(3) and some signal handlers like the ones
from dhclient(8) and ntpd(8).

> Index: fuse.c
> ===================================================================
> RCS file: /cvs/src/lib/libfuse/fuse.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 fuse.c
> --- fuse.c 4 Nov 2017 13:17:18 -0000 1.34
> +++ fuse.c 9 Nov 2017 08:41:11 -0000
> @@ -303,26 +303,40 @@ ifuse_get_signal(unused int num)
>  {
>   struct fuse *f;
>   pid_t child;
> - int status;
> + int status, err;
> +
> + if (num == SIGCHLD) {
> + /* child fuse_unmount() completed, check error */
> + while (waitpid(WAIT_ANY, &status, WNOHANG) == -1) {
> + if (errno != EINTR)
> + fprintf(stderr, "fuse: %s\n", strerror(errno));
> + }
> +
> + if (WIFEXITED(status)) {
> + err = WEXITSTATUS(status);
> + if (err)
> + fprintf(stderr, "fuse: %s\n", strerror(err));
> + }
> +
> + signal(SIGCHLD, SIG_DFL);
> + return;
> + }
>  
>   if (sigse != NULL) {
> + signal(SIGCHLD, ifuse_get_signal);
>   child = fork();
>  
>   if (child < 0)
> - return;
> + return ;
>  
> - f = sigse->args;
>   if (child == 0) {
> + /* try to unmount, file system may be busy */
> + f = sigse->args;
> + errno = 0;
>   fuse_unmount(f->fc->dir, f->fc);
> - sigse = NULL;
> - exit(0);
> + _exit(errno);
>   }
>  
> - fuse_loop(f);
> - while (waitpid(child, &status, 0) == -1) {
> - if (errno != EINTR)
> - break;
> - }
>   }
>  }
>  
>

Reply | Threaded
Open this post in threaded view
|

Re: libfuse: signal handler doesn't cater for "Device busy" and other errors

Anton Lindqvist-2
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

Reply | Threaded
Open this post in threaded view
|

Re: libfuse: signal handler doesn't cater for "Device busy" and other errors

Helg Bredow
On Fri, 10 Nov 2017 10:13:35 +0100
Anton Lindqvist <[hidden email]> 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 <[hidden email]>

Reply | Threaded
Open this post in threaded view
|

Re: libfuse: signal handler doesn't cater for "Device busy" and other errors

Helg Bredow
On Fri, 10 Nov 2017 11:55:53 +0000
Helg Bredow <[hidden email]> wrote:

> On Fri, 10 Nov 2017 10:13:35 +0100
> Anton Lindqvist <[hidden email]> 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 <[hidden email]>
>

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

Reply | Threaded
Open this post in threaded view
|

Re: libfuse: signal handler doesn't cater for "Device busy" and other errors

Martin Pieuchot
On 12/11/17(Sun) 04:36, Helg Bredow wrote:

> On Fri, 10 Nov 2017 11:55:53 +0000
> Helg Bredow <[hidden email]> wrote:
>
> > On Fri, 10 Nov 2017 10:13:35 +0100
> > Anton Lindqvist <[hidden email]> 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 <[hidden email]>
> >
>
> 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);
>