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

[Cole Robinson]

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

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