[libvirt] [PATCH 0/2] Fix virVolLookupByPath error msg with pools that use URI as path
Peter Krempa (2): test: Add tests for virFileSanitizePath util: file: Don't carelessly sanitize URIs src/util/virfile.c | 6 ++++++ tests/virfiletest.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) -- 2.2.2
Add test infrastructure for virFileSanitizePath so that it can be sensibly refactored later. --- tests/virfiletest.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/tests/virfiletest.c b/tests/virfiletest.c index 6c619df..826b2b9 100644 --- a/tests/virfiletest.c +++ b/tests/virfiletest.c @@ -89,10 +89,40 @@ static int testFileGetMountSubtree(const void *opaque) } #endif /* ! defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R */ +struct testFileSanitizePathData +{ + const char *path; + const char *expect; +}; + +static int +testFileSanitizePath(const void *opaque) +{ + const struct testFileSanitizePathData *data = opaque; + int ret = -1; + char *actual; + + if (!(actual = virFileSanitizePath(data->path))) + return -1; + + if (STRNEQ(actual, data->expect)) { + fprintf(stderr, "\nexpect: '%s'\nactual: '%s'\n", data->expect, actual); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(actual); + return ret; +} + + static int mymain(void) { int ret = 0; + struct testFileSanitizePathData data1; #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R # define MTAB_PATH1 abs_srcdir "/virfiledata/mounts1.txt" @@ -126,6 +156,29 @@ mymain(void) DO_TEST_MOUNT_SUBTREE("/etc/aliases.db", MTAB_PATH2, "/etc/aliases.db", wantmounts2b, false); #endif /* ! defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R */ +#define DO_TEST_SANITIZE_PATH(PATH, EXPECT) \ + do { \ + data1.path = PATH; \ + data1.expect = EXPECT; \ + if (virtTestRun(virtTestCounterNext(), testFileSanitizePath, \ + &data1) < 0) \ + ret = -1; \ + } while (0) + + virtTestCounterReset("testFileSanitizePath "); + DO_TEST_SANITIZE_PATH("", ""); + DO_TEST_SANITIZE_PATH("/", "/"); + DO_TEST_SANITIZE_PATH("/path", "/path"); + DO_TEST_SANITIZE_PATH("/path/to/blah", "/path/to/blah"); + DO_TEST_SANITIZE_PATH("/path/", "/path"); + DO_TEST_SANITIZE_PATH("///////", "/"); + DO_TEST_SANITIZE_PATH("//", "//"); + DO_TEST_SANITIZE_PATH(".", "."); + DO_TEST_SANITIZE_PATH("../", ".."); + DO_TEST_SANITIZE_PATH("../../", "../.."); + DO_TEST_SANITIZE_PATH("//foo//bar", "//foo/bar"); + DO_TEST_SANITIZE_PATH("/bar//foo", "/bar/foo"); + return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS; } -- 2.2.2
rfc3986 states that the separator in URI path is a single slash. Multiple slashes may potentially lead to different resources and thus we should not remove them. --- src/util/virfile.c | 6 ++++++ tests/virfiletest.c | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/src/util/virfile.c b/src/util/virfile.c index c528a1c..87d121d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2812,12 +2812,18 @@ char * virFileSanitizePath(const char *path) { const char *cur = path; + char *uri; char *cleanpath; int idx = 0; if (VIR_STRDUP(cleanpath, path) < 0) return NULL; + /* don't sanitize URIs - rfc3986 states that two slashes may lead to a + * different resource, thus removing them would possibly change the path */ + if ((uri = strstr(path, "://")) && strchr(path, '/') > uri) + return cleanpath; + /* Need to sanitize: * // -> // * /// -> / diff --git a/tests/virfiletest.c b/tests/virfiletest.c index 826b2b9..628fa1f 100644 --- a/tests/virfiletest.c +++ b/tests/virfiletest.c @@ -165,6 +165,8 @@ mymain(void) ret = -1; \ } while (0) +#define DO_TEST_SANITIZE_PATH_SAME(PATH) DO_TEST_SANITIZE_PATH(PATH, PATH) + virtTestCounterReset("testFileSanitizePath "); DO_TEST_SANITIZE_PATH("", ""); DO_TEST_SANITIZE_PATH("/", "/"); @@ -178,6 +180,11 @@ mymain(void) DO_TEST_SANITIZE_PATH("../../", "../.."); DO_TEST_SANITIZE_PATH("//foo//bar", "//foo/bar"); DO_TEST_SANITIZE_PATH("/bar//foo", "/bar/foo"); + DO_TEST_SANITIZE_PATH_SAME("gluster://bar.baz/foo/hoo"); + DO_TEST_SANITIZE_PATH_SAME("gluster://bar.baz//fooo/hoo"); + DO_TEST_SANITIZE_PATH_SAME("gluster://bar.baz//////fooo/hoo"); + DO_TEST_SANITIZE_PATH_SAME("gluster://bar.baz/fooo//hoo"); + DO_TEST_SANITIZE_PATH_SAME("gluster://bar.baz/fooo///////hoo"); return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS; } -- 2.2.2
On Wed, Apr 08, 2015 at 11:21:59AM +0200, Peter Krempa wrote:
rfc3986 states that the separator in URI path is a single slash. Multiple slashes may potentially lead to different resources and thus we should not remove them. --- src/util/virfile.c | 6 ++++++ tests/virfiletest.c | 7 +++++++ 2 files changed, 13 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c index c528a1c..87d121d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2812,12 +2812,18 @@ char * virFileSanitizePath(const char *path) { const char *cur = path; + char *uri; char *cleanpath; int idx = 0;
if (VIR_STRDUP(cleanpath, path) < 0) return NULL;
+ /* don't sanitize URIs - rfc3986 states that two slashes may lead to a + * different resource, thus removing them would possibly change the path */ + if ((uri = strstr(path, "://")) && strchr(path, '/') > uri) + return cleanpath; +
It took me a while to understand this condition, but I don't know how to write it more simply. ACK to both. Jan
On Wed, Apr 08, 2015 at 13:17:49 +0200, Ján Tomko wrote:
On Wed, Apr 08, 2015 at 11:21:59AM +0200, Peter Krempa wrote:
rfc3986 states that the separator in URI path is a single slash. Multiple slashes may potentially lead to different resources and thus we should not remove them. --- src/util/virfile.c | 6 ++++++ tests/virfiletest.c | 7 +++++++ 2 files changed, 13 insertions(+)
diff --git a/src/util/virfile.c b/src/util/virfile.c index c528a1c..87d121d 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2812,12 +2812,18 @@ char * virFileSanitizePath(const char *path) { const char *cur = path; + char *uri; char *cleanpath; int idx = 0;
if (VIR_STRDUP(cleanpath, path) < 0) return NULL;
+ /* don't sanitize URIs - rfc3986 states that two slashes may lead to a + * different resource, thus removing them would possibly change the path */ + if ((uri = strstr(path, "://")) && strchr(path, '/') > uri) + return cleanpath; +
It took me a while to understand this condition, but I don't know how to write it more simply.
ACK to both.
Pushed; Thanks. Peter
participants (2)
-
Ján Tomko -
Peter Krempa