1:52 p.m.
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(a)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