4:21 a.m.
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
4:21 a.m.
New subject: [libvirt] [PATCH 1/2] test: Add tests for virFileSanitizePath
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
4:21 a.m.
New subject: [libvirt] [PATCH 2/2] util: file: Don't carelessly sanitize URIs
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
6:17 a.m.
New subject: [libvirt] [PATCH 2/2] util: file: Don't carelessly sanitize URIs
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
2:51 a.m.
New subject: [libvirt] [PATCH 2/2] util: file: Don't carelessly sanitize URIs
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
3518
days inactive
3519
days old
4 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa