For Device parameters we expect a block device name. However we were only testing for "/dev/..." and so chardevs (from the appliance) could be passed here, resulting in strange effects. This adds a function is_device_parameter which tests for a valid block device name. For Dev_or_Path parameters much the same, except we can also use the is_device_parameter function elsewhere in the daemon to distinguish if we were called with a device or path parameter. Previously we used a simple test if the path begins with "/dev/...". Reported by Mathieu Tarral. --- daemon/daemon.h | 1 + daemon/dd.c | 8 +++--- daemon/ext2.c | 4 +-- daemon/file.ml | 2 +- daemon/mount.c | 2 +- daemon/stubs-macros.h | 11 ++------ daemon/upload.c | 6 ++-- daemon/utils-c.c | 7 +++++ daemon/utils.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++ daemon/utils.ml | 1 + daemon/utils.mli | 4 +++ daemon/xfs.c | 4 +-- 12 files changed, 107 insertions(+), 21 deletions(-) diff --git a/daemon/daemon.h b/daemon/daemon.h index a40dc3834..02ae34a6f 100644 --- a/daemon/daemon.h +++ b/daemon/daemon.h @@ -62,6 +62,7 @@ extern dev_t root_device; extern char *sysroot_path (const char *path); extern char *sysroot_realpath (const char *path); extern int is_root_device (const char *device); +extern int is_device_parameter (const char *device); extern int xwrite (int sock, const void *buf, size_t len) __attribute__((__warn_unused_result__)); extern int xread (int sock, void *buf, size_t len) diff --git a/daemon/dd.c b/daemon/dd.c index 15f3f7a6c..0b61c87d8 100644 --- a/daemon/dd.c +++ b/daemon/dd.c @@ -35,7 +35,7 @@ do_dd (const char *src, const char *dest) CLEANUP_FREE char *err = NULL; int r; - src_is_dev = STRPREFIX (src, "/dev/"); + src_is_dev = is_device_parameter (src); if (src_is_dev) r = asprintf (&if_arg, "if=%s", src); @@ -46,7 +46,7 @@ do_dd (const char *src, const char *dest) return -1; } - dest_is_dev = STRPREFIX (dest, "/dev/"); + dest_is_dev = is_device_parameter (dest); if (dest_is_dev) r = asprintf (&of_arg, "of=%s", dest); @@ -71,7 +71,7 @@ do_copy_size (const char *src, const char *dest, int64_t ssize) { int src_fd, dest_fd; - if (STRPREFIX (src, "/dev/")) + if (is_device_parameter (src)) src_fd = open (src, O_RDONLY | O_CLOEXEC); else { CLEANUP_FREE char *buf = sysroot_path (src); @@ -86,7 +86,7 @@ do_copy_size (const char *src, const char *dest, int64_t ssize) return -1; } - if (STRPREFIX (dest, "/dev/")) + if (is_device_parameter (dest)) dest_fd = open (dest, O_WRONLY | O_CLOEXEC); else { CLEANUP_FREE char *buf = sysroot_path (dest); diff --git a/daemon/ext2.c b/daemon/ext2.c index 0c776b6d1..3d16af08e 100644 --- a/daemon/ext2.c +++ b/daemon/ext2.c @@ -1049,7 +1049,7 @@ do_mke2fs (const char *device, /* 0 */ * have to do it manually here, but note that LABEL=.. and * UUID=.. are valid strings which do not require translation. */ - if (STRPREFIX (journaldevice, "/dev/")) { + if (is_device_parameter (journaldevice)) { if (is_root_device (journaldevice)) { reply_with_error ("%s: device not found", journaldevice); return -1; @@ -1068,7 +1068,7 @@ do_mke2fs (const char *device, /* 0 */ sprintf (journaldevice_s, "device=%s", journaldevice_translated); } - else { + else /* XXX check only UUID= or LABEL= should be used here */ { journaldevice_s = malloc (strlen (journaldevice) + 8); if (!journaldevice_s) { reply_with_perror ("malloc"); diff --git a/daemon/file.ml b/daemon/file.ml index b45bd9019..e2c9a0e38 100644 --- a/daemon/file.ml +++ b/daemon/file.ml @@ -25,7 +25,7 @@ open Utils (* This runs the [file] command. *) let file path = - let is_dev = String.is_prefix path "/dev/" in + let is_dev = is_device_parameter path in (* For non-dev, check this is a regular file, else just return the * file type as a string (RHBZ#582484). diff --git a/daemon/mount.c b/daemon/mount.c index bf58bd527..61ce64449 100644 --- a/daemon/mount.c +++ b/daemon/mount.c @@ -120,7 +120,7 @@ do_umount (const char *pathordevice, const char *argv[MAX_ARGS]; size_t i = 0; - is_dev = STREQLEN (pathordevice, "/dev/", 5); + is_dev = is_device_parameter (pathordevice); buf = is_dev ? strdup (pathordevice) : sysroot_path (pathordevice); if (buf == NULL) { diff --git a/daemon/stubs-macros.h b/daemon/stubs-macros.h index e9da16227..2a90938c6 100644 --- a/daemon/stubs-macros.h +++ b/daemon/stubs-macros.h @@ -30,16 +30,11 @@ */ #define RESOLVE_DEVICE(path,path_out,is_filein) \ do { \ - if (STRNEQLEN ((path), "/dev/", 5)) { \ + if (!is_device_parameter ((path))) { \ if (is_filein) cancel_receive (); \ reply_with_error ("%s: %s: expecting a device name", __func__, (path)); \ return; \ } \ - if (is_root_device (path)) { \ - if (is_filein) cancel_receive (); \ - reply_with_error ("%s: %s: device not found", __func__, path); \ - return; \ - } \ (path_out) = device_name_translation ((path)); \ if ((path_out) == NULL) { \ const int err = errno; \ @@ -86,7 +81,7 @@ */ #define REQUIRE_ROOT_OR_RESOLVE_DEVICE(path,path_out,is_filein) \ do { \ - if (STREQLEN ((path), "/dev/", 5)) \ + if (is_device_parameter ((path))) \ RESOLVE_DEVICE ((path), (path_out), (is_filein)); \ else { \ NEED_ROOT ((is_filein), return); \ @@ -105,7 +100,7 @@ */ #define REQUIRE_ROOT_OR_RESOLVE_MOUNTABLE(string, mountable, is_filein) \ do { \ - if (STRPREFIX ((string), "/dev/") || (string)[0] != '/') { \ + if (is_device_parameter ((string)) || (string)[0] != '/') { \ RESOLVE_MOUNTABLE ((string), (mountable), (is_filein)); \ } \ else { \ diff --git a/daemon/upload.c b/daemon/upload.c index 8aaeb2570..9ae1df559 100644 --- a/daemon/upload.c +++ b/daemon/upload.c @@ -94,7 +94,7 @@ upload (const char *filename, int flags, int64_t offset) { int err, is_dev, fd; - is_dev = STRPREFIX (filename, "/dev/"); + is_dev = is_device_parameter (filename); if (!is_dev) CHROOT_IN; fd = open (filename, flags, 0666); @@ -170,7 +170,7 @@ do_download (const char *filename) return -1; } - is_dev = STRPREFIX (filename, "/dev/"); + is_dev = is_device_parameter (filename); if (!is_dev) CHROOT_IN; fd = open (filename, O_RDONLY|O_CLOEXEC); @@ -264,7 +264,7 @@ do_download_offset (const char *filename, int64_t offset, int64_t size) } uint64_t usize = (uint64_t) size; - is_dev = STRPREFIX (filename, "/dev/"); + is_dev = is_device_parameter (filename); if (!is_dev) CHROOT_IN; fd = open (filename, O_RDONLY|O_CLOEXEC); diff --git a/daemon/utils-c.c b/daemon/utils-c.c index 22b0d57c6..d3a8f330b 100644 --- a/daemon/utils-c.c +++ b/daemon/utils-c.c @@ -46,6 +46,13 @@ guestfs_int_daemon_get_verbose_flag (value unitv) /* NB: This is a "noalloc" call. */ value +guestfs_int_daemon_is_device_parameter (value device) +{ + return Val_bool (is_device_parameter (String_val (device))); +} + +/* NB: This is a "noalloc" call. */ +value guestfs_int_daemon_is_root_device (value device) { return Val_bool (is_root_device (String_val (device))); diff --git a/daemon/utils.c b/daemon/utils.c index 6ec232252..6d6aad218 100644 --- a/daemon/utils.c +++ b/daemon/utils.c @@ -37,6 +37,8 @@ #include <sys/wait.h> #include <arpa/inet.h> #include <netinet/in.h> +#include <sys/ioctl.h> +#include <linux/fs.h> #include <errno.h> #include <error.h> #include <assert.h> @@ -100,6 +102,82 @@ is_root_device (const char *device) } /** + * Parameters marked as C<Device>, C<Dev_or_Path>, etc can be passed a + * block device name. This function tests if the parameter is a block + * device name. + * + * It can also be used in daemon code to test if the string passed + * as a C<Dev_or_Path> parameter is a device or path. + */ +int +is_device_parameter (const char *device) +{ + struct stat statbuf; + int fd; + uint64_t n; + + udev_settle_file (device); + + if (!STRPREFIX (device, "/dev/")) + return 0; + + /* Allow any /dev/sd device, so device name translation works. */ + if (STRPREFIX (device, "/dev/sd")) + return 1; + + /* Is it a block device in the appliance? */ + if (stat (device, &statbuf) == -1) { + if (verbose) + fprintf (stderr, "%s: stat: %s: %m\n", "is_device_parameter", device); + return 0; + } + + fprintf (stderr, "mode = %o\n", statbuf.st_mode); + if (S_ISBLK (statbuf.st_mode)) + /* continue */; + else if (S_ISDIR (statbuf.st_mode)) { + fprintf (stderr, "S_ISDIR\n"); + /* The lvremove API allows you to remove all LVs by pointing to + * the VG directory. This was misconceived in the extreme, but + * here we are. XXX + */ + return strlen (device) > 5; + } + else + return 0; + + /* Reject the root (appliance) device. */ + if (is_root_device_stat (&statbuf)) { + if (verbose) + fprintf (stderr, "%s: %s is the root device\n", + "is_device_parameter", device); + return 0; + } + + /* Only now is it safe to try opening the device since chardev devices + * might block when opened. + * + * Only disk-like things should support BLKGETSIZE64. + */ + fd = open (device, O_RDONLY|O_CLOEXEC); + if (fd == -1) { + if (verbose) + fprintf (stderr, "%s: open: %s: %m\n", "is_device_parameter", device); + return 0; + } + if (ioctl (fd, BLKGETSIZE64, &n) == -1) { + if (verbose) + fprintf (stderr, "%s: ioctl BLKGETSIZE64: %s: %m\n", + "is_device_parameter", device); + close (fd); + return 0; + } + close (fd); + + return 1; +} + +/** * Turn C<"/path"> into C<"/sysroot/path">. * * Returns C<NULL> on failure. The caller I<must> check for this and diff --git a/daemon/utils.ml b/daemon/utils.ml index 9e9e68ab4..20a82e212 100644 --- a/daemon/utils.ml +++ b/daemon/utils.ml @@ -22,6 +22,7 @@ open Printf open Std_utils external get_verbose_flag : unit -> bool = "guestfs_int_daemon_get_verbose_flag" "noalloc" +external is_device_parameter : string -> bool = "guestfs_int_daemon_is_device_parameter" "noalloc" external is_root_device : string -> bool = "guestfs_int_daemon_is_root_device" "noalloc" external prog_exists : string -> bool = "guestfs_int_daemon_prog_exists" "noalloc" external udev_settle : ?filename:string -> unit -> unit = "guestfs_int_daemon_udev_settle" "noalloc" diff --git a/daemon/utils.mli b/daemon/utils.mli index 8807b864b..9f9f91a49 100644 --- a/daemon/utils.mli +++ b/daemon/utils.mli @@ -37,6 +37,10 @@ val udev_settle : ?filename:string -> unit -> unit val is_root_device : string -> bool (** Return true if this is the root (appliance) device. *) +val is_device_parameter : string -> bool +(** Use this function to tell the difference between a device + or path for [Dev_or_Path] parameters. *) + val split_device_partition : string -> string * int (** Split a device name like [/dev/sda1] into a device name and partition number, eg. ["sda", 1]. diff --git a/daemon/xfs.c b/daemon/xfs.c index 84e361569..3707f56d9 100644 --- a/daemon/xfs.c +++ b/daemon/xfs.c @@ -327,7 +327,7 @@ do_xfs_info (const char *pathordevice) CLEANUP_FREE_STRING_LIST char **lines = NULL; int is_dev; - is_dev = STREQLEN (pathordevice, "/dev/", 5); + is_dev = is_device_parameter (pathordevice); buf = is_dev ? strdup (pathordevice) : sysroot_path (pathordevice); if (buf == NULL) { @@ -631,7 +631,7 @@ do_xfs_repair (const char *device, ADD_ARG (argv, i, rtdev); } - is_device = STREQLEN (device, "/dev/", 5); + is_device = is_device_parameter (device); if (!is_device) { buf = sysroot_path (device); if (buf == NULL) { -- 2.13.1