[libvirt] [PATCH] Fix a crash when using Open vSwitch virtual ports

[Eric Blake]

On 08/29/2012 10:32 PM, Eric Blake wrote:
> On 08/29/2012 11:44 AM, Kyle Mestery wrote:
>> Fixup buffer usage when handling VLANs. Also fix the logic
>> used to determine if the virNetDevVlanPtr is valid or not.
>> Fixes crashes in the latest code when using Open vSwitch
>> virtualports.
>>
>> Signed-off-by: Kyle Mestery <kmestery at cisco.com>
>> ---
> 
> It's hard to see the real fix amongst the churn.
> 

>> -    virBufferPtr buf;
>> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> 
> Yes, the old code risked freeing an uninitialized pointer.
> 

>> -    if (virtVlan) {
>> -        if (VIR_ALLOC(buf) < 0)
>> -            goto out_of_memory;
>> +
>> +    if (virtVlan && virtVlan->nTags > 0) {
> 
> This line also impacts behavior, unrelated to the change between 'buf'
> being heap-allocated vs. stack-allocated...

That said, I _like_ using stack-allocated instead of heap-allocated, but
that means that I think there's two separate issues (virtVlan->nTags vs.
virBuffer usage cleanup), so maybe this deserves two patches instead of
one...


> Note that the new code is wrong as well - you should unconditionally
> free a virBufferPtr's contents, rather than conditionally freeing it
> based on virBufferUse() returning non-zero.
> 
> That is, I think the real patch here would be simply this (untested)
> version:

This 3-liner to fix the bug at hand would be the first patch, then
another to convert to stack-allocation with no semantic changes.

> 
> diff --git i/src/util/virnetdevopenvswitch.c
> w/src/util/virnetdevopenvswitch.c
> index b903ae4..e8b9f4a 100644
> --- i/src/util/virnetdevopenvswitch.c
> +++ w/src/util/virnetdevopenvswitch.c
> @@ -59,7 +59,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
>      char *ifaceid_ex_id = NULL;
>      char *profile_ex_id = NULL;
>      char *vmid_ex_id = NULL;
> -    virBufferPtr buf;
> +    virBufferPtr buf = NULL;
> 
>      virMacAddrFormat(macaddr, macaddrstr);
>      virUUIDFormat(ovsport->interfaceID, ifuuidstr);
> @@ -79,7 +79,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
>                          ovsport->profileID) < 0)
>              goto out_of_memory;
>      }
> -    if (virtVlan) {
> +    if (virtVlan && virtVlan->tag[0]) {
>          if (VIR_ALLOC(buf) < 0)
>              goto out_of_memory;
> 
> @@ -135,6 +135,7 @@ int virNetDevOpenvswitchAddPort(const char *brname,
> const char *ifname,
> 
>      ret = 0;
>  cleanup:
> +    virBufferFreeAndReset(&buf);
>      VIR_FREE(buf);
>      VIR_FREE(attachedmac_ex_id);
>      VIR_FREE(ifaceid_ex_id);
> 

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

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