[libvirt] [PATCH 4/4] xend: Escape reserved sexpr characters

[Eric Blake]

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 at 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?

> 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.

> 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?

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20101119/8631af80/attachment-0001.sig>