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

[Alex Jia <ajia@xxxxxxxxxx>]

On 08/30/2012 01:49 PM, Daniel Veillard wrote:


Daniel, BTW, To test this patch again, it works for me :)

> 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<kmestery@xxxxxxxxx>
> > ---
> >   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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list