Libguestfs - Dec 2011 - Remove temporary directories created during appliance building along error paths (RHBZ#769680)

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

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

This function does 'rm -rf <dir>' for temporary directories,
safely
working if '<dir>' contains shell meta-characters.

Replace existing code for removing directories with this.
---
 src/appliance.c        |    2 +-
 src/filearch.c         |    7 +------
 src/guestfs-internal.h |    1 +
 src/guestfs.c          |   41 ++---------------------------------------
 src/launch.c           |   30 ++++++++++++++++++++++++++++++
 5 files changed, 35 insertions(+), 46 deletions(-)

diff --git a/src/appliance.c b/src/appliance.c
index 836e679..5f77a56 100644
--- a/src/appliance.c
+++ b/src/appliance.c
@@ -534,7 +534,7 @@ build_supermin_appliance (guestfs_h *g,
     return -1;
   }
 
-  rmdir (tmpcd);
+  guestfs___remove_tmpdir (tmpcd);
 
   /* Now finish off by linking to the cached appliance and returning it. */
   if (hard_link_to_cached_appliance (g, cachedir,
diff --git a/src/filearch.c b/src/filearch.c
index 0ed6425..1802ec4 100644
--- a/src/filearch.c
+++ b/src/filearch.c
@@ -213,12 +213,7 @@ cpio_arch (guestfs_h *g, const char *file, const char
*path)
   error (g, "file_architecture: could not determine architecture of cpio
archive");
 
  out:
-  /* Free up the temporary directory.  Note the directory name cannot
-   * contain shell meta-characters because of the way it was
-   * constructed above.
-   */
-  snprintf (cmd, cmd_len, "rm -rf %s", dir);
-  ignore_value (system (cmd));
+  guestfs___remove_tmpdir (dir);
 
   return ret;
 #undef dir_len
diff --git a/src/guestfs-internal.h b/src/guestfs-internal.h
index 4b7cb16..541afb3 100644
--- a/src/guestfs-internal.h
+++ b/src/guestfs-internal.h
@@ -374,6 +374,7 @@ extern void guestfs___debug (guestfs_h *g, const char *fs,
...)
 extern void guestfs___trace (guestfs_h *g, const char *fs, ...)
   __attribute__((format (printf,2,3)));
 extern const char *guestfs___persistent_tmpdir (void);
+extern void guestfs___remove_tmpdir (const char *dir);
 extern void guestfs___print_timestamped_message (guestfs_h *g, const char *fs,
...);
 extern void guestfs___free_inspect_info (guestfs_h *g);
 extern void guestfs___free_drives (struct drive **drives);
diff --git a/src/guestfs.c b/src/guestfs.c
index 450ffd8..9f73486 100644
--- a/src/guestfs.c
+++ b/src/guestfs.c
@@ -72,7 +72,6 @@
 #include "guestfs_protocol.h"
 
 static void default_error_cb (guestfs_h *g, void *data, const char *msg);
-static void remove_tmpdir (guestfs_h *g);
 static void close_handles (void);
 
 gl_lock_define_initialized (static, handles_lock);
@@ -224,7 +223,8 @@ guestfs_close (guestfs_h *g)
   if (g->recoverypid > 0) waitpid (g->recoverypid, NULL, 0);
 
   /* Remove whole temporary directory. */
-  remove_tmpdir (g);
+  guestfs___remove_tmpdir (g->tmpdir);
+  free (g->tmpdir);
 
   if (g->cmdline) {
     size_t i;
@@ -260,43 +260,6 @@ guestfs_close (guestfs_h *g)
   free (g);
 }
 
-/* g->tmpdir can contain any files (but not subdirectories).  Remove
- * those and the directory itself.  Note that errors in this function
- * aren't really that important: if we end up not deleting temporary
- * files it's only annoying.
- */
-static void
-remove_tmpdir (guestfs_h *g)
-{
-  DIR *dir;
-  struct dirent *d;
-
-  if (!g->tmpdir)
-    return;
-
-  dir = opendir (g->tmpdir);
-  if (dir == NULL) {
-    perror (g->tmpdir);
-    return;
-  }
-
-  while ((d = readdir (dir)) != NULL) {
-    if (STRNEQ (d->d_name, ".") && STRNEQ (d->d_name,
"..")) {
-      if (unlinkat (dirfd (dir), d->d_name, 0) == -1)
-        perror (d->d_name);
-    }
-  }
-
-  if (closedir (dir) == -1)
-    perror (g->tmpdir);
-
-  if (rmdir (g->tmpdir) == -1)
-    perror (g->tmpdir);
-
-  free (g->tmpdir);
-  g->tmpdir = NULL;
-}
-
 /* Close all open handles (called from atexit(3)). */
 static void
 close_handles (void)
diff --git a/src/launch.c b/src/launch.c
index 9add092..ca89b63 100644
--- a/src/launch.c
+++ b/src/launch.c
@@ -32,6 +32,8 @@
 #include <time.h>
 #include <sys/stat.h>
 #include <sys/select.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 #include <dirent.h>
 #include <signal.h>
 #include <assert.h>
@@ -1092,6 +1094,34 @@ guestfs___persistent_tmpdir (void)
   return tmpdir;
 }
 
+/* Recursively remove a temporary directory.  If removal fails, just
+ * return (it's a temporary directory so it'll eventually be cleaned
+ * up by a temp cleaner).  This is done using "rm -rf" because
that's
+ * simpler and safer, but we have to exec to ensure that paths don't
+ * need to be quoted.
+ */
+void
+guestfs___remove_tmpdir (const char *dir)
+{
+  pid_t pid = fork ();
+
+  if (pid == -1) {
+    perror ("remove tmpdir: fork");
+    return;
+  }
+  if (pid == 0) {
+    execlp ("rm", "rm", "-rf", dir, NULL);
+    perror ("remove tmpdir: exec: rm");
+    _exit (EXIT_FAILURE);
+  }
+
+  /* Parent. */
+  if (waitpid (pid, NULL, 0) == -1) {
+    perror ("remove tmpdir: waitpid");
+    return;
+  }
+}
+
 /* Compute Y - X and return the result in milliseconds.
  * Approximately the same as this code:
  * http://www.mpp.mpg.de/~huber/util/timevaldiff.c
-- 
1.7.6

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

---
 src/appliance.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/src/appliance.c b/src/appliance.c
index 5f77a56..57ff38f 100644
--- a/src/appliance.c
+++ b/src/appliance.c
@@ -450,8 +450,10 @@ build_supermin_appliance (guestfs_h *g,
     guestfs___print_timestamped_message (g, "run
febootstrap-supermin-helper");
 
   int r = run_supermin_helper (g, supermin_path, tmpcd, len);
-  if (r == -1)
+  if (r == -1) {
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
+  }
 
   if (g->verbose)
     guestfs___print_timestamped_message (g, "finished building supermin
appliance");
@@ -468,6 +470,7 @@ build_supermin_appliance (guestfs_h *g,
   int fd = open (filename, O_WRONLY|O_CREAT, 0755);
   if (fd == -1) {
     perrorf (g, "open: %s", filename);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
   struct flock fl;
@@ -481,6 +484,7 @@ build_supermin_appliance (guestfs_h *g,
       goto again;
     perrorf (g, "fcntl: F_SETLKW: %s", filename);
     close (fd);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
 
@@ -492,6 +496,7 @@ build_supermin_appliance (guestfs_h *g,
   if (ftruncate (fd, clen) == -1) {
     perrorf (g, "ftruncate: %s", filename);
     close (fd);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
 
@@ -499,11 +504,13 @@ build_supermin_appliance (guestfs_h *g,
   if (rr == -1) {
     perrorf (g, "write: %s", filename);
     close (fd);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
   if ((size_t) rr != clen) {
     error (g, "partial write: %s", filename);
     close (fd);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
 
@@ -513,6 +520,7 @@ build_supermin_appliance (guestfs_h *g,
   if (rename (filename, filename2) == -1) {
     perrorf (g, "rename: %s %s", filename, filename2);
     close (fd);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
 
@@ -522,6 +530,7 @@ build_supermin_appliance (guestfs_h *g,
   if (rename (filename, filename2) == -1) {
     perrorf (g, "rename: %s %s", filename, filename2);
     close (fd);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
 
@@ -531,6 +540,7 @@ build_supermin_appliance (guestfs_h *g,
   if (rename (filename, filename2) == -1) {
     perrorf (g, "rename: %s %s", filename, filename2);
     close (fd);
+    guestfs___remove_tmpdir (tmpcd);
     return -1;
   }
 
-- 
1.7.6

Hi Rich:

Do you know how to reproduce now?

Thanks
-Wanlong Gao
> https://bugzilla.redhat.com/show_bug.cgi?id=769680
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://www.redhat.com/mailman/listinfo/libguestfs

If guestfish or other progs which launching appliance was aborted or
killed last time, the temp dir will be remained, so delete it when
this time launching.
Prevent the possible waste of disk space.

Signed-off-by: Wanlong Gao <gaowanlong at cn.fujitsu.com>
---
 src/appliance.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/src/appliance.c b/src/appliance.c
index 57ff38f..4ce8405 100644
--- a/src/appliance.c
+++ b/src/appliance.c
@@ -36,6 +36,8 @@
 #include <sys/types.h>
 #endif
 
+#include "ignore-value.h"
+
 #include "guestfs.h"
 #include "guestfs-internal.h"
 #include "guestfs-internal-actions.h"
@@ -437,6 +439,14 @@ build_supermin_appliance (guestfs_h *g,
    */
   size_t len = strlen (tmpdir) + 128;
 
+  /* If guestfish or other progs which launching appliance was aborted or
+   * killed last time, the temp dir will be remained, so delete it when
+   * this time launching.
+   */
+  char cmd[len];
+  snprintf (cmd, len, "rm -rf %s/guestfs.??????", tmpdir);
+  ignore_value (system (cmd));
+
   /* Build the appliance into a temporary directory. */
   char tmpcd[len];
   snprintf (tmpcd, len, "%s/guestfs.XXXXXX", tmpdir);
-- 
1.7.8