Re: [libvirt] [PATCH v3] storage: Check for invalid storage mode before opening

28 May 2010

On 05/28/2010 12:55 PM, Eric Blake wrote:
...
On 05/24/2010 03:36 PM, Cole Robinson wrote:
...
If a directory pool contains pipes or sockets, a pool start can fail or hang:
https://bugzilla.redhat.com/show_bug.cgi?id=589577
We already try to avoid these special files, but only attempt after
opening the path, which is where the problems lie. Unify volume opening
into helper functions, which use the proper open() flags to avoid error,
followed by fstat to validate storage mode.

--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -872,6 +872,61 @@ virStorageBackendForType(int type) {
     return NULL;
 }
+static int
+virStorageBackendVolOpenInternal(const char *path)
+{
+    int fd;
+    struct stat sb;
+
+    if ((fd = open(path, O_RDONLY|O_NONBLOCK|O_NOCTTY)) < 0) {
+        virReportSystemError(errno,
+                             _("cannot open volume '%s'"),
+                             path);
+        return -1;
+    }
+
+    if (fstat(fd, &sb) < 0) {
+        virReportSystemError(errno,
+                             _("cannot stat file '%s'"),
+                             path);
+        close(fd);
+        return -1;
+    }
+
+    if (!S_ISREG(sb.st_mode) &&
+        !S_ISCHR(sb.st_mode) &&
+        !S_ISBLK(sb.st_mode)) {
+        close(fd);
+        return -2;
+    }
+
+    return fd;
+}
+
+/*
+ * Allows caller to silently ignore files with improper mode
+ *
+ * Returns -1 on error, -2 if file mode is unexpected.
+ */
+int virStorageBackendVolOpenModeSkip(const char *path)
+{
+    return virStorageBackendVolOpenInternal(path);
+}
+
+int
+virStorageBackendVolOpen(const char *path)
Hmm, I'm thinking that better semantics might be:
enum {
  VIR_STORAGE_OPEN_WARN = 1 << 0; // warn if unexpected type encountered
  VIR_STORAGE_OPEN_REG = 1 << 1; // regular files okay
  VIR_STORAGE_OPEN_BLOCK = 1 << 2; // block files okay
  VIR_STORAGE_OPEN_DIR = 1 << 3; // directories okay
  ...
}
int virStorageBackendVolOpen(const char *path) {
    return virStorageBackendVolOpenCheckMode(path, 0);
}
int virStorageBackendVolOpenCheckMode(const char *path,
      unsigned int flags) {
  open
  fstat
  if (flags & VIR_STORAGE_OPEN_WARN) {
    if (type == REG && !(flags & VIR_STORAGE_OPEN_REG))
       warn
    else if (type == DIR && !(flags & VIR_STORAGE_OPEN_DIR))
       warn
    ...
  }
  return result;
}
Then, you can preserve existing semantics of no warning by calling the
one-argument version, or fine-tune which file types you are willing to
accept (some callers will accept directories, others will not) by
calling the two-arg version.
Thanks, I've sent a new version. It doesn't add the directory handling
or change the FS backend values, I think that's best saved for another
patch so we aren't mixing bug fixing with enhancement.

Thanks,
Cole

    