| Message ID | 1415278831-9249-1-git-send-email-ian.campbell@citrix.com |
|---|---|
| State | New |
| Headers | show |
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
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)
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.
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;
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(-)