[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 cisco com>
> ---

It's hard to see the real fix amongst the churn.

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

Yes, the old code risked freeing an uninitialized pointer.

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

This line also impacts behavior, unrelated to the change between 'buf'
being heap-allocated vs. stack-allocated...

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

'cond == true' as a condition is a pointless change, 'cond' is sufficient.

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

...and this change fixes both a potential memory leak, where the old
VIR_FREE(buf) risked leaking the contents of buf, and the potential
free(garbage).

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


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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]