Libguestfs - Jul 2012 - [PATCH 0/4] Provide guestmount --pid-file and document possible race when unmounting FUSE filesystems.

The full description of this bug is here:

  https://bugzilla.redhat.com/show_bug.cgi?id=838592

and the effect it has on OpenStack is described here:

  https://bugzilla.redhat.com/show_bug.cgi?id=835466#c9

Rich.

From: "Richard W.M. Jones" <rjones at redhat.com>

See also:
https://bugzilla.redhat.com/show_bug.cgi?id=835466#c9
---
 fuse/guestmount.pod |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fuse/guestmount.pod b/fuse/guestmount.pod
index a0bc6f1..c37301c 100644
--- a/fuse/guestmount.pod
+++ b/fuse/guestmount.pod
@@ -106,6 +106,22 @@ namespace using the Linux-specific
L<clone(2)>/L<unshare(2)> flag
 C<CLONE_NEWNS>.  Unfortunately at the moment this requires root and we
 would also probably need to add it as a feature to guestmount.
 
+=head2 Race conditions possible when shutting down the connection
+
+When C<fusermount -u> exits, guestmount may still be running and
+cleaning up the mountpoint.  The disk image will not be fully
+finalized.
+
+This means that scripts like the following have a nasty race
+condition:
+
+ guestmount -a disk.img -i /mnt
+ # copy things into /mnt
+ fusermount -u /mnt
+ # immediately try to use 'disk.img' <-- UNSAFE
+
+The solution is to spin waiting for the guestmount process to exit.
+
 =head1 OPTIONS
 
 =over 4
-- 
1.7.10.4

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 fuse/guestmount.pod |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fuse/guestmount.pod b/fuse/guestmount.pod
index c37301c..d459045 100644
--- a/fuse/guestmount.pod
+++ b/fuse/guestmount.pod
@@ -122,6 +122,10 @@ condition:
 
 The solution is to spin waiting for the guestmount process to exit.
 
+Note that if you use the C<guestfs_mount_local> API directly (see
+L<guestfs(3)/MOUNT LOCAL>) then it is much easier to write a safe,
+race-free program.
+
 =head1 OPTIONS
 
 =over 4
@@ -354,7 +358,7 @@ L<virt-inspector(1)>,
 L<virt-cat(1)>,
 L<virt-edit(1)>,
 L<virt-tar(1)>,
-L<guestfs(3)>,
+L<guestfs(3)/MOUNT LOCAL>,
 L<http://libguestfs.org/>,
 L<http://fuse.sf.net/>.
 
-- 
1.7.10.4

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 fuse/guestmount.c   |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 fuse/guestmount.pod |    8 +++++++-
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/fuse/guestmount.c b/fuse/guestmount.c
index 0c2e2dc..fd31726 100644
--- a/fuse/guestmount.c
+++ b/fuse/guestmount.c
@@ -42,6 +42,8 @@
 #include "guestmount.h"
 #include "options.h"
 
+static int write_pid_file (const char *pid_file, pid_t pid);
+
 #ifndef HAVE_FUSE_OPT_ADD_OPT_ESCAPED
 /* Copied from lib/fuse_opt.c and modified.
  * Copyright (C) 2001-2007  Miklos Szeredi <miklos at szeredi.hu>
@@ -119,6 +121,7 @@ usage (int status)
              "  -m|--mount dev[:mnt[:opts]] Mount dev on mnt (if omitted,
/)\n"
              "  -n|--no-sync         Don't autosync\n"
              "  -o|--option opt      Pass extra option to FUSE\n"
+             "  --pid-file filename  Write PID to filename\n"
              "  -r|--ro              Mount read-only\n"
              "  --selinux            Enable SELinux support\n"
              "  -v|--verbose         Verbose messages\n"
@@ -161,6 +164,7 @@ main (int argc, char *argv[])
     { "mount", 1, 0, 'm' },
     { "no-sync", 0, 0, 'n' },
     { "option", 1, 0, 'o' },
+    { "pid-file", 1, 0, 0 },
     { "ro", 0, 0, 'r' },
     { "rw", 0, 0, 'w' },
     { "selinux", 0, 0, 0 },
@@ -184,6 +188,7 @@ main (int argc, char *argv[])
   int dir_cache_timeout = -1;
   int do_fork = 1;
   char *fuse_options = NULL;
+  char *pid_file = NULL;
 
   struct guestfs_mount_local_argv optargs;
 
@@ -227,6 +232,8 @@ main (int argc, char *argv[])
         echo_keys = 1;
       } else if (STREQ (long_options[option_index].name, "live")) {
         live = 1;
+      } else if (STREQ (long_options[option_index].name, "pid-file"))
{
+        pid_file = optarg;
       } else {
         fprintf (stderr, _("%s: unknown long option: %s (%d)\n"),
                  program_name, long_options[option_index].name, option_index);
@@ -406,8 +413,12 @@ main (int argc, char *argv[])
       exit (EXIT_FAILURE);
     }
 
-    if (pid != 0)               /* parent */
+    if (pid != 0) {             /* parent */
+      if (write_pid_file (pid_file, pid) == -1)
+        _exit (EXIT_FAILURE);
+
       _exit (EXIT_SUCCESS);
+    }
 
     /* Emulate what old fuse_daemonize used to do. */
     if (setsid () == -1) {
@@ -426,6 +437,11 @@ main (int argc, char *argv[])
         close (fd);
     }
   }
+  else {
+    /* not forking, write PID file anyway */
+    if (write_pid_file (pid_file, getpid ()) == -1)
+      exit (EXIT_FAILURE);
+  }
 
   /* Main loop. */
   r = guestfs_mount_local_run (g);
@@ -435,5 +451,33 @@ main (int argc, char *argv[])
     r = -1;
   guestfs_close (g);
 
+  /* Don't delete PID file until the cleanup has been completed. */
+  if (pid_file)
+    unlink (pid_file);
+
   exit (r == 0 ? EXIT_SUCCESS : EXIT_FAILURE);
 }
+
+static int
+write_pid_file (const char *pid_file, pid_t pid)
+{
+  FILE *fp;
+
+  if (pid_file == NULL)
+    return 0;
+
+  fp = fopen (pid_file, "w");
+  if (fp == NULL) {
+  error:
+    perror (pid_file);
+    return -1;
+  }
+
+  if (fprintf (fp, "%d\n", pid) == -1)
+    goto error;
+
+  if (fclose (fp) == -1)
+    goto error;
+
+  return 0;
+}
diff --git a/fuse/guestmount.pod b/fuse/guestmount.pod
index d459045..fdcf133 100644
--- a/fuse/guestmount.pod
+++ b/fuse/guestmount.pod
@@ -120,7 +120,9 @@ condition:
  fusermount -u /mnt
  # immediately try to use 'disk.img' <-- UNSAFE
 
-The solution is to spin waiting for the guestmount process to exit.
+The solution is to use the I<--pid-file> option to write the
+guestmount PID to a file, then after fusermount spin waiting for this
+PID to exit.
 
 Note that if you use the C<guestfs_mount_local> API directly (see
 L<guestfs(3)/MOUNT LOCAL>) then it is much easier to write a safe,
@@ -282,6 +284,10 @@ to the chosen values.
 
 =back
 
+=item B<--pid-file filename>
+
+Write the PID of the guestmount worker process to C<filename>.
+
 =item B<-r>
 
 =item B<--ro>
-- 
1.7.10.4

From: "Richard W.M. Jones" <rjones at redhat.com>

---
 fuse/Makefile.am              |    2 +-
 fuse/test-fuse-umount-race.sh |   77 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100755 fuse/test-fuse-umount-race.sh

diff --git a/fuse/Makefile.am b/fuse/Makefile.am
index 1ab8c7b..8176221 100644
--- a/fuse/Makefile.am
+++ b/fuse/Makefile.am
@@ -73,7 +73,7 @@ stamp-guestmount.pod: guestmount.pod
 # Tests.
 
 if ENABLE_APPLIANCE
-TESTS = test-fuse.sh
+TESTS = test-fuse.sh test-fuse-umount-race.sh
 endif ENABLE_APPLIANCE
 TESTS_ENVIRONMENT = \
 	top_builddir=.. \
diff --git a/fuse/test-fuse-umount-race.sh b/fuse/test-fuse-umount-race.sh
new file mode 100755
index 0000000..a5a6e21
--- /dev/null
+++ b/fuse/test-fuse-umount-race.sh
@@ -0,0 +1,77 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2012 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# https://bugzilla.redhat.com/show_bug.cgi?id=838592
+# This tests that the --pid-file option can be used to fix the race.
+
+unset CDPATH
+set -e
+#set -v
+
+if [ -n "$SKIP_TEST_FUSE_SH" ]; then
+    echo "$0: test skipped because environment variable is set."
+    exit 0
+fi
+
+if [ ! -w /dev/fuse ]; then
+    echo "SKIPPING guestmount test, because there is no /dev/fuse."
+    exit 0
+fi
+
+rm -f test.qcow2 test-copy.qcow2 test.pid
+rm -rf mp
+
+# Make a copy of the Fedora image so we can write to it then discard it.
+qemu-img create -F raw -b ../tests/guests/fedora.img -f qcow2 test.qcow2
+
+mkdir mp
+./guestmount -a test.qcow2 -m /dev/VG/Root --pid-file test.pid mp
+cp $0 mp/test-umount
+
+count=10
+while ! fusermount -u mp && [ $count -gt 0 ]; do
+    sleep 1
+    ((count--))
+done
+if [ $count -eq 0 ]; then
+    echo "$0: fusermount failed after 10 attempts"
+    exit 1
+fi
+
+# Wait for guestmount to exit.
+count=10
+while kill -0 `cat test.pid` 2>/dev/null && [ $count -gt 0 ]; do
+    sleep 1
+    ((count--))
+done
+if [ $count -eq 0 ]; then
+    echo "$0: wait for guestmount to exit failed after 10 seconds"
+    exit 1
+fi
+
+# It should now be safe to copy and read the disk image.
+cp test.qcow2 test-copy.qcow2
+
+if [ "$(../fish/guestfish -a test-copy.qcow2 --ro -i is-file
/test-umount)" != "true" ]; then
+    echo "$0: test failed"
+    exit 1
+fi
+
+rm test.qcow2 test-copy.qcow2
+rm -f test.pid
+rm -r mp
-- 
1.7.10.4