[v1] Couple of memleak fixes

[libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos

Posted by Michal Privoznik 8 years, 6 months ago

This function calls virDomainDefAddImplicitDevices() which adds
implicit video device to domain definition. However, later in the
process the function just ignores this and overwrites the @videos
array without prior free.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/vmx/vmx.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
index 96507f10f..858bc090d 100644
--- a/src/vmx/vmx.c
+++ b/src/vmx/vmx.c
@@ -1305,6 +1305,7 @@ virVMXParseConfig(virVMXContext *ctx,
     long long sharedFolder_maxNum = 0;
     int cpumasklen;
     char *namespaceData;
+    size_t i;
 
     if (ctx->parseFileName == NULL) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
@@ -1761,11 +1762,13 @@ virVMXParseConfig(virVMXContext *ctx,
     /* FIXME */
 
     /* def:videos */
-    if (VIR_ALLOC_N(def->videos, 1) < 0)
-        goto cleanup;
-
+    for (i = 0; i < def->nvideos; i++)
+        virDomainVideoDefFree(def->videos[i]);
     def->nvideos = 0;
 
+    if (VIR_REALLOC_N(def->videos, 1) < 0)
+        goto cleanup;
+
     if (virVMXParseSVGA(conf, &def->videos[def->nvideos]) < 0)
         goto cleanup;
 
-- 
2.13.0

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

Re: [libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos

Posted by Michal Privoznik 8 years, 6 months ago

On 08/04/2017 04:22 PM, Michal Privoznik wrote:
> This function calls virDomainDefAddImplicitDevices() which adds
> implicit video device to domain definition. However, later in the
> process the function just ignores this and overwrites the @videos
> array without prior free.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/vmx/vmx.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/vmx/vmx.c b/src/vmx/vmx.c
> index 96507f10f..858bc090d 100644
> --- a/src/vmx/vmx.c
> +++ b/src/vmx/vmx.c
> @@ -1305,6 +1305,7 @@ virVMXParseConfig(virVMXContext *ctx,
>      long long sharedFolder_maxNum = 0;
>      int cpumasklen;
>      char *namespaceData;
> +    size_t i;
>  
>      if (ctx->parseFileName == NULL) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -1761,11 +1762,13 @@ virVMXParseConfig(virVMXContext *ctx,
>      /* FIXME */
>  
>      /* def:videos */
> -    if (VIR_ALLOC_N(def->videos, 1) < 0)
> -        goto cleanup;
> -
> +    for (i = 0; i < def->nvideos; i++)
> +        virDomainVideoDefFree(def->videos[i]);
>      def->nvideos = 0;
>  
> +    if (VIR_REALLOC_N(def->videos, 1) < 0)
> +        goto cleanup;
> +
>      if (virVMXParseSVGA(conf, &def->videos[def->nvideos]) < 0)
>          goto cleanup;
>  
> 

Oh, forgot to push this into the commit:

diff --git i/src/vmx/vmx.c w/src/vmx/vmx.c
index 858bc090d..d111b1ef1 100644
--- i/src/vmx/vmx.c
+++ w/src/vmx/vmx.c
@@ -3035,7 +3035,7 @@ virVMXParseSVGA(virConfPtr conf, virDomainVideoDefPtr *def)
     int result = -1;
     long long svga_vramSize = 0;
 
-    if (def == NULL || *def != NULL) {
+    if (def == NULL) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Invalid argument"));
         return -1;
     }

Consider it squashed in locally.

Michal

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

Re: [libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos

Posted by Ján Tomko 8 years, 6 months ago

On Fri, Aug 04, 2017 at 04:22:33PM +0200, Michal Privoznik wrote:
>This function calls virDomainDefAddImplicitDevices() which adds
>implicit video device to domain definition. However, later in the
>process the function just ignores this and overwrites the @videos
>array without prior free.
>

We should not be calling virDomainDefAddImplicitDevices before
all the explicit devices are added to the domain definition.

What is the point of adding a device just to free it a few lines later?

Jan

>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> src/vmx/vmx.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 5/8] virVMXParseConfig: Don't leak def->videos

Posted by Michal Privoznik 8 years, 6 months ago

On 08/04/2017 04:56 PM, Ján Tomko wrote:
> On Fri, Aug 04, 2017 at 04:22:33PM +0200, Michal Privoznik wrote:
>> This function calls virDomainDefAddImplicitDevices() which adds
>> implicit video device to domain definition. However, later in the
>> process the function just ignores this and overwrites the @videos
>> array without prior free.
>>
> 
> We should not be calling virDomainDefAddImplicitDevices before
> all the explicit devices are added to the domain definition.
> 
> What is the point of adding a device just to free it a few lines later?

Because the parsing code expects some controllers to be in place before
it gets to video devices. Just look around the place where
virDomainDefAddImplicitDevices() is called. If you have a bright idea
how to fix it I'm all ears.

Michal

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