[PATCH] tools: libxl: do not leak diskpath during local disk attach

[PATCH] tools: libxl: do not leak diskpath during local disk attach

[Ian Campbell]

libxl__device_disk_local_initiate_attach is assigning dls->diskpath with a
strdup of the device path. This is then passed to the callback, e.g.
parse_bootloader_result but bootloader_cleanup will not free it.

Since the callback is within the scope of the (e)gc and therefore doesn't need
to be malloc'd, a gc'd alloc will do. All other assignments to this field use
the gc.

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=767295

Reported-by: Gedalya <gedalya@gedalya.net>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
This is a bug fix for 4.5.

This fix should be queued for backporting to at least 4.4
---
 tools/libxl/libxl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 18561fb..e76d898 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3030,7 +3030,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
     }
 
     if (dev != NULL)
-        dls->diskpath = strdup(dev);
+        dls->diskpath = libxl__strdup(gc, dev);
 
     dls->callback(egc, dls, 0);
     return;
-- 
1.7.10.4

Re: [PATCH] tools: libxl: do not leak diskpath during local disk attach

[Wei Liu]

On Thu, Nov 06, 2014 at 01:00:31PM +0000, Ian Campbell wrote:
> libxl__device_disk_local_initiate_attach is assigning dls->diskpath with a
> strdup of the device path. This is then passed to the callback, e.g.
> parse_bootloader_result but bootloader_cleanup will not free it.
> 
> Since the callback is within the scope of the (e)gc and therefore doesn't need
> to be malloc'd, a gc'd alloc will do. All other assignments to this field use
> the gc.
> 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=767295
> 
> Reported-by: Gedalya <gedalya@gedalya.net>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

> ---
> This is a bug fix for 4.5.
> 
> This fix should be queued for backporting to at least 4.4
> ---
>  tools/libxl/libxl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 18561fb..e76d898 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -3030,7 +3030,7 @@ void libxl__device_disk_local_initiate_attach(libxl__egc *egc,
>      }
>  
>      if (dev != NULL)
> -        dls->diskpath = strdup(dev);
> +        dls->diskpath = libxl__strdup(gc, dev);
>  
>      dls->callback(egc, dls, 0);
>      return;
> -- 
> 1.7.10.4

Re: [PATCH] tools: libxl: do not leak diskpath during local disk attach

[Ian Campbell]

On Thu, 2014-11-06 at 13:00 +0000, Ian Campbell wrote:
> libxl__device_disk_local_initiate_attach is assigning dls->diskpath with a
> strdup of the device path. This is then passed to the callback, e.g.
> parse_bootloader_result but bootloader_cleanup will not free it.
> 
> Since the callback is within the scope of the (e)gc and therefore doesn't need
> to be malloc'd, a gc'd alloc will do. All other assignments to this field use
> the gc.
> 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=767295

I should really have included the valgrind spew:

==4739== 48 bytes in 2 blocks are definitely lost in loss record 30 of 41
==4739==    at 0x4026B2D: malloc (vg_replace_malloc.c:299)
==4739==    by 0x41979FF: strdup (strdup.c:43)
==4739==    by 0x4064EC4: libxl__device_disk_local_initiate_attach (libxl.c:3033)
==4739==    by 0x4099256: libxl__bootloader_run (libxl_bootloader.c:387)
==4739==    by 0x4073CCE: initiate_domain_create (libxl_create.c:915)
==4739==    by 0x4073CCE: do_domain_create (libxl_create.c:1513)
==4739==    by 0x4073E75: libxl_domain_create_new (libxl_create.c:1536)
==4739==    by 0x80578DB: create_domain (xl_cmdimpl.c:2518)
==4739==    by 0x805B4B2: main_create (xl_cmdimpl.c:4652)
==4739==    by 0x804EAB2: main (xl.c:378)

Re: [PATCH] tools: libxl: do not leak diskpath during local disk attach

[Ian Jackson]

Ian Campbell writes ("[PATCH] tools: libxl: do not leak diskpath during local disk attach"):
> libxl__device_disk_local_initiate_attach is assigning dls->diskpath with a
> strdup of the device path. This is then passed to the callback, e.g.
> parse_bootloader_result but bootloader_cleanup will not free it.
> 
> Since the callback is within the scope of the (e)gc and therefore doesn't need
> to be malloc'd, a gc'd alloc will do. All other assignments to this field use
> the gc.
> 
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=767295
> 
> Reported-by: Gedalya <gedalya@gedalya.net>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

This patch is correct.  Apropos of your question on IRC, it is correct
to use `gc', which is the gc from the ao.  Its lifetime is the whole
device creation operation, which is fine.

You sometimes (including here) don't want to use the egc's gc in an ao
operation, because its lifetime is just the current event callback.
This is why we have STATE_AO_GC at the top of these functions, to make
sure `gc' is simply the right gc.

I will apply this patch to staging and queue it for backport.

Ian.

Re: [PATCH] tools: libxl: do not leak diskpath during local disk attach

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] tools: libxl: do not leak diskpath during local disk attach"):
> I will apply this patch to staging and queue it for backport.

Backported to 4.3 and 4.4.

Thanks,
Ian.