Remove temporary directories created during appliance building along error paths (RHBZ#769680)
From: "Richard W.M. Jones" <rjones@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@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@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs
On Fri, Dec 23, 2011 at 07:27:32PM +0800, Wanlong Gao wrote:
Hi Rich:
Do you know how to reproduce [it] now?
I do see the same temporary directories left around occasionally. But I wasn't able to reliably reproduce the bug, even after trying several things. 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 07:55 PM, Richard W.M. Jones wrote:
On Fri, Dec 23, 2011 at 07:27:32PM +0800, Wanlong Gao wrote:
Hi Rich:
Do you know how to reproduce [it] now?
I do see the same temporary directories left around occasionally. But I wasn't able to reliably reproduce the bug, even after trying several things.
I cooked a patch to remove all the guestfs.?????? when launching, with yours two patches, I think we can prevent to the waste of disk space almost. Right? Thanks -Wanlong Gao
Rich.
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@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
On Fri, Dec 23, 2011 at 10:06:37PM +0800, Wanlong Gao wrote:
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@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);
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:17 PM, Richard W.M. Jones wrote:
On Fri, Dec 23, 2011 at 10:06:37PM +0800, Wanlong Gao wrote:
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@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);
This isn't safe. It'll remove unrelated guestfs.?????? directories that might be in use by other processes.
Yeah, you are right.
Almost every Linux system out there has a /tmp cleaner process which will clean up anything that the current system doesn't catch.
But I can just produce this by kill the guestfish when it's launching. And, can we get the status of which tempdir is in use? Thanks -Wanlong Gao
Rich.
On 12/23/2011 10:22 PM, Wanlong Gao wrote:
On 12/23/2011 10:17 PM, Richard W.M. Jones wrote:
On Fri, Dec 23, 2011 at 10:06:37PM +0800, Wanlong Gao wrote:
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@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);
This isn't safe. It'll remove unrelated guestfs.?????? directories that might be in use by other processes.
Yeah, you are right.
Almost every Linux system out there has a /tmp cleaner process which will clean up anything that the current system doesn't catch.
But I can just produce this by kill the guestfish when it's launching.
And, can we get the status of which tempdir is in use?
Can we take a lock here?
Thanks -Wanlong Gao
Rich.
On Fri, Dec 23, 2011 at 10:38:10PM +0800, Wanlong Gao wrote:
On 12/23/2011 10:22 PM, Wanlong Gao wrote:
On 12/23/2011 10:17 PM, Richard W.M. Jones wrote:
On Fri, Dec 23, 2011 at 10:06:37PM +0800, Wanlong Gao wrote:
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@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);
This isn't safe. It'll remove unrelated guestfs.?????? directories that might be in use by other processes.
Yeah, you are right.
Almost every Linux system out there has a /tmp cleaner process which will clean up anything that the current system doesn't catch.
But I can just produce this by kill the guestfish when it's launching.
And, can we get the status of which tempdir is in use?
Can we take a lock here?
I think the real place to fix this is by keeping the tmpdir in the handle, and cleaning it up during handle cleanup. Handle cleanup happens even if you ^C the process (since there is an atexit handler). Now it's a bit more complicated than this ... At the moment, each libguestfs handle can use one or two temporary directories. The "normal" g->tmpdir is created early on and the directory name is kept in the handle, and it is cleaned up when the handle is closed. This is used to store the virtio-serial socket and some temporary files like the Windows Registry during inspection. The normal g->tmpdir is not used for appliance building however. For appliance building another tmpdir is created -- and this is the one which RHBZ#769680 is talking about. The reason the appliance builder doesn't use g->tmpdir is because the appliance tmpdir must be on the same filesystem as the filesystem we use to store the cached appliance. The cached appliance is stored in /var/tmp/.guestfs-<UUID>. g->tmpdir is created in /tmp by default. So we have to create this second appliance-building tmpdir in /var/tmp. [1] So a solution to this would be to add g->persistent_tmpdir to the handle. It would act similarly to g->tmpdir, being cleaned up in guestfs_close. Anyway, this solution is quite complicated to implement, or at least, more complicated than I was prepared to do for now. Rich. [1] Both tmpdir paths can be changed by setting TMPDIR, and the reason for using separate paths is because of the LSB -- see commit 78f1405de05ef1f2efebafd8245658d1707e59ef. -- 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
participants (2)
-
Richard W.M. Jones -
Wanlong Gao