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

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(a)cisco.com&gt; > --- 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...

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:

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);