Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/util/conf.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/util/conf.c b/src/util/conf.c index ba1a384..a31bbc4 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -394,17 +394,20 @@ virConfParseString(virConfParserCtxtPtr ctxt) return NULL; } NEXT; - } else if ((ctxt->cur + 6 < ctxt->end) && (ctxt->cur[0] == '"') && - (ctxt->cur[1] == '"') && (ctxt->cur[2] == '"')) { + } else if ((ctxt->cur + 6 < ctxt->end) && + (STRPREFIX(ctxt->cur, "\"\"\""))) { + /* String starts with python-style triple quotes """ */ ctxt->cur += 3; base = ctxt->cur; - while ((ctxt->cur + 2 < ctxt->end) && (ctxt->cur[0] == '"') && - (ctxt->cur[1] == '"') && (ctxt->cur[2] == '"')) { - if (CUR == '\n') ctxt->line++; - NEXT; + + while ((ctxt->cur + 2 < ctxt->end) && + (STRPREFIX(ctxt->cur, "\"\"\""))) { + if (CUR == '\n') + ctxt->line++; + NEXT; } - if ((ctxt->cur[0] != '"') || (ctxt->cur[1] != '"') || - (ctxt->cur[2] != '"')) { + + if (!STRPREFIX(ctxt->cur, "\"\"\"")) { virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("unterminated string")); return(NULL); } -- 1.7.3.2

On 11/19/2010 03:00 PM, Eric Blake wrote: Yeah, it didn't work :) See patch 2 for that bug fix, I just didn't want to refactor and change functionality in 1 patch. - Cole

On 11/19/2010 09:15 AM, Cole Robinson wrote: So far, so good... but given that a devicePath can't have '.', should it really be allowed to have other characters like &, ", ', <, or >? Likewise for deviceName. Ah, the bug fix for patch 1. ACK to patch 1, then. Hmm; this really is a case of deviceName in the domain.rng schema. Are there really devices named with ' or " in the name? If so, then ACK to patch 2. If not, then it would be better to use <disk type='file'><source file='/.../XenGuest&apos;&quot;'/>, since that would pick up on absFilePath, which is more likely to match reality of having ' or " in the name. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

Since we send the sexpr to xend via HTTP, we need to properly escape '&' Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; --- src/xen/xend_internal.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d6d66bd..3ccadde 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -710,6 +710,7 @@ urlencode(const char *string) switch (string[i]) { case ' ': case '\n': + case '&': snprintf(ptr, 4, "%%%02x", string[i]); ptr += 3; break; -- 1.7.3.2

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(a)redhat.com&gt; --- 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); + if (VIR_ALLOC_N(escaped, 2 * len + 1) < 0) { + virBufferNoMemory(buf); + return; + } + + cur = str; + out = escaped; + while (*cur != 0) { + switch (*cur) { + case '\\': + case '\'': + *out++ = '\\'; + /* fallthrough */ + default: + *out++ = *cur; + } + cur++; + } + *out = 0; + + if ((buf->use >= buf->size) && + virBufferGrow(buf, 100) < 0) { + goto err; + } + + size = buf->size - buf->use; + if ((count = snprintf(&buf->content[buf->use], size, + format, (char *)escaped)) < 0) { + buf->error = 1; + goto err; + } + + /* Grow buffer if necessary and retry */ + if (count >= size) { + buf->content[buf->use] = 0; + grow_size = (count + 1 > 1000) ? count + 1 : 1000; + if (virBufferGrow(buf, grow_size) < 0) { + goto err; + } + size = buf->size - buf->use; + + if ((count = snprintf(&buf->content[buf->use], size, + format, (char *)escaped)) < 0) { + buf->error = 1; + goto err; + } + } + buf->use += count; + +err: + VIR_FREE(escaped); +} + +/** * virBufferURIEncodeString: * @buf: the buffer to append to * @str: the string argument which will be URI-encoded diff --git a/src/util/buf.h b/src/util/buf.h index 6616898..54f4de5 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -45,6 +45,7 @@ void virBufferVSprintf(const virBufferPtr buf, const char *format, ...) void virBufferStrcat(const virBufferPtr buf, ...) ATTRIBUTE_SENTINEL; void virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str); +void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str); void virBufferURIEncodeString (const virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 3ccadde..7a36fb0 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -522,6 +522,7 @@ xend_op_ext(virConnectPtr xend, const char *path, const char *key, va_list ap) } content = virBufferContentAndReset(&buf); + DEBUG("xend op: %s\n", content); ret = http2unix(xend_post(xend, path, content)); VIR_FREE(content); @@ -4605,8 +4606,6 @@ virDomainPtr xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc) { goto error; } - DEBUG("Defining w/ sexpr: \n%s", sexpr); - ret = xend_op(conn, "", "op", "new", "config", sexpr, NULL); VIR_FREE(sexpr); if (ret != 0) { @@ -5297,11 +5296,12 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferVSprintf(buf, "%s:%s", type, def->data.file.path); + virBufferVSprintf(buf, "%s:", type); + virBufferEscapeSexpr(buf, "%s", def->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferVSprintf(buf, "%s", def->data.file.path); + virBufferEscapeSexpr(buf, "%s", def->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_TCP: @@ -5322,8 +5322,9 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferVSprintf(buf, "%s:%s%s", type, - def->data.nix.path, + virBufferVSprintf(buf, "%s:", type); + virBufferEscapeSexpr(buf, "%s", def->data.nix.path); + virBufferVSprintf(buf, "%s", def->data.nix.listen ? ",server,nowait" : ""); break; } @@ -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"); + } } else if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { - virBufferVSprintf(buf, "(dev '%s:cdrom')", def->dst); + virBufferEscapeSexpr(buf, "(dev '%s:cdrom')", def->dst); } else { - virBufferVSprintf(buf, "(dev '%s')", def->dst); + virBufferEscapeSexpr(buf, "(dev '%s')", def->dst); } if (def->src) { if (def->driverName) { if (STREQ(def->driverName, "tap") || STREQ(def->driverName, "tap2")) { - virBufferVSprintf(buf, "(uname '%s:%s:%s')", - def->driverName, - def->driverType ? def->driverType : "aio", - def->src); + virBufferEscapeSexpr(buf, "(uname '%s:", def->driverName); + virBufferEscapeSexpr(buf, "%s:", + def->driverType ? def->driverType : "aio"); + virBufferEscapeSexpr(buf, "%s')", def->src); } else { - virBufferVSprintf(buf, "(uname '%s:%s')", - def->driverName, - def->src); + virBufferEscapeSexpr(buf, "(uname '%s:", def->driverName); + virBufferEscapeSexpr(buf, "%s')", def->src); } } else { if (def->type == VIR_DOMAIN_DISK_TYPE_FILE) { - virBufferVSprintf(buf, "(uname 'file:%s')", def->src); + virBufferEscapeSexpr(buf, "(uname 'file:%s')", def->src); } else if (def->type == VIR_DOMAIN_DISK_TYPE_BLOCK) { if (def->src[0] == '/') - virBufferVSprintf(buf, "(uname 'phy:%s')", def->src); + virBufferEscapeSexpr(buf, "(uname 'phy:%s')", def->src); else - virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", def->src); + virBufferEscapeSexpr(buf, "(uname 'phy:/dev/%s')", + def->src); } else { virXendError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported disk type %s"), @@ -5501,13 +5505,13 @@ xenDaemonFormatSxprNet(virConnectPtr conn, switch (def->type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: - virBufferVSprintf(buf, "(bridge '%s')", def->data.bridge.brname); + virBufferEscapeSexpr(buf, "(bridge '%s')", def->data.bridge.brname); if (def->data.bridge.script) script = def->data.bridge.script; - virBufferVSprintf(buf, "(script '%s')", script); + virBufferEscapeSexpr(buf, "(script '%s')", script); if (def->data.bridge.ipaddr != NULL) - virBufferVSprintf(buf, "(ip '%s')", def->data.bridge.ipaddr); + virBufferEscapeSexpr(buf, "(ip '%s')", def->data.bridge.ipaddr); break; case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -5530,17 +5534,18 @@ xenDaemonFormatSxprNet(virConnectPtr conn, def->data.network.name); return -1; } - virBufferVSprintf(buf, "(bridge '%s')", bridge); - virBufferVSprintf(buf, "(script '%s')", script); + virBufferEscapeSexpr(buf, "(bridge '%s')", bridge); + virBufferEscapeSexpr(buf, "(script '%s')", script); VIR_FREE(bridge); } break; case VIR_DOMAIN_NET_TYPE_ETHERNET: if (def->data.ethernet.script) - virBufferVSprintf(buf, "(script '%s')", def->data.ethernet.script); + virBufferEscapeSexpr(buf, "(script '%s')", + def->data.ethernet.script); if (def->data.ethernet.ipaddr != NULL) - virBufferVSprintf(buf, "(ip '%s')", def->data.ethernet.ipaddr); + virBufferEscapeSexpr(buf, "(ip '%s')", def->data.ethernet.ipaddr); break; case VIR_DOMAIN_NET_TYPE_USER: @@ -5555,11 +5560,11 @@ xenDaemonFormatSxprNet(virConnectPtr conn, if (def->ifname != NULL && !STRPREFIX(def->ifname, "vif")) - virBufferVSprintf(buf, "(vifname '%s')", def->ifname); + virBufferEscapeSexpr(buf, "(vifname '%s')", def->ifname); if (!hvm) { if (def->model != NULL) - virBufferVSprintf(buf, "(model '%s')", def->model); + virBufferEscapeSexpr(buf, "(model '%s')", def->model); } else if (def->model == NULL) { /* @@ -5573,7 +5578,7 @@ xenDaemonFormatSxprNet(virConnectPtr conn, virBufferAddLit(buf, "(type netfront)"); } else { - virBufferVSprintf(buf, "(model '%s')", def->model); + virBufferEscapeSexpr(buf, "(model '%s')", def->model); virBufferAddLit(buf, "(type ioemu)"); } @@ -5680,7 +5685,8 @@ xenDaemonFormatSxprSound(virDomainDefPtr def, def->sounds[i]->model); return -1; } - virBufferVSprintf(buf, "%s%s", i ? "," : "", str); + virBufferVSprintf(buf, "%s", i ? "," : ""); + virBufferEscapeSexpr(buf, "%s", str); } if (virBufferError(buf)) { @@ -5737,10 +5743,13 @@ xenDaemonFormatSxpr(virConnectPtr conn, virBuffer buf = VIR_BUFFER_INITIALIZER; char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *tmp; + char *bufout; int hvm = 0, i; + DEBUG0("Formatting domain sexpr"); + virBufferAddLit(&buf, "(vm "); - virBufferVSprintf(&buf, "(name '%s')", def->name); + virBufferEscapeSexpr(&buf, "(name '%s')", def->name); virBufferVSprintf(&buf, "(memory %lu)(maxmem %lu)", def->mem.cur_balloon/1024, def->mem.max_balloon/1024); virBufferVSprintf(&buf, "(vcpus %u)", def->maxvcpus); @@ -5753,7 +5762,7 @@ xenDaemonFormatSxpr(virConnectPtr conn, char *ranges = virDomainCpuSetFormat(def->cpumask, def->cpumasklen); if (ranges == NULL) goto error; - virBufferVSprintf(&buf, "(cpus '%s')", ranges); + virBufferEscapeSexpr(&buf, "(cpus '%s')", ranges); VIR_FREE(ranges); } @@ -5761,16 +5770,16 @@ xenDaemonFormatSxpr(virConnectPtr conn, virBufferVSprintf(&buf, "(uuid '%s')", uuidstr); if (def->description) - virBufferVSprintf(&buf, "(description '%s')", def->description); + virBufferEscapeSexpr(&buf, "(description '%s')", def->description); if (def->os.bootloader) { if (def->os.bootloader[0]) - virBufferVSprintf(&buf, "(bootloader '%s')", def->os.bootloader); + virBufferEscapeSexpr(&buf, "(bootloader '%s')", def->os.bootloader); else virBufferAddLit(&buf, "(bootloader)"); if (def->os.bootloaderArgs) - virBufferVSprintf(&buf, "(bootloader_args '%s')", def->os.bootloaderArgs); + virBufferEscapeSexpr(&buf, "(bootloader_args '%s')", def->os.bootloaderArgs); } if (!(tmp = virDomainLifecycleTypeToString(def->onPoweroff))) { @@ -5827,20 +5836,20 @@ xenDaemonFormatSxpr(virConnectPtr conn, } if (def->os.kernel) - virBufferVSprintf(&buf, "(kernel '%s')", def->os.kernel); + virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.kernel); if (def->os.initrd) - virBufferVSprintf(&buf, "(ramdisk '%s')", def->os.initrd); + virBufferEscapeSexpr(&buf, "(ramdisk '%s')", def->os.initrd); if (def->os.root) - virBufferVSprintf(&buf, "(root '%s')", def->os.root); + virBufferEscapeSexpr(&buf, "(root '%s')", def->os.root); if (def->os.cmdline) - virBufferVSprintf(&buf, "(args '%s')", def->os.cmdline); + virBufferEscapeSexpr(&buf, "(args '%s')", def->os.cmdline); if (hvm) { char bootorder[VIR_DOMAIN_BOOT_LAST+1]; if (def->os.kernel) - virBufferVSprintf(&buf, "(loader '%s')", def->os.loader); + virBufferEscapeSexpr(&buf, "(loader '%s')", def->os.loader); else - virBufferVSprintf(&buf, "(kernel '%s')", def->os.loader); + virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.loader); virBufferVSprintf(&buf, "(vcpus %u)", def->maxvcpus); if (def->vcpus < def->maxvcpus) @@ -5883,14 +5892,14 @@ xenDaemonFormatSxpr(virConnectPtr conn, def->disks[i]->src == NULL) break; - virBufferVSprintf(&buf, "(cdrom '%s')", - def->disks[i]->src); + virBufferEscapeSexpr(&buf, "(cdrom '%s')", + def->disks[i]->src); break; case VIR_DOMAIN_DISK_DEVICE_FLOPPY: /* all xend versions define floppies here */ - virBufferVSprintf(&buf, "(%s '%s')", def->disks[i]->dst, - def->disks[i]->src); + virBufferEscapeSexpr(&buf, "(%s ", def->disks[i]->dst); + virBufferEscapeSexpr(&buf, "'%s')", def->disks[i]->src); break; default: @@ -5942,7 +5951,7 @@ xenDaemonFormatSxpr(virConnectPtr conn, /* get the device emulation model */ if (def->emulator && (hvm || xendConfigVersion >= 3)) - virBufferVSprintf(&buf, "(device_model '%s')", def->emulator); + virBufferEscapeSexpr(&buf, "(device_model '%s')", def->emulator); /* PV graphics for xen <= 3.0.4, or HVM graphics for xen <= 3.1.0 */ @@ -5986,7 +5995,9 @@ xenDaemonFormatSxpr(virConnectPtr conn, goto error; } - return virBufferContentAndReset(&buf); + bufout = virBufferContentAndReset(&buf); + DEBUG("Formatted sexpr: \n%s", bufout); + return bufout; error: virBufferFreeAndReset(&buf); diff --git a/tests/xml2sexprdata/xml2sexpr-escape.sexpr b/tests/xml2sexprdata/xml2sexpr-escape.sexpr new file mode 100644 index 0000000..efb0344 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-escape.sexpr @@ -0,0 +1 @@ +(vm (name 'fvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d21-71f4-8fb2-e068-e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (hvm (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://&""download.fedora.devel.redhat.com/pub/fedor... ')(loader '/usr/lib/xen/boot/hvmloader')(vcpus 2)(boot c)(usb 1)(parallel none)(serial pty)(device_model '/usr/lib/xen/bin/qemu-dm')))(device (vbd (dev 'ioemu:xvda')(uname 'file:/root/\'\\some.img')(mode 'w')))) \ No newline at end of file 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://&amp;""download.fedora.devel.redhat.com/pub/f... </cmdline> + <loader>/usr/lib/xen/boot/hvmloader</loader> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='file' device='disk'> + <source file='/root/&apos;\some.img'/> + <target dev='xvda'/> + </disk> + <console tty='/dev/pts/4'/> + </devices> +</domain> diff --git a/tests/xml2sexprtest.c b/tests/xml2sexprtest.c index 9cf8d39..8a5d115 100644 --- a/tests/xml2sexprtest.c +++ b/tests/xml2sexprtest.c @@ -163,6 +163,7 @@ mymain(int argc, char **argv) DO_TEST("fv-net-netfront", "fv-net-netfront", "fvtest", 1); DO_TEST("boot-grub", "boot-grub", "fvtest", 1); + DO_TEST("escape", "escape", "fvtest", 1); virCapabilitiesFree(caps); -- 1.7.3.2

On 11/19/2010 03:57 PM, Eric Blake wrote: This function is basically a copy of virBufferEscapeString. I'll add a patch to cleanup the original function before duplicating. Okay, i'll fold in these changes. 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