6:28 a.m.
For
https://bugzilla.redhat.com/show_bug.cgi?id=1184131
https://bugzilla.redhat.com/show_bug.cgi?id=1066564
v1: https://www.redhat.com/archives/libvir-list/2015-March/msg01529.html
v2:
* added patches 1-4 that should prevent us from calling
virBufferEscapeString with control codes
* patch 5 is unchanged
Ján Tomko (5):
tests: rename testStripIPv6BracketsData to testStripData
Add functions dealing with control characters in strings
Strip control characters from sysfs attributes
Ignore storage volumes with control codes in their names
Strip control codes in virBufferEscapeString
src/libvirt_private.syms | 2 ++
src/node_device/node_device_udev.c | 2 ++
src/storage/storage_backend_fs.c | 6 +++++
src/util/virbuffer.c | 14 ++++++++---
src/util/virstring.c | 39 ++++++++++++++++++++++++++++++
src/util/virstring.h | 2 ++
tests/virbuftest.c | 49 ++++++++++++++++++++++++++++++++++++++
tests/virstringtest.c | 45 +++++++++++++++++++++++++++++++---
8 files changed, 153 insertions(+), 6 deletions(-)
--
2.0.5
6:28 a.m.
New subject: [libvirt] [PATCHv2 1/5] tests: rename testStripIPv6BracketsData to testStripData
For reuse with other Strip* functions.
---
tests/virstringtest.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tests/virstringtest.c b/tests/virstringtest.c
index a0bfd61..9d0b438 100644
--- a/tests/virstringtest.c
+++ b/tests/virstringtest.c
@@ -522,14 +522,14 @@ testVirStringFreeListCount(const void *opaque ATTRIBUTE_UNUSED)
}
-struct testStripIPv6BracketsData {
+struct testStripData {
const char *string;
const char *result;
};
static int testStripIPv6Brackets(const void *args)
{
- const struct testStripIPv6BracketsData *data = args;
+ const struct testStripData *data = args;
int ret = -1;
char *res = NULL;
@@ -766,7 +766,7 @@ mymain(void)
#define TEST_STRIP_IPV6_BRACKETS(str, res) \
do { \
- struct testStripIPv6BracketsData stripData = { \
+ struct testStripData stripData = { \
.string = str, \
.result = res, \
}; \
--
2.0.5
3:59 a.m.
New subject: [libvirt] [PATCHv2 1/5] tests: rename testStripIPv6BracketsData to testStripData
On Tue, Apr 14, 2015 at 13:28:46 +0200, Ján Tomko wrote:
For reuse with other Strip* functions.
---
tests/virstringtest.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
ACK,
Peter
6:28 a.m.
New subject: [libvirt] [PATCHv2 2/5] Add functions dealing with control characters in strings
Add virStringHasControlChars that checks if the string has
any control characters other than \t\r\n,
and virStringStripControlChars that removes them in-place.
---
src/libvirt_private.syms | 2 ++
src/util/virstring.c | 39 +++++++++++++++++++++++++++++++++++++++
src/util/virstring.h | 2 ++
tests/virstringtest.c | 39 +++++++++++++++++++++++++++++++++++++++
4 files changed, 82 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7166283..d37a6b5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2134,6 +2134,7 @@ virStrdup;
virStringArrayHasString;
virStringFreeList;
virStringFreeListCount;
+virStringHasControlChars;
virStringIsEmpty;
virStringJoin;
virStringListLength;
@@ -2143,6 +2144,7 @@ virStringSortCompare;
virStringSortRevCompare;
virStringSplit;
virStringSplitCount;
+virStringStripControlChars;
virStringStripIPv6Brackets;
virStrncpy;
virStrndup;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 3dad9dd..9eb48b5 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -968,3 +968,42 @@ virStringStripIPv6Brackets(char *str)
str[len - 2] = '\0';
}
}
+
+static const char control_chars[] =
+ "\x01\x02\x03\x04\x05\x06\x07"
+ "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F"
+ "\x10\x11\x12\x13\x14\x15\x16\x17"
+ "\x18\x19\x1A\x1B\x1C\x1D\x1E\x1F";
+
+bool
+virStringHasControlChars(const char *str)
+{
+ if (!str)
+ return false;
+
+ return strcspn(str, control_chars) != strlen(str);
+}
+/**
+ * virStringStripControlChars:
+ * @str: the string to strip
+ *
+ * Modify the string in-place to remove the control characters
+ * in the interval: (0x01, 0x20)
+ */
+void
+virStringStripControlChars(char *str)
+{
+ size_t len, i, j;
+
+ if (!virStringHasControlChars(str))
+ return;
+
+ len = strlen(str);
+ for (i = 0, j = 0; i < len; i++) {
+ if (index(control_chars, str[i]))
+ continue;
+
+ str[j++] = str[i];
+ }
+ str[j] = '\0';
+}
diff --git a/src/util/virstring.h b/src/util/virstring.h
index 2ec60fa..e6dcb32 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -271,5 +271,7 @@ char *virStringReplace(const char *haystack,
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
void virStringStripIPv6Brackets(char *str);
+bool virStringHasControlChars(const char *str);
+void virStringStripControlChars(char *str);
#endif /* __VIR_STRING_H__ */
diff --git a/tests/virstringtest.c b/tests/virstringtest.c
index 9d0b438..38d0126 100644
--- a/tests/virstringtest.c
+++ b/tests/virstringtest.c
@@ -551,6 +551,29 @@ static int testStripIPv6Brackets(const void *args)
return ret;
}
+static int testStripControlChars(const void *args)
+{
+ const struct testStripData *data = args;
+ int ret = -1;
+ char *res = NULL;
+
+ if (VIR_STRDUP(res, data->string) < 0)
+ goto cleanup;
+
+ virStringStripControlChars(res);
+
+ if (STRNEQ_NULLABLE(res, data->result)) {
+ fprintf(stderr, "Returned '%s', expected '%s'\n",
+ NULLSTR(res), NULLSTR(data->result));
+ goto cleanup;
+ }
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(res);
+ return ret;
+}
static int
mymain(void)
@@ -783,6 +806,22 @@ mymain(void)
TEST_STRIP_IPV6_BRACKETS(":hello]", ":hello]");
TEST_STRIP_IPV6_BRACKETS(":[]:", ":[]:");
+#define TEST_STRIP_CONTROL_CHARS(str, res) \
+ do { \
+ struct testStripData stripData = { \
+ .string = str, \
+ .result = res, \
+ }; \
+ if (virtTestRun("Strip control chars from " #str, \
+ testStripControlChars, &stripData) < 0) \
+ ret = -1; \
+ } while (0)
+
+ TEST_STRIP_CONTROL_CHARS(NULL, NULL);
+ TEST_STRIP_CONTROL_CHARS("\nhello \r hello\t", "\nhello \r
hello\t");
+ TEST_STRIP_CONTROL_CHARS("\x01H\x02" "E\x03L\x04L\x05O",
"HELLO");
+ TEST_STRIP_CONTROL_CHARS("\x01\x02\x03\x04HELL\x05O", "HELLO");
+ TEST_STRIP_CONTROL_CHARS("\nhello \x01\x07hello\t", "\nhello
hello\t");
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
2.0.5
5 a.m.
New subject: [libvirt] [PATCHv2 2/5] Add functions dealing with control characters in strings
On Tue, Apr 14, 2015 at 13:28:47 +0200, Ján Tomko wrote:
Add virStringHasControlChars that checks if the string has
any control characters other than \t\r\n,
and virStringStripControlChars that removes them in-place.
---
src/libvirt_private.syms | 2 ++
src/util/virstring.c | 39 +++++++++++++++++++++++++++++++++++++++
src/util/virstring.h | 2 ++
tests/virstringtest.c | 39 +++++++++++++++++++++++++++++++++++++++
4 files changed, 82 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 7166283..d37a6b5 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2134,6 +2134,7 @@ virStrdup;
virStringArrayHasString;
virStringFreeList;
virStringFreeListCount;
+virStringHasControlChars;
virStringIsEmpty;
virStringJoin;
virStringListLength;
@@ -2143,6 +2144,7 @@ virStringSortCompare;
virStringSortRevCompare;
virStringSplit;
virStringSplitCount;
+virStringStripControlChars;
virStringStripIPv6Brackets;
virStrncpy;
virStrndup;
diff --git a/src/util/virstring.c b/src/util/virstring.c
index 3dad9dd..9eb48b5 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -968,3 +968,42 @@ virStringStripIPv6Brackets(char *str)
str[len - 2] = '\0';
}
}
+
+static const char control_chars[] =
+ "\x01\x02\x03\x04\x05\x06\x07"
+ "\x08" /* \t \n */ "\x0B\x0C" /* \r */ "\x0E\x0F"
The alignment is off
+ "\x10\x11\x12\x13\x14\x15\x16\x17"
+ "\x18\x19\x1A\x1B\x1C\x1D\x1E\x1F";
+
+bool
+virStringHasControlChars(const char *str)
+{
+ if (!str)
+ return false;
+
+ return strcspn(str, control_chars) != strlen(str);
+}
Missing newlines between function and comment.
+/**
+ * virStringStripControlChars:
+ * @str: the string to strip
+ *
+ * Modify the string in-place to remove the control characters
+ * in the interval: (0x01, 0x20)
The interval is more like <0x01, 0x20).
+ */
+void
+virStringStripControlChars(char *str)
+{
+ size_t len, i, j;
+
+ if (!virStringHasControlChars(str))
+ return;
The check above calls strlen and iterates through the string looking for
the characters.
+
+ len = strlen(str);
+ for (i = 0, j = 0; i < len; i++) {
+ if (index(control_chars, str[i]))
+ continue;
Here you iterate through the string and check for the characters ... I
think it should be safe to just execute this function right away.
+
+ str[j++] = str[i];
+ }
+ str[j] = '\0';
+}
The rest looks good.
ACK with the nits above addressed.
Peter
10:27 a.m.
New subject: [libvirt] [PATCHv2 2/5] Add functions dealing with control characters in strings
On Wed, Apr 15, 2015 at 12:00:51PM +0200, Peter Krempa wrote:
On Tue, Apr 14, 2015 at 13:28:47 +0200, Ján Tomko wrote:
> Add virStringHasControlChars that checks if the string has
> any control characters other than \t\r\n,
> and virStringStripControlChars that removes them in-place.
> ---
> src/libvirt_private.syms | 2 ++
> src/util/virstring.c | 39 +++++++++++++++++++++++++++++++++++++++
> src/util/virstring.h | 2 ++
> tests/virstringtest.c | 39 +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 82 insertions(+)
> +
> +bool
> +virStringHasControlChars(const char *str)
> +{
> + if (!str)
> + return false;
> +
> + return strcspn(str, control_chars) != strlen(str);
> +}
> + */
> +void
> +virStringStripControlChars(char *str)
> +{
> + size_t len, i, j;
> +
> + if (!virStringHasControlChars(str))
> + return;
The check above calls strlen and iterates through the string looking for
the characters.
Right, the check can do just:
return str[strcspn(str, control_chars)] != '\0';
Jan
6:28 a.m.
New subject: [libvirt] [PATCHv2 3/5] Strip control characters from sysfs attributes
Including them in the XML makes them unparsable.
https://bugzilla.redhat.com/show_bug.cgi?id=1184131
---
src/node_device/node_device_udev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 8c39e5f..5e98222 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -239,6 +239,8 @@ static int udevGetStringSysfsAttr(struct udev_device *udev_device,
ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &tmp);
+ virStringStripControlChars(tmp);
+
if (tmp != NULL && (STREQ(tmp, ""))) {
VIR_FREE(tmp);
tmp = NULL;
--
2.0.5
7:37 a.m.
New subject: [libvirt] [PATCHv2 3/5] Strip control characters from sysfs attributes
On Tue, Apr 14, 2015 at 13:28:48 +0200, Ján Tomko wrote:
Including them in the XML makes them unparsable.
https://bugzilla.redhat.com/show_bug.cgi?id=1184131
---
src/node_device/node_device_udev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 8c39e5f..5e98222 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -239,6 +239,8 @@ static int udevGetStringSysfsAttr(struct udev_device *udev_device,
ret = udevGetDeviceSysfsAttr(udev_device, attr_name, &tmp);
+ virStringStripControlChars(tmp);
+
Looks safe enoguh to me, but I'd state in a comment that this function
strips possible control chars.
if (tmp != NULL && (STREQ(tmp, ""))) {
VIR_FREE(tmp);
tmp = NULL;
ACK with comment added.
Peter
6:28 a.m.
New subject: [libvirt] [PATCHv2 4/5] Ignore storage volumes with control codes in their names
To prevent generating invalid XML.
https://bugzilla.redhat.com/show_bug.cgi?id=1066564
---
src/storage/storage_backend_fs.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index d4d65bc..fb33f08 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -860,6 +860,12 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn
ATTRIBUTE_UNUSED,
while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) {
int ret;
+ if (virStringHasControlChars(ent->d_name)) {
+ VIR_WARN("Ignoring file with control characters in its name:
'%s'",
+ ent->d_name);
+ continue;
+ }
+
if (VIR_ALLOC(vol) < 0)
goto error;
--
2.0.5
7:19 a.m.
New subject: [libvirt] [PATCHv2 4/5] Ignore storage volumes with control codes in their names
On Tue, Apr 14, 2015 at 13:28:49 +0200, Ján Tomko wrote:
To prevent generating invalid XML.
https://bugzilla.redhat.com/show_bug.cgi?id=1066564
---
src/storage/storage_backend_fs.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index d4d65bc..fb33f08 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -860,6 +860,12 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn
ATTRIBUTE_UNUSED,
while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) {
int ret;
+ if (virStringHasControlChars(ent->d_name)) {
+ VIR_WARN("Ignoring file with control characters in its name:
'%s'",
+ ent->d_name);
I'm not quite sure that printing the string containing control
characters to the log file is a good idea. While journalctl probably
will handle the string properly, classic text logs may cause problems
then.
I'd rather log the pool name or directory path or something else than
the offending name itself.
+ continue;
+ }
+
if (VIR_ALLOC(vol) < 0)
goto error;
Peter
6:28 a.m.
New subject: [libvirt] [PATCHv2 5/5] Strip control codes in virBufferEscapeString
These cannot be represented in XML.
We have been stripping them, but only if the string had
characters that needed escaping: <>"'&
Extend the strcspn check to include control codes, and strip
them even if we don't do any escaping.
https://bugzilla.redhat.com/show_bug.cgi?id=1184131
https://bugzilla.redhat.com/show_bug.cgi?id=1066564
---
src/util/virbuffer.c | 14 +++++++++++---
tests/virbuftest.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 3 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
index 706dbfa..3d13c90 100644
--- a/src/util/virbuffer.c
+++ b/src/util/virbuffer.c
@@ -438,6 +438,13 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const
char *str)
int len;
char *escaped, *out;
const char *cur;
+ const char forbidden_characters[] = {
+ 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
+ /*\t*/ /*\n*/ 0x0B, 0x0C, /*\r*/ 0x0E, 0x0F, 0x10,
+ 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18,
+ 0x19, '"', '&', '\'',
'<', '>',
+ '\0'
+ };
if ((format == NULL) || (buf == NULL) || (str == NULL))
return;
@@ -446,7 +453,7 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char
*str)
return;
len = strlen(str);
- if (strcspn(str, "<>&'\"") == len) {
+ if (strcspn(str, forbidden_characters) == len) {
virBufferAsprintf(buf, format, str);
return;
}
@@ -490,8 +497,7 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char
*str)
*out++ = 'o';
*out++ = 's';
*out++ = ';';
- } else if (((unsigned char)*cur >= 0x20) || (*cur == '\n') || (*cur ==
'\t') ||
- (*cur == '\r')) {
+ } else if (!strchr(forbidden_characters, *cur)) {
/*
* default case, just copy !
* Note that character over 0x80 are likely to give problem
@@ -499,6 +505,8 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char
*str)
* it's hard to handle properly we have to assume it's UTF-8 too
*/
*out++ = *cur;
+ } else {
+ /* silently ignore control characters */
}
cur++;
}
diff --git a/tests/virbuftest.c b/tests/virbuftest.c
index 21cb18b..10398d5 100644
--- a/tests/virbuftest.c
+++ b/tests/virbuftest.c
@@ -349,6 +349,39 @@ testBufAddStr(const void *opaque ATTRIBUTE_UNUSED)
static int
+testBufEscapeStr(const void *opaque ATTRIBUTE_UNUSED)
+{
+ const struct testBufAddStrData *data = opaque;
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ char *actual;
+ int ret = -1;
+
+ virBufferAddLit(&buf, "<c>\n");
+ virBufferAdjustIndent(&buf, 2);
+ virBufferEscapeString(&buf, "<el>%s</el>\n",
data->data);
+ virBufferAdjustIndent(&buf, -2);
+ virBufferAddLit(&buf, "</c>");
+
+ if (!(actual = virBufferContentAndReset(&buf))) {
+ TEST_ERROR("buf is empty");
+ goto cleanup;
+ }
+
+ if (STRNEQ_NULLABLE(actual, data->expect)) {
+ TEST_ERROR("testBufEscapeStr(): Strings don't match:\n");
+ virtTestDifference(stderr, data->expect, actual);
+ goto cleanup;
+ }
+
+ ret = 0;
+
+ cleanup:
+ VIR_FREE(actual);
+ return ret;
+}
+
+
+static int
mymain(void)
{
int ret = 0;
@@ -379,6 +412,22 @@ mymain(void)
DO_TEST_ADD_STR("<a/>\n", "<c>\n
<a/>\n</c>");
DO_TEST_ADD_STR("<b>\n <a/>\n</b>\n", "<c>\n
<b>\n <a/>\n </b>\n</c>");
+#define DO_TEST_ESCAPE(data, expect) \
+ do { \
+ struct testBufAddStrData info = { data, expect }; \
+ if (virtTestRun("Buf: EscapeStr", testBufEscapeStr, &info) < 0)
\
+ ret = -1; \
+ } while (0)
+
+ DO_TEST_ESCAPE("<td></td><td></td>",
+ "<c>\n
<el><td></td><td></td></el>\n</c>");
+ DO_TEST_ESCAPE("\007\"&&\"\x15",
+ "<c>\n
<el>"&&"</el>\n</c>");
+ DO_TEST_ESCAPE(",,'..',,",
+ "<c>\n
<el>,,'..',,</el>\n</c>");
+ DO_TEST_ESCAPE("\x01\x01\x02\x03\x05\x08",
+ "<c>\n <el></el>\n</c>");
+
return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE;
}
--
2.0.5
7:29 a.m.
New subject: [libvirt] [PATCHv2 5/5] Strip control codes in virBufferEscapeString
On Tue, Apr 14, 2015 at 13:28:50 +0200, Ján Tomko wrote:
These cannot be represented in XML.
We have been stripping them, but only if the string had
characters that needed escaping: <>"'&
Extend the strcspn check to include control codes, and strip
them even if we don't do any escaping.
https://bugzilla.redhat.com/show_bug.cgi?id=1184131
https://bugzilla.redhat.com/show_bug.cgi?id=1066564
---
src/util/virbuffer.c | 14 +++++++++++---
tests/virbuftest.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+), 3 deletions(-)
ACK,
Peter
3509
days inactive
3510
days old
11 comments
2 participants
participants (2)
-
Ján Tomko -
Peter Krempa