On 11/19/2010 03:57 PM, Eric Blake wrote:
On 11/19/2010 09:15 AM, Cole Robinson wrote:
If we don't escape ' or \ xend can't parse the generated sexpr. This might over apply the EscapeSexpr routine, but it shouldn't hurt.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/buf.c | 79 +++++++++++++++++++ src/util/buf.h | 1 + src/xen/xend_internal.c | 115 +++++++++++++++------------- tests/xml2sexprdata/xml2sexpr-escape.sexpr | 1 + tests/xml2sexprdata/xml2sexpr-escape.xml | 24 ++++++ tests/xml2sexprtest.c | 1 + 6 files changed, 169 insertions(+), 52 deletions(-) create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.xml
diff --git a/src/util/buf.c b/src/util/buf.c index 553e2a0..90034ad 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -379,6 +379,85 @@ err: }
/** + * virBufferEscapeSexpr: + * @buf: the buffer to dump + * @format: a printf like format string but with only one %s parameter + * @str: the string argument which need to be escaped + * + * Do a formatted print with a single string to an sexpr buffer. The string + * is escaped to avoid generating a sexpr that xen will choke on. This + * doesn't fully escape the sexpr, just enough for our code to work. + */ +void +virBufferEscapeSexpr(const virBufferPtr buf, + const char *format, + const char *str) +{ + int size, count, len, grow_size; + char *escaped, *out; + const char *cur; + + if ((format == NULL) || (buf == NULL) || (str == NULL)) + return; + + if (buf->error) + return; + + len = strlen(str);
Right here, is it worth adding:
if (strcspn(str, "\\'") == len) { virBufferVSprintf(buf, format, str); return; }
+ if (VIR_ALLOC_N(escaped, 2 * len + 1) < 0) { + virBufferNoMemory(buf); + return; + }
so as to avoid the alloc in the common case of nothing to escape?
+ *out = 0; + + if ((buf->use >= buf->size) && + virBufferGrow(buf, 100) < 0) { + goto err; + }
That 100 looks wrong; shouldn't it instead be max(100,out-escaped)? For that matter, why do all this low level stuff; wouldn't it be easier after *out = 0 to just do:
virBufferVSpring(buf, format, escaped); VIR_FREE(escaped); return;
and leave all the resizing and snprintf stuff to the already written function?
This function is basically a copy of virBufferEscapeString. I'll add a patch to cleanup the original function before duplicating.
diff --git a/src/util/buf.h b/src/util/buf.h index 6616898..54f4de5 100644 --- a/src/util/buf.h @@ -5400,39 +5401,42 @@ xenDaemonFormatSxprDisk(virConnectPtr conn ATTRIBUTE_UNUSED,
if (hvm) { /* Xend <= 3.0.2 wants a ioemu: prefix on devices for HVM */ - if (xendConfigVersion == 1) - virBufferVSprintf(buf, "(dev 'ioemu:%s')", def->dst); - else /* But newer does not */ - virBufferVSprintf(buf, "(dev '%s:%s')", def->dst, - def->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? - "cdrom" : "disk"); + if (xendConfigVersion == 1) { + virBufferEscapeSexpr(buf, "(dev 'ioemu:%s')", def->dst); + } else { + /* But newer does not */ + virBufferEscapeSexpr(buf, "(dev '%s:", def->dst); + virBufferEscapeSexpr(buf, "%s')", + def->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? + "cdrom" : "disk");
This can be virBufferVSprintf(buf, "%s')",...), given that the only two possible strings for %s don't need escaping.
@@ -5680,7 +5685,8 @@ xenDaemonFormatSxprSound(virDomainDefPtr def, def->sounds[i]->model); return -1; } - virBufferVSprintf(buf, "%s%s", i ? "," : "", str); + virBufferVSprintf(buf, "%s", i ? "," : "");
I'd write this as 'if (i) virBufferAddChar(buf, ',')' rather than a complicated VSprintf.
Okay, i'll fold in these changes.
diff --git a/tests/xml2sexprdata/xml2sexpr-escape.xml b/tests/xml2sexprdata/xml2sexpr-escape.xml new file mode 100644 index 0000000..73beb6b --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-escape.xml @@ -0,0 +1,24 @@ +<domain type='xen' id='15'> + <name>fvtest</name> + <uuid>596a5d2171f48fb2e068e2386a5c413e</uuid> + <os> + <type>hvm</type> + <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> + <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> + <cmdline> method=http://&""download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os </cmdline>
Seems unusual to have & and " in a URL; but the point of this test was not so much a real configuration as a way to expose the sexpr escaping code path. Is there any better place to stick this where it won't be quite so confusing?
Actually the BZ prompting this work was for a URL with & in it: https://bugzilla.redhat.com/show_bug.cgi?id=472437 I'll format the URL in a more sensible way though. Thanks, Cole