[Qemu-devel] [PATCH] virtio-gpu-3d: fix memory leak in resource attach backing

[Qemu-devel] [PATCH] virtio-gpu-3d: fix memory leak in resource attach backing

[Li Qiang]

If the virgl_renderer_resource_attach_iov function fails the
'res_iovs' will be leaked. Add check of the return value to
free the 'res_iovs' when failing.

Signed-off-by: Li Qiang <liq3ea@gmail.com>
---
 hw/display/virtio-gpu-3d.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
index 23f39de..07b3691 100644
--- a/hw/display/virtio-gpu-3d.c
+++ b/hw/display/virtio-gpu-3d.c
@@ -291,8 +291,11 @@ static void virgl_resource_attach_backing(VirtIOGPU *g,
         return;
     }
 
-    virgl_renderer_resource_attach_iov(att_rb.resource_id,
-                                       res_iovs, att_rb.nr_entries);
+    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
+                                             res_iovs, att_rb.nr_entries);
+
+    if (ret != 0)
+        virtio_gpu_cleanup_mapping_iov(res_iovs, att_rb.nr_entries);
 }
 
 static void virgl_resource_detach_backing(VirtIOGPU *g,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] virtio-gpu-3d: fix memory leak in resource attach backing

[Marc-André Lureau]

Hi

----- Original Message -----
> If the virgl_renderer_resource_attach_iov function fails the
> 'res_iovs' will be leaked. Add check of the return value to
> free the 'res_iovs' when failing.
> 
> Signed-off-by: Li Qiang <liq3ea@gmail.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Interestingly, in vrend_renderer_resource_attach_iov(), if the resource has already been attached, it returns 0 too, which will also leak. I guess the fix is it return an error in this case.


> ---
>  hw/display/virtio-gpu-3d.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> index 23f39de..07b3691 100644
> --- a/hw/display/virtio-gpu-3d.c
> +++ b/hw/display/virtio-gpu-3d.c
> @@ -291,8 +291,11 @@ static void virgl_resource_attach_backing(VirtIOGPU *g,
>          return;
>      }
>  
> -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
> -                                       res_iovs, att_rb.nr_entries);
> +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> +                                             res_iovs, att_rb.nr_entries);
> +
> +    if (ret != 0)
> +        virtio_gpu_cleanup_mapping_iov(res_iovs, att_rb.nr_entries);


>  }
>  
>  static void virgl_resource_detach_backing(VirtIOGPU *g,
> --
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH] virtio-gpu-3d: fix memory leak in resource attach backing

[Li Qiang]

2016-12-29 21:56 GMT+08:00 Marc-André Lureau <mlureau@redhat.com>:

> Hi
>
> ----- Original Message -----
> > If the virgl_renderer_resource_attach_iov function fails the
> > 'res_iovs' will be leaked. Add check of the return value to
> > free the 'res_iovs' when failing.
> >
> > Signed-off-by: Li Qiang <liq3ea@gmail.com>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Interestingly, in vrend_renderer_resource_attach_iov(), if the resource
> has already been attached, it returns 0 too, which will also leak. I guess
> the fix is it return an error in this case.
>
>
Right, actually I have write a patch for this here:

https://cgit.freedesktop.org/virglrenderer/commit/?id=40b0e7813325b08077b6f541b3989edb2d86d837


But the patch just return 0 if the 'iov' exist.


> > ---
> >  hw/display/virtio-gpu-3d.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu-3d.c b/hw/display/virtio-gpu-3d.c
> > index 23f39de..07b3691 100644
> > --- a/hw/display/virtio-gpu-3d.c
> > +++ b/hw/display/virtio-gpu-3d.c
> > @@ -291,8 +291,11 @@ static void virgl_resource_attach_backing(VirtIOGPU
> *g,
> >          return;
> >      }
> >
> > -    virgl_renderer_resource_attach_iov(att_rb.resource_id,
> > -                                       res_iovs, att_rb.nr_entries);
> > +    ret = virgl_renderer_resource_attach_iov(att_rb.resource_id,
> > +                                             res_iovs,
> att_rb.nr_entries);
> > +
> > +    if (ret != 0)
> > +        virtio_gpu_cleanup_mapping_iov(res_iovs, att_rb.nr_entries);
>
>
> >  }
> >
> >  static void virgl_resource_detach_backing(VirtIOGPU *g,
> > --
> > 1.8.3.1
> >
> >
>