Re: [PATCH] virnetdev: Fix Memory Leak in virNetDevReplaceMacAddress() and virNetDevRestoreMacAddress()

[Michal Privoznik <mprivozn@xxxxxxxxxx>]

On 08.04.2014 06:25, Wangrui (K) wrote:
> function virNetDevRestoreMacAddress() and virNetDevRestoreMacAddress() alloc memory for
> variable 'path' using virAsprintf(), but they havn't free that memory before returning out.

The subject is rather long.
s/function/Functions/
s/alloc/allocate/
s/havn't free/haven't freed/

> 
> Signed-off-by: Zhang bo <oscar.zhangbo@xxxxxxxxxx>
> ---
>   src/util/virnetdev.c | 22 ++++++++++++++--------
>   1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> index f26ea80..853f6ef 100644
> --- a/src/util/virnetdev.c
> +++ b/src/util/virnetdev.c
> @@ -310,6 +310,7 @@ virNetDevReplaceMacAddress(const char *linkdev,
>       virMacAddr oldmac;
>       char *path = NULL;
>       char macstr[VIR_MAC_STRING_BUFLEN];
> +    int ret = -1;
>   
>       if (virNetDevGetMAC(linkdev, &oldmac) < 0)
>           return -1;
> @@ -323,13 +324,17 @@ virNetDevReplaceMacAddress(const char *linkdev,
>       if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
>           virReportSystemError(errno, _("Unable to preserve mac for %s"),
>                                linkdev);
> -        return -1;
> +       goto cleanup;

Indentation's off.

>       }
>   
>       if (virNetDevSetMAC(linkdev, macaddress) < 0)
> -        return -1;
> +        goto cleanup;
>   
> -    return 0;
> +    ret = 0;
> +
> +cleanup:

Here we tend to put space prior to the label name..

> +    VIR_FREE(path);
> +    return ret;
>   }
>   
>   /**
> @@ -344,7 +349,7 @@ int
>   virNetDevRestoreMacAddress(const char *linkdev,
>                              const char *stateDir)
>   {
> -    int rc;
> +    int rc = -1;
>       char *oldmacname = NULL;
>       char *macstr = NULL;
>       char *path = NULL;
> @@ -356,21 +361,22 @@ virNetDevRestoreMacAddress(const char *linkdev,
>           return -1;
>   
>       if (virFileReadAll(path, VIR_MAC_STRING_BUFLEN, &macstr) < 0)
> -        return -1;
> +        goto cleanup;
>   
>       if (virMacAddrParse(macstr, &oldmac) != 0) {
>           virReportError(VIR_ERR_INTERNAL_ERROR,
>                          _("Cannot parse MAC address from '%s'"),
>                          oldmacname);
> -        VIR_FREE(macstr);
> -        return -1;
> +        goto cleanup;
>       }
>   
>       /*reset mac and remove file-ignore results*/
>       rc = virNetDevSetMAC(linkdev, &oldmac);
>       ignore_value(unlink(path));
> -    VIR_FREE(macstr);
>   
> +cleanup:
> +    VIR_FREE(macstr);
> +    VIR_FREE(path);
>       return rc;
>   }
>   
> 

ACKed with this squashed in:

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 853f6ef..3a60cf7 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -315,7 +315,6 @@ virNetDevReplaceMacAddress(const char *linkdev,
     if (virNetDevGetMAC(linkdev, &oldmac) < 0)
         return -1;
 
-
     if (virAsprintf(&path, "%s/%s",
                     stateDir,
                     linkdev) < 0)
@@ -324,7 +323,7 @@ virNetDevReplaceMacAddress(const char *linkdev,
     if (virFileWriteStr(path, macstr, O_CREAT|O_TRUNC|O_WRONLY) < 0) {
         virReportSystemError(errno, _("Unable to preserve mac for %s"),
                              linkdev);
-       goto cleanup;
+        goto cleanup;
     }
 
     if (virNetDevSetMAC(linkdev, macaddress) < 0)
@@ -332,7 +331,7 @@ virNetDevReplaceMacAddress(const char *linkdev,
 
     ret = 0;
 
-cleanup:
+ cleanup:
     VIR_FREE(path);
     return ret;
 }
@@ -374,7 +373,7 @@ virNetDevRestoreMacAddress(const char *linkdev,
     rc = virNetDevSetMAC(linkdev, &oldmac);
     ignore_value(unlink(path));
 
-cleanup:
+ cleanup:
     VIR_FREE(macstr);
     VIR_FREE(path);
     return rc;


Adjusted the commit message and pushed. Congratulations to Oscar on his first libvirt patch!

Michal

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