[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH v3] storage: Sanitize pool target paths



Spurious / in a pool target path makes life difficult for apps using the
GetVolByPath, and doing other path based comparisons with pools. This
has caused a few issues for virt-manager users:

https://bugzilla.redhat.com/show_bug.cgi?id=494005
https://bugzilla.redhat.com/show_bug.cgi?id=593565

Add a new util API which removes spurious /, virFileSanitizePath. Sanitize
target paths when parsing pool XML, and for paths passed to GetVolByPath.

v2: Leading // must be preserved, properly sanitize path=/, sanitize
    away /./ -> /

v3: Properly handle starting ./ and ending /.

Signed-off-by: Cole Robinson <crobinso redhat com>
---
 src/conf/storage_conf.c      |    8 ++++-
 src/libvirt_private.syms     |    1 +
 src/storage/storage_driver.c |    8 ++++-
 src/util/util.c              |   75 ++++++++++++++++++++++++++++++++++++++++++
 src/util/util.h              |    3 ++
 5 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 9aad081..422e76a 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -602,6 +602,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) {
     xmlNodePtr source_node;
     char *type = NULL;
     char *uuid = NULL;
+    char *tmppath;
 
     if (VIR_ALLOC(ret) < 0) {
         virReportOOMError();
@@ -699,11 +700,16 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt) {
         }
     }
 
-    if ((ret->target.path = virXPathString("string(./target/path)", ctxt)) == NULL) {
+    if ((tmppath = virXPathString("string(./target/path)", ctxt)) == NULL) {
         virStorageReportError(VIR_ERR_XML_ERROR,
                               "%s", _("missing storage pool target path"));
         goto cleanup;
     }
+    ret->target.path = virFileSanitizePath(tmppath);
+    VIR_FREE(tmppath);
+    if (!ret->target.path)
+        goto cleanup;
+
 
     if (virStorageDefParsePerms(ctxt, &ret->target.perms,
                                 "./target/permissions", 0700) < 0)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 1e4bfd0..8b397bf 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -675,6 +675,7 @@ virFileReadLimFD;
 virFilePid;
 virFileReadPid;
 virFileLinkPointsTo;
+virFileSanitizePath;
 virParseNumber;
 virParseVersionString;
 virPipeReadUntilEOF;
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index b148e39..0870f74 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1204,6 +1204,11 @@ storageVolumeLookupByPath(virConnectPtr conn,
     virStorageDriverStatePtr driver = conn->storagePrivateData;
     unsigned int i;
     virStorageVolPtr ret = NULL;
+    char *cleanpath;
+
+    cleanpath = virFileSanitizePath(path);
+    if (!cleanpath)
+        return NULL;
 
     storageDriverLock(driver);
     for (i = 0 ; i < driver->pools.count && !ret ; i++) {
@@ -1213,7 +1218,7 @@ storageVolumeLookupByPath(virConnectPtr conn,
             const char *stable_path;
 
             stable_path = virStorageBackendStablePath(driver->pools.objs[i],
-                                                      path);
+                                                      cleanpath);
             /*
              * virStorageBackendStablePath already does
              * virStorageReportError if it fails; we just need to keep
@@ -1242,6 +1247,7 @@ storageVolumeLookupByPath(virConnectPtr conn,
                               "%s", _("no storage vol with matching path"));
 
 cleanup:
+    VIR_FREE(cleanpath);
     storageDriverUnlock(driver);
     return ret;
 }
diff --git a/src/util/util.c b/src/util/util.c
index 92b9a0f..ca10444 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1921,6 +1921,81 @@ int virFileAbsPath(const char *path, char **abspath)
     return 0;
 }
 
+/* Remove spurious / characters from a path. The result must be freed */
+char *
+virFileSanitizePath(const char *path)
+{
+    const char *cur = path;
+    char *cleanpath;
+    int idx = 0;
+
+    cleanpath = strdup(path);
+    if (!cleanpath) {
+        virReportOOMError();
+        return NULL;
+    }
+
+    /* Need to sanitize:
+     * //           -> //
+     * ///          -> /
+     * /../foo      -> /../foo
+     * /.//foo      -> /foo
+     * /foo///bar/  -> /foo/bar
+     * ./foo/./.    -> /foo
+     */
+
+    /* Starting with // is valid posix, but ///foo == /foo */
+    if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') {
+        idx = 2;
+        cur += 2;
+    }
+
+    /* Sanitize path in place */
+    while (*cur != '\0') {
+        int offset = cur - path;
+
+        /* Copy all dirname characters */
+        if ((cur[0] != '/' && cur[0] != '.') ||
+            (cur[0] == '.' && cur[1] != '/' && cur[1] != '\0')) {
+            cleanpath[idx++] = *cur++;
+            continue;
+        }
+
+        /* Sanitize away / and single . */
+        do {
+            bool slash_follow = (cur[1] == '/');
+            bool slash_before = (offset != 0 && cur[-1] == '/');
+
+            /* Skip all extra / */
+            if (*cur == '/') {
+                cur++;
+                continue;
+            }
+
+            /* Handle starting ./   ending /.   //./   . */
+            if ((slash_follow && slash_before) ||
+                (offset == 0 && slash_follow) ||
+                (slash_before && cur[1] == '\0') ||
+                (offset == 0 && cur[1] == '\0')) {
+                cur++;
+                continue;
+            }
+
+            /* '.' was part of a dirname */
+            break;
+        } while (*cur == '/' || *cur == '.');
+
+        /* Don't add trailing / */
+        if (idx > 0 && (cleanpath[idx-1] == '/' || *cur == '\0'))
+            continue;
+
+        cleanpath[idx++] = '/';
+    }
+    cleanpath[idx] = '\0';
+
+    return cleanpath;
+}
+
 /* Like strtol, but produce an "int" result, and check more carefully.
    Return 0 upon success;  return -1 to indicate failure.
    When END_PTR is NULL, the byte after the final valid digit must be NUL.
diff --git a/src/util/util.h b/src/util/util.h
index 6bf6bcc..302bddf 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -118,6 +118,9 @@ char *virFindFileInPath(const char *file);
 
 int virFileExists(const char *path);
 
+char *virFileSanitizePath(const char *path)
+ATTRIBUTE_NONNULL(1);
+
 enum {
     VIR_FILE_OP_NONE        = 0,
     VIR_FILE_OP_AS_UID      = (1 << 0),
-- 
1.6.6.1


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]