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&lt;kmestery(a)cisco.com&gt; --- src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index b903ae4..cdbc5ef 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/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; + virBuffer buf = VIR_BUFFER_INITIALIZER; virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID)< 0) goto out_of_memory; } - if (virtVlan) { - if (VIR_ALLOC(buf)< 0) - goto out_of_memory; + + if (virtVlan&& virtVlan->nTags> 0) { /* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) { + virBufferAddLit(&buf, "trunk="); /* * Trunk ports have at least one VLAN. Do the first one @@ -93,21 +92,21 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, * start of the for loop if there are more than one VLANs * on this trunk port. */ - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); for (i = 1; i< virtVlan->nTags; i++) { - virBufferAddLit(buf, ","); - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); + virBufferAddLit(&buf, ","); + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); } } else if (virtVlan->nTags) { - virBufferAsprintf(buf, "tag=%d", virtVlan->tag[0]); + virBufferAsprintf(&buf, "tag=%d", virtVlan->tag[0]); } } cmd = virCommandNew(OVSVSCTL); if (ovsport->profileID[0] == '\0') { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -116,7 +115,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, NULL); } else { virCommandAddArgList(cmd, "--", "--may-exist", "add-port", - brname, ifname, virBufferContentAndReset(buf), + brname, ifname, virBufferCurrentContent(&buf), "--", "set", "Interface", ifname, attachedmac_ex_id, "--", "set", "Interface", ifname, ifaceid_ex_id, "--", "set", "Interface", ifname, vmid_ex_id, @@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ret = 0; cleanup: - VIR_FREE(buf); + if (virBufferUse(&buf)> 0) + virBufferFreeAndReset(&buf); VIR_FREE(attachedmac_ex_id); VIR_FREE(ifaceid_ex_id); VIR_FREE(vmid_ex_id);

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

src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index b903ae4..cdbc5ef 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/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; + virBuffer buf = VIR_BUFFER_INITIALIZER;

virMacAddrFormat(macaddr, macaddrstr); virUUIDFormat(ovsport->interfaceID, ifuuidstr); @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ovsport->profileID) < 0) goto out_of_memory; } - if (virtVlan) { - if (VIR_ALLOC(buf) < 0) - goto out_of_memory; + + if (virtVlan && virtVlan->nTags > 0) {

/* Trunk port first */ - if (virtVlan->trunk) { - virBufferAddLit(buf, "trunk="); + if (virtVlan->trunk == true) {

@@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, ret = 0; cleanup: - VIR_FREE(buf); + if (virBufferUse(&buf) > 0) + virBufferFreeAndReset(&buf);

On 08/30/2012 01:32 AM, Eric Blake wrote: > [...] > 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 you were to believe that virtVlan might be sent with no tags (which isn't the case with my patch applied...), then checking for non-0 virtVLan->tag[0] without checking for non-NULL virtVlan->tag would itself lead to a crash.

On Wed, Aug 29, 2012 at 02:44:36PM -0400, 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&lt;kmestery(a)cisco.com&gt; > --- > src/util/virnetdevopenvswitch.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c > index b903ae4..cdbc5ef 100644 > --- a/src/util/virnetdevopenvswitch.c > +++ b/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; > + virBuffer buf = VIR_BUFFER_INITIALIZER; > > virMacAddrFormat(macaddr, macaddrstr); > virUUIDFormat(ovsport->interfaceID, ifuuidstr); > @@ -79,13 +79,12 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, > ovsport->profileID)< 0) > goto out_of_memory; > } > - if (virtVlan) { > - if (VIR_ALLOC(buf)< 0) > - goto out_of_memory; > + > + if (virtVlan&& virtVlan->nTags> 0) { > > /* Trunk port first */ > - if (virtVlan->trunk) { > - virBufferAddLit(buf, "trunk="); > + if (virtVlan->trunk == true) { > + virBufferAddLit(&buf, "trunk="); > > /* > * Trunk ports have at least one VLAN. Do the first one > @@ -93,21 +92,21 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, > * start of the for loop if there are more than one VLANs > * on this trunk port. > */ > - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); > + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); > > for (i = 1; i< virtVlan->nTags; i++) { > - virBufferAddLit(buf, ","); > - virBufferAsprintf(buf, "%d", virtVlan->tag[i]); > + virBufferAddLit(&buf, ","); > + virBufferAsprintf(&buf, "%d", virtVlan->tag[i]); > } > } else if (virtVlan->nTags) { > - virBufferAsprintf(buf, "tag=%d", virtVlan->tag[0]); > + virBufferAsprintf(&buf, "tag=%d", virtVlan->tag[0]); > } > } > > cmd = virCommandNew(OVSVSCTL); > if (ovsport->profileID[0] == '\0') { > virCommandAddArgList(cmd, "--", "--may-exist", "add-port", > - brname, ifname, virBufferContentAndReset(buf), > + brname, ifname, virBufferCurrentContent(&buf), > "--", "set", "Interface", ifname, attachedmac_ex_id, > "--", "set", "Interface", ifname, ifaceid_ex_id, > "--", "set", "Interface", ifname, vmid_ex_id, > @@ -116,7 +115,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, > NULL); > } else { > virCommandAddArgList(cmd, "--", "--may-exist", "add-port", > - brname, ifname, virBufferContentAndReset(buf), > + brname, ifname, virBufferCurrentContent(&buf), > "--", "set", "Interface", ifname, attachedmac_ex_id, > "--", "set", "Interface", ifname, ifaceid_ex_id, > "--", "set", "Interface", ifname, vmid_ex_id, > @@ -135,7 +134,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, > > ret = 0; > cleanup: > - VIR_FREE(buf); > + if (virBufferUse(&buf)> 0) > + virBufferFreeAndReset(&buf); that looks fine up to here, where we could leak in theory, virBufferUse() return the amount of bytes used in the buffer, not if the buffer was allocated. Sounds to me we can use virBufferFreeAndReset() directly without the test > VIR_FREE(attachedmac_ex_id); > VIR_FREE(ifaceid_ex_id); > VIR_FREE(vmid_ex_id); I pushed with that small change, thanks ! Daniel