From 5922d7084d6b43f0a1a15b664c7082dfeaf584d0 Mon Sep 17 00:00:00 2001 From: Richard Jones Date: Sat, 17 Apr 2010 14:25:55 +0100 Subject: [PATCH] Fix upload losing synchronization if root not mounted (RHBZ#576879). Modify the generator so that it can correctly handle early cancellation for Pathname|Device|.. parameters. This fixes the upload command, but consequently we need to fix the parameters for tar_in and t?z_in commands. This should also mean that 'win:' can now be used as the second argument of tar_in and t?z_in commands in guestfish, whereas previously this wouldn't have worked. Adds a regression test for the original problem. --- daemon/command.c | 2 +- daemon/daemon.h | 26 +++++++++++++++++--------- daemon/df.c | 4 ++-- daemon/guestfsd.c | 24 +++++++++--------------- daemon/inotify.c | 2 +- daemon/mount.c | 8 ++++---- daemon/tar.c | 12 ------------ daemon/upload.c | 7 ------- regressions/Makefile.am | 1 + regressions/rhbz576879.sh | 33 +++++++++++++++++++++++++++++++++ src/generator.ml | 37 ++++++++++++++++++++++++------------- 11 files changed, 92 insertions(+), 64 deletions(-) create mode 100755 regressions/rhbz576879.sh diff --git a/daemon/command.c b/daemon/command.c index ca06478..5e87067 100644 --- a/daemon/command.c +++ b/daemon/command.c @@ -36,7 +36,7 @@ do_command (char *const *argv) int dev_ok, dev_pts_ok, proc_ok, selinux_ok, sys_ok; /* We need a root filesystem mounted to do this. */ - NEED_ROOT (return NULL); + NEED_ROOT (, return NULL); /* Conveniently, argv is already a NULL-terminated argv-style array * of parameters, so we can pass it straight in to our internal diff --git a/daemon/daemon.h b/daemon/daemon.h index ebbeaa2..81a9f36 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -68,7 +68,7 @@ extern char **split_lines (char *str); extern void trim (char *str); -extern int device_name_translation (char *device, const char *func); +extern int device_name_translation (char *device); extern int prog_exists (const char *prog); @@ -155,9 +155,10 @@ extern void reply (xdrproc_t xdrp, char *ret); /* Helper for functions that need a root filesystem mounted. * NB. Cannot be used for FileIn functions. */ -#define NEED_ROOT(fail_stmt) \ +#define NEED_ROOT(cancel_stmt,fail_stmt) \ do { \ if (!root_mounted) { \ + cancel_stmt; \ reply_with_error ("%s: you must call 'mount' first to mount the root filesystem", __func__); \ fail_stmt; \ } \ @@ -167,9 +168,10 @@ extern void reply (xdrproc_t xdrp, char *ret); /* Helper for functions that need an argument ("path") that is absolute. * NB. Cannot be used for FileIn functions. */ -#define ABS_PATH(path,fail_stmt) \ +#define ABS_PATH(path,cancel_stmt,fail_stmt) \ do { \ if ((path)[0] != '/') { \ + cancel_stmt; \ reply_with_error ("%s: path must start with a / character", __func__); \ fail_stmt; \ } \ @@ -182,14 +184,20 @@ extern void reply (xdrproc_t xdrp, char *ret); * * NB. Cannot be used for FileIn functions. */ -#define RESOLVE_DEVICE(path,fail_stmt) \ +#define RESOLVE_DEVICE(path,cancel_stmt,fail_stmt) \ do { \ if (STRNEQLEN ((path), "/dev/", 5)) { \ + cancel_stmt; \ reply_with_error ("%s: %s: expecting a device name", __func__, (path)); \ fail_stmt; \ } \ - if (device_name_translation ((path), __func__) == -1) \ + if (device_name_translation ((path)) == -1) { \ + int err = errno; \ + cancel_stmt; \ + errno = err; \ + reply_with_perror ("%s: %s", __func__, path); \ fail_stmt; \ + } \ } while (0) /* Helper for functions which need either an absolute path in the @@ -202,13 +210,13 @@ extern void reply (xdrproc_t xdrp, char *ret); * because we intend in future to make device parameters a distinct * type from filenames. */ -#define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,fail_stmt) \ +#define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,cancel_stmt,fail_stmt) \ do { \ if (STREQLEN ((path), "/dev/", 5)) \ - RESOLVE_DEVICE ((path), fail_stmt); \ + RESOLVE_DEVICE ((path), cancel_stmt, fail_stmt); \ else { \ - NEED_ROOT (fail_stmt); \ - ABS_PATH ((path),fail_stmt); \ + NEED_ROOT (cancel_stmt, fail_stmt); \ + ABS_PATH ((path), cancel_stmt, fail_stmt); \ } \ } while (0) diff --git a/daemon/df.c b/daemon/df.c index b5c4980..7aa6f4f 100644 --- a/daemon/df.c +++ b/daemon/df.c @@ -33,7 +33,7 @@ do_df (void) int r; char *out, *err; - NEED_ROOT (return NULL); + NEED_ROOT (, return NULL); r = command (&out, &err, "df", NULL); if (r == -1) { @@ -54,7 +54,7 @@ do_df_h (void) int r; char *out, *err; - NEED_ROOT (return NULL); + NEED_ROOT (, return NULL); r = command (&out, &err, "df", "-h", NULL); if (r == -1) { diff --git a/daemon/guestfsd.c b/daemon/guestfsd.c index 69baf9e..06ad702 100644 --- a/daemon/guestfsd.c +++ b/daemon/guestfsd.c @@ -1022,42 +1022,36 @@ print_arginfo (const struct printf_info *info, size_t n, int *argtypes) * the device nodes themselves will exist in the appliance. */ int -device_name_translation (char *device, const char *func) +device_name_translation (char *device) { int fd; fd = open (device, O_RDONLY); if (fd >= 0) { + close_ok: close (fd); return 0; } - if (errno != ENXIO && errno != ENOENT) { - error: - reply_with_perror ("%s: %s", func, device); + if (errno != ENXIO && errno != ENOENT) return -1; - } /* If the name begins with "/dev/sd" then try the alternatives. */ if (STRNEQLEN (device, "/dev/sd", 7)) - goto error; + return -1; device[5] = 'h'; /* /dev/hd (old IDE driver) */ fd = open (device, O_RDONLY); - if (fd >= 0) { - close (fd); - return 0; - } + if (fd >= 0) + goto close_ok; device[5] = 'v'; /* /dev/vd (for virtio devices) */ fd = open (device, O_RDONLY); - if (fd >= 0) { - close (fd); - return 0; - } + if (fd >= 0) + goto close_ok; device[5] = 's'; /* Restore original device name. */ - goto error; + return -1; } /* Check program exists and is executable on $PATH. Actually, we diff --git a/daemon/inotify.c b/daemon/inotify.c index ec3aa64..104fa60 100644 --- a/daemon/inotify.c +++ b/daemon/inotify.c @@ -70,7 +70,7 @@ do_inotify_init (int max_events) #ifdef HAVE_SYS_INOTIFY_H FILE *fp; - NEED_ROOT (return -1); + NEED_ROOT (, return -1); if (max_events < 0) { reply_with_error ("max_events < 0"); diff --git a/daemon/mount.c b/daemon/mount.c index 838bef8..8927c6c 100644 --- a/daemon/mount.c +++ b/daemon/mount.c @@ -48,7 +48,7 @@ do_mount_vfs (const char *options, const char *vfstype, char *mp; char *error; - ABS_PATH (mountpoint, return -1); + ABS_PATH (mountpoint, , return -1); is_root = STREQ (mountpoint, "/"); @@ -121,7 +121,7 @@ do_umount (const char *pathordevice) } if (is_dev) - RESOLVE_DEVICE (buf, { free (buf); return -1; }); + RESOLVE_DEVICE (buf, , { free (buf); return -1; }); r = command (NULL, &err, "umount", buf, NULL); free (buf); @@ -356,7 +356,7 @@ do_mkmountpoint (const char *path) int r; /* NEED_ROOT (return -1); - we don't want this test for this call. */ - ABS_PATH (path, return -1); + ABS_PATH (path, , return -1); CHROOT_IN; r = mkdir (path, 0777); @@ -381,7 +381,7 @@ do_rmmountpoint (const char *path) int r; /* NEED_ROOT (return -1); - we don't want this test for this call. */ - ABS_PATH (path, return -1); + ABS_PATH (path, , return -1); CHROOT_IN; r = rmdir (path); diff --git a/daemon/tar.c b/daemon/tar.c index 5b73599..26a0d30 100644 --- a/daemon/tar.c +++ b/daemon/tar.c @@ -42,12 +42,6 @@ do_tar_in (const char *dir) FILE *fp; char *cmd; - if (!root_mounted || dir[0] != '/') { - cancel_receive (); - reply_with_error ("root must be mounted and path must be absolute"); - return -1; - } - /* "tar -C /sysroot%s -xf -" but we have to quote the dir. */ if (asprintf_nowarn (&cmd, "tar -C %R -xf -", dir) == -1) { err = errno; @@ -165,12 +159,6 @@ do_tXz_in (const char *dir, char filter) FILE *fp; char *cmd; - if (!root_mounted || dir[0] != '/') { - cancel_receive (); - reply_with_error ("root must be mounted and path must be absolute"); - return -1; - } - /* "tar -C /sysroot%s -zxf -" but we have to quote the dir. */ if (asprintf_nowarn (&cmd, "tar -C %R -%cxf -", dir, filter) == -1) { err = errno; diff --git a/daemon/upload.c b/daemon/upload.c index 3c20d6f..d93a5ad 100644 --- a/daemon/upload.c +++ b/daemon/upload.c @@ -41,13 +41,6 @@ do_upload (const char *filename) int err, fd, r, is_dev; is_dev = STRPREFIX (filename, "/dev/"); - if (!is_dev) { - if (!root_mounted || filename[0] != '/') { - cancel_receive (); - reply_with_error ("root must be mounted and path must be absolute"); - return -1; - } - } if (!is_dev) CHROOT_IN; fd = open (filename, O_WRONLY|O_CREAT|O_TRUNC|O_NOCTTY, 0666); diff --git a/regressions/Makefile.am b/regressions/Makefile.am index 8513f9a..7ec9dc8 100644 --- a/regressions/Makefile.am +++ b/regressions/Makefile.am @@ -27,6 +27,7 @@ TESTS = \ rhbz503169c10.sh \ rhbz503169c13.sh \ rhbz557655.sh \ + rhbz576879.sh \ rhbz578407.sh \ rhbz580246.sh \ test-cancellation-download-librarycancels.sh \ diff --git a/regressions/rhbz576879.sh b/regressions/rhbz576879.sh new file mode 100755 index 0000000..639dd7c --- /dev/null +++ b/regressions/rhbz576879.sh @@ -0,0 +1,33 @@ +#!/bin/bash - +# libguestfs +# Copyright (C) 2010 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., 675 Mass Ave, Cambridge, MA 02139, USA. + +# Regression test for: +# https://bugzilla.redhat.com/show_bug.cgi?id=576879 +# upload loses synchronization if the disk is not mounted + +set -e + +rm -f test1.img + +../fish/guestfish < program). The checksum is returned as a printable string."); - ("tar_in", (RErr, [FileIn "tarfile"; String "directory"]), 69, [], + ("tar_in", (RErr, [FileIn "tarfile"; Pathname "directory"]), 69, [], [InitBasicFS, Always, TestOutput ( [["tar_in"; "../images/helloworld.tar"; "/"]; ["cat"; "/hello"]], "hello\n")], @@ -2050,7 +2050,7 @@ it to local file C. To download a compressed tarball, use C or C."); - ("tgz_in", (RErr, [FileIn "tarball"; String "directory"]), 71, [], + ("tgz_in", (RErr, [FileIn "tarball"; Pathname "directory"]), 71, [], [InitBasicFS, Always, TestOutput ( [["tgz_in"; "../images/helloworld.tar.gz"; "/"]; ["cat"; "/hello"]], "hello\n")], @@ -4382,7 +4382,7 @@ This command writes zeroes over the entire C. Compare with C which just zeroes the first few blocks of a device."); - ("txz_in", (RErr, [FileIn "tarball"; String "directory"]), 229, [], + ("txz_in", (RErr, [FileIn "tarball"; Pathname "directory"]), 229, [], [InitBasicFS, Always, TestOutput ( [["txz_in"; "../images/helloworld.tar.xz"; "/"]; ["cat"; "/hello"]], "hello\n")], @@ -6032,14 +6032,19 @@ and generate_daemon_actions () = ); pr "\n"; + let is_filein = + List.exists (function FileIn _ -> true | _ -> false) (snd style) in + (match snd style with | [] -> () | args -> pr " memset (&args, 0, sizeof args);\n"; pr "\n"; pr " if (!xdr_guestfs_%s_args (xdr_in, &args)) {\n" name; + if is_filein then + pr " cancel_receive ();\n"; pr " reply_with_error (\"daemon failed to decode procedure arguments\");\n"; - pr " return;\n"; + pr " goto done;\n"; pr " }\n"; let pr_args n = pr " char *%s = args.%s;\n" n n @@ -6048,6 +6053,8 @@ and generate_daemon_actions () = pr " %s = realloc (args.%s.%s_val,\n" n n n; pr " sizeof (char *) * (args.%s.%s_len+1));\n" n n; pr " if (%s == NULL) {\n" n; + if is_filein then + pr " cancel_receive ();\n"; pr " reply_with_perror (\"realloc\");\n"; pr " goto done;\n"; pr " }\n"; @@ -6058,13 +6065,16 @@ and generate_daemon_actions () = function | Pathname n -> pr_args n; - pr " ABS_PATH (%s, goto done);\n" n; + pr " ABS_PATH (%s, %s, goto done);\n" + n (if is_filein then "cancel_receive ()" else ""); | Device n -> pr_args n; - pr " RESOLVE_DEVICE (%s, goto done);\n" n; + pr " RESOLVE_DEVICE (%s, %s, goto done);\n" + n (if is_filein then "cancel_receive ()" else ""); | Dev_or_Path n -> pr_args n; - pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (%s, goto done);\n" n; + pr " REQUIRE_ROOT_OR_RESOLVE_DEVICE (%s, %s, goto done);\n" + n (if is_filein then "cancel_receive ()" else ""); | String n -> pr_args n | OptString n -> pr " %s = args.%s ? *args.%s : NULL;\n" n n n | StringList n -> @@ -6074,7 +6084,8 @@ and generate_daemon_actions () = pr " /* Ensure that each is a device,\n"; pr " * and perform device name translation. */\n"; pr " { int pvi; for (pvi = 0; physvols[pvi] != NULL; ++pvi)\n"; - pr " RESOLVE_DEVICE (physvols[pvi], goto done);\n"; + pr " RESOLVE_DEVICE (physvols[pvi], %s, goto done);\n" + (if is_filein then "cancel_receive ()" else ""); pr " }\n"; | Bool n -> pr " %s = args.%s;\n" n n | Int n -> pr " %s = args.%s;\n" n n @@ -6089,7 +6100,8 @@ and generate_daemon_actions () = if List.exists (function Pathname _ -> true | _ -> false) (snd style) then ( (* Emit NEED_ROOT just once, even when there are two or more Pathname args *) - pr " NEED_ROOT (goto done);\n"; + pr " NEED_ROOT (%s, goto done);\n" + (if is_filein then "cancel_receive ()" else ""); ); (* Don't want to call the impl with any FileIn or FileOut @@ -6175,15 +6187,14 @@ and generate_daemon_actions () = ); (* Free the args. *) + pr "done:\n"; (match snd style with - | [] -> - pr "done: ;\n"; + | [] -> () | _ -> - pr "done:\n"; pr " xdr_free ((xdrproc_t) xdr_guestfs_%s_args, (char *) &args);\n" name ); - + pr " return;\n"; pr "}\n\n"; ) daemon_functions; -- 1.8.3.1