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 +
> Helg Bredow  wrote:
> 
> > On Fri, 10 Nov 2017 10:13:35 +0100
> > Anton Lindqvist  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 
> > > > > 
> > > > > 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 
> > 
> 
> 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.c4 Nov 2017 13:17:18 -   1.34
> +++ fuse.c12 Nov 2017 04:28:21 -
> @@ -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, , 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, >fc->event, 1, , 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) 

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

[Helg Bredow]

On Fri, 10 Nov 2017 11:55:53 +
Helg Bredow  wrote:

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

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 -   1.34
+++ fuse.c  12 Nov 2017 04:28:21 -
@@ -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, , 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, >fc->event, 1, , 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, , 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;
-
-  

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

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

[Anton Lindqvist]

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

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 
> 
> 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.c4 Nov 2017 13:17:18 -   1.34
> +++ fuse.c9 Nov 2017 08:41:11 -
> @@ -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, , 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, , 0) == -1) {
> - if (errno != EINTR)
> - break;
> - }
>   }
>  }
>  
> 

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 

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 -   1.34
+++ fuse.c  9 Nov 2017 08:41:11 -
@@ -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, , 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, , 0) == -1) {
-   if (errno != EINTR)
-   break;
-   }
}
 }