[libvirt] [PATCH v2] 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 /./ -> / 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 | 49 ++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 3 ++ 5 files changed, 67 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 1594a08..2c74b08 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -667,6 +667,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..b141406 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1921,6 +1921,55 @@ 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; + } + + /* Starting with // is valid posix, but ///foo == /foo */ + if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') { + cleanpath[0] = '/'; + cleanpath[1] = '/'; + idx = 2; + cur += 2; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + if (*cur != '/') { + cleanpath[idx++] = *cur++; + continue; + } + + /* Skip all extra / */ + while (*cur == '/') { + cur++; + + /* Resolve away /./ to just / */ + if (cur[0] == '.' && cur[1] == '/') + cur++; + } + + /* Don't add a trailing / unless path is only made of / */ + if (idx != 0 && *cur == '\0') + break; + + 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
On 05/21/2010 11:05 AM, Cole Robinson wrote:
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 /./ -> /
Nice - you caught one case that my review did not ("/" as the entire path).
+/* 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; + } + + /* Starting with // is valid posix, but ///foo == /foo */ + if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') { + cleanpath[0] = '/'; + cleanpath[1] = '/';
Delete these two lines; since cleanpath was created by strdup(), they are redundant assignments.
+ idx = 2; + cur += 2; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + if (*cur != '/') { + cleanpath[idx++] = *cur++; + continue; + } + + /* Skip all extra / */ + while (*cur == '/') { + cur++; + + /* Resolve away /./ to just / */ + if (cur[0] == '.' && cur[1] == '/') + cur++; + }
That doesn't simplify "./a" to "a", nor "b/." to "b"
+ + /* Don't add a trailing / unless path is only made of / */ + if (idx != 0 && *cur == '\0') + break;
This incorrectly collapses plain "//" to "/".
+ + cleanpath[idx++] = '/'; + }
Maybe a better approach is this pseudo-code: if (2 leading slashes) { advance 2 if (end) return } else if (leading slash) advance 1 do { advance past all slashes - they are redundant from previous round if (./ or .-end) advance 1 and continue copy all non-slashes copy one slash } while (!end) if (trailing slash) back up 1
+ cleanpath[idx] = '\0'; + + return cleanpath; +} +
-- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/21/2010 02:33 PM, Eric Blake wrote:
On 05/21/2010 11:05 AM, Cole Robinson wrote:
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 /./ -> /
Nice - you caught one case that my review did not ("/" as the entire path).
+/* 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; + } + + /* Starting with // is valid posix, but ///foo == /foo */ + if (cur[0] == '/' && cur[1] == '/' && cur[2] != '/') { + cleanpath[0] = '/'; + cleanpath[1] = '/';
Delete these two lines; since cleanpath was created by strdup(), they are redundant assignments.
Will do
+ idx = 2; + cur += 2; + } + + /* Sanitize path in place */ + while (*cur != '\0') { + if (*cur != '/') { + cleanpath[idx++] = *cur++; + continue; + } + + /* Skip all extra / */ + while (*cur == '/') { + cur++; + + /* Resolve away /./ to just / */ + if (cur[0] == '.' && cur[1] == '/') + cur++; + }
That doesn't simplify "./a" to "a", nor "b/." to "b"
Argh, good point.
+ + /* Don't add a trailing / unless path is only made of / */ + if (idx != 0 && *cur == '\0') + break;
This incorrectly collapses plain "//" to "/".
Actually we never entered the while() loop with a path of just //, so this worked in practice.
+ + cleanpath[idx++] = '/'; + }
Maybe a better approach is this pseudo-code:
if (2 leading slashes) { advance 2 if (end) return } else if (leading slash) advance 1 do { advance past all slashes - they are redundant from previous round if (./ or .-end) advance 1 and continue copy all non-slashes copy one slash } while (!end) if (trailing slash) back up 1
I didn't end up going with this configuration, but the resulting code isn't too bad, and seems to cover all the tests I could think up. Updated patch sending shortly. Thanks, Cole
participants (2)
-
Cole Robinson -
Eric Blake