[libvirt] [PATCH] virsh: Null terminated the string memcpy from buffer explicitly

[Osier Yang]

On 2012年06月14日 19:16, Eric Blake wrote:
> On 06/14/2012 05:09 AM, Osier Yang wrote:
>> ---
>>   tools/virsh.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>
> A bit light on the commit message.  Does this solve an actual crash, or
> does it just silence an overactive static code analyzer?

It's detected by valgrind:

# valgrind -v virsh change-media 3 hdc /var/lib/libvirt/images/foo2.img
==16217== 1 errors in context 1 of 12:
==16217== Invalid read of size 1
==16217==    at 0x4A07804: __GI_strlen (mc_replace_strmem.c:284)
==16217==    by 0x3019F167F6: xdr_string (in /lib64/libc-2.12.so)
==16217==    by 0x3033709E8D: xdr_remote_nonnull_string 
(remote_protocol.c:31)
==16217==    by 0x303370E5CB: xdr_remote_domain_update_device_flags_args 
(remote_protocol.c:2028)
==16217==    by 0x30337197D1: virNetMessageEncodePayload 
(virnetmessage.c:341)
==16217==    by 0x30337135E1: virNetClientProgramCall 
(virnetclientprogram.c:327)
==16217==    by 0x30336F1EFD: callWithFD (remote_driver.c:4586)
==16217==    by 0x30336F1F7B: call (remote_driver.c:4607)
==16217==    by 0x30336F42F2: remoteDomainUpdateDeviceFlags 
(remote_client_bodies.h:2865)
==16217==    by 0x30336D46E5: virDomainUpdateDeviceFlags (libvirt.c:9457)
==16217==    by 0x41AEE8: cmdChangeMedia (virsh.c:15249)
==16217==    by 0x413CB4: vshCommandRun (virsh.c:18669)
==16217==  Address 0x4ec5e25 is 0 bytes after a block of size 293 alloc'd
==16217==    at 0x4A04A28: calloc (vg_replace_malloc.c:467)
==16217==    by 0x303364F1DB: virAllocN (memory.c:129)
==16217==    by 0x41A844: vshPrepareDiskXML (virsh.c:15043)
==16217==    by 0x41AECC: cmdChangeMedia (virsh.c:15246)
==16217==    by 0x413CB4: vshCommandRun (virsh.c:18669)
==16217==    by 0x423973: main (virsh.c:20261)
>
>>
>> diff --git a/tools/virsh.c b/tools/virsh.c
>> index 98305c0..4509020 100644
>> --- a/tools/virsh.c
>> +++ b/tools/virsh.c
>> @@ -15069,11 +15069,13 @@ cleanup:
>>       VIR_FREE(device_type);
>>       VIR_FREE(disk_type);
>>       if (xml_buf) {
>> -        if (VIR_ALLOC_N(ret, xmlBufferLength(xml_buf))<  0) {
>> +        int len = xmlBufferLength(xml_buf);
>> +        if (VIR_ALLOC_N(ret, len + 1)<  0) {
>>               virReportOOMError();
>>               return NULL;
>>           }
>> -        memcpy(ret, (char *)xmlBufferContent(xml_buf), xmlBufferLength(xml_buf));
>> +        memcpy(ret, (char *)xmlBufferContent(xml_buf), len);
>> +        ret[len] = '\0';
>
> At any rate, the conversion makes sense.  ACK, if you'll improve the
> commit message.

Thanks, pushed with commit messege improved.

Osier