From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; 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

On Fri, Dec 23, 2011 at 10:06:37PM +0800, Wanlong Gao wrote: This isn't safe. It'll remove unrelated guestfs.?????? directories that might be in use by other processes. Almost every Linux system out there has a /tmp cleaner process which will clean up anything that the current system doesn't catch. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top

On 12/23/2011 10:22 PM, Wanlong Gao wrote: Can we take a lock here?