[Libguestfs] [PATCH 3/6] daemon: Refine check for Device and Dev_or_Path parameters (RHBZ#1477623).
Richard W.M. Jones
rjones at redhat.com
Thu Aug 3 17:13:48 UTC 2017
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
More information about the Libguestfs
mailing list