Xen devel - Aug 2010 - [PATCH 0 of 3] libxl: leak in uuid to string conversions

Fix a few memory leaks in the functions which convert uuids into
strings.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1281709508 -3600
# Node ID 72d92574410da1d388dd33e5fe10e955fd1a93ec
# Parent  adefd8ef59992b04b6d7da35f81ff6d7cc48a6a6
libxl: do not leak uuid strings used internally

Use string_of_uuid (which adds the string to the gc context) instead
of libxl_uuid_to_string (which adds the string to the context and then
strdups it!).

Fixes this valgrind warning:
==10005== 37 bytes in 1 blocks are definitely lost in loss record 1 of 1
==10005==    at 0x4022F0A: malloc (vg_replace_malloc.c:236)
==10005==    by 0x411A22F: strdup (in /lib/i686/cmov/libc-2.7.so)
==10005==    by 0x4047930: libxl_uuid2string (libxl_dom.c:454)
==10005==    by 0x404185A: libxl_domain_make (libxl.c:121)
==10005==    by 0x8056F5B: create_domain (xl_cmdimpl.c:1387)
==10005==    by 0x8058216: main_create (xl_cmdimpl.c:3171)
==10005==    by 0x804B5AB: main (xl.c:76)

libxl_domain_preserve and libxl_set_memory_target suffer the same
problem by inspection only.

Also since string_of_uuid now takes a gc not a ctx rename the variable
to be less confusing.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r adefd8ef5999 -r 72d92574410d tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Fri Aug 13 14:47:53 2010 +0100
+++ b/tools/libxl/libxl.c	Fri Aug 13 15:25:08 2010 +0100
@@ -118,7 +118,7 @@ int libxl_domain_make(libxl_ctx *ctx, li
     xs_transaction_t t;
     xen_domain_handle_t handle;
 
-    uuid_string = libxl_uuid2string(ctx, info->uuid);
+    uuid_string = string_of_uuid(&gc, info->uuid);
     if (!uuid_string) {
         libxl_free_all(&gc);
         return ERROR_NOMEM;
@@ -481,7 +481,7 @@ int libxl_domain_preserve(libxl_ctx *ctx
         return ERROR_NOMEM;
     }
 
-    uuid_string = libxl_uuid2string(ctx, new_uuid);
+    uuid_string = string_of_uuid(&gc, new_uuid);
     if (!uuid_string) {
         libxl_free_all(&gc);
         return ERROR_NOMEM;
@@ -2813,7 +2813,7 @@ int libxl_set_memory_target(libxl_ctx *c
     if (rc != 1 || info.domain != domid)
         goto out;
     xcinfo2xlinfo(&info, &ptr);
-    uuid = libxl_uuid2string(ctx, ptr.uuid);
+    uuid = string_of_uuid(&gc, ptr.uuid);
     libxl_xs_write(&gc, XBT_NULL, libxl_sprintf(&gc,
"/vm/%s/memory", uuid), "%"PRIu32, target_memkb / 1024);
 
     if (enforce || !domid)
diff -r adefd8ef5999 -r 72d92574410d tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h	Fri Aug 13 14:47:53 2010 +0100
+++ b/tools/libxl/libxl_internal.h	Fri Aug 13 15:25:08 2010 +0100
@@ -107,8 +107,8 @@ typedef struct {
 #define PRINTF_ATTRIBUTE(x, y) __attribute__((format(printf, x, y)))
 
 #define UUID_FMT
"%02hhx%02hhx%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx-%02hhx%02hhx%02hhx%02hhx%02hhx%02hhx"
-#define string_of_uuid(ctx, u) \
-    libxl_sprintf(ctx, UUID_FMT, \
+#define string_of_uuid(gc, u) \
+    libxl_sprintf(gc, UUID_FMT, \
                 (u)[0], (u)[1], (u)[2], (u)[3], (u)[4], (u)[5], (u)[6], (u)[7],
\
                 (u)[8], (u)[9], (u)[10], (u)[11], (u)[12], (u)[13], (u)[14],
(u)[15])
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1281709509 -3600
# Node ID cf21a4ad0e798f76adb91453769a36e0856ccd12
# Parent  72d92574410da1d388dd33e5fe10e955fd1a93ec
libxl: avoid multiple allocations in libxl_uuid2string

The pointer returned by libxl_uuid2string is the callers
responsibility but the function currently allocates the string into
the current context and then duplicates the result.

Instead of allocating the memory twice (and immediately throwing one
away) just allocate the memory ourselves.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 72d92574410d -r cf21a4ad0e79 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Fri Aug 13 15:25:08 2010 +0100
+++ b/tools/libxl/libxl_dom.c	Fri Aug 13 15:25:09 2010 +0100
@@ -444,17 +444,22 @@ int save_device_model(libxl_ctx *ctx, ui
 
 char *libxl_uuid2string(libxl_ctx *ctx, const libxl_uuid uuid)
 {
-    libxl_gc gc = LIBXL_INIT_GC(ctx);
-    char *s = string_of_uuid(&gc, uuid);
-    char *ret;
-    if (!s) {
-        XL_LOG(ctx, XL_LOG_ERROR, "cannot allocate for uuid");
-        ret = NULL;
-    }else{
-        ret = strdup(s);
-    }
-    libxl_free_all(&gc);
-    return ret;
+    char *s;
+    int ret;
+
+    ret = snprintf(NULL, 0, UUID_FMT,
+                   uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
uuid[6], uuid[7],
+                   uuid[8], uuid[9], uuid[10], uuid[11], uuid[12], uuid[13],
uuid[14], uuid[15]);
+
+    if (ret < 0)
+        return NULL;
+
+    s = malloc(ret + 1);
+    snprintf(s, ret + 1, UUID_FMT,
+                   uuid[0], uuid[1], uuid[2], uuid[3], uuid[4], uuid[5],
uuid[6], uuid[7],
+                   uuid[8], uuid[9], uuid[10], uuid[11], uuid[12], uuid[13],
uuid[14], uuid[15]);
+
+    return s;
 }
 
 static const char *userdata_path(libxl_gc *gc, uint32_t domid,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# Date 1281709714 -3600
# Node ID 0a55168059b6c875d5923fd56d908db4636f4a74
# Parent  cf21a4ad0e798f76adb91453769a36e0856ccd12
xl: don''t leak uuid string on xl list

==10272== 37 bytes in 1 blocks are definitely lost in loss record 1 of 1
==10272==    at 0x4022F0A: malloc (vg_replace_malloc.c:236)
==10272==    by 0x4047B2B: libxl_uuid2string (libxl_dom.c:457)
==10272==    by 0x8050A02: list_domains (xl_cmdimpl.c:2198)
==10272==    by 0x8054FC5: main_list (xl_cmdimpl.c:3064)
==10272==    by 0x804B5AB: main (xl.c:76)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r cf21a4ad0e79 -r 0a55168059b6 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Fri Aug 13 15:25:09 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Fri Aug 13 15:28:34 2010 +0100
@@ -2197,6 +2197,7 @@ void list_domains(int verbose, const lib
         if (verbose) {
             char *uuid = libxl_uuid2string(&ctx, info[i].uuid);
             printf(" %s", uuid);
+            free(uuid);
         }
         putchar(''\n'');
     }

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to
string conversions"):
> Fix a few memory leaks in the functions which convert uuids into
> strings.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(all three)

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2010-08-13 at 16:16 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid
to string conversions"):
> > Fix a few memory leaks in the functions which convert uuids into
> > strings.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> (all three)
> 
> Ian.
I''m not sure I agree with this patch, especially 3 of 3. The callers
are
inconsistent and also pointless. There''s no need to allocate a uuid
string when all that''s really needed is a few printf macros as in
"[PATCH,v2] xl: make libxl_uuid2string internal to libxenlight" - Also
UUID_FMT remains duplicated.

The first two patches are probably fine but then we should just nuke
libxl_uuid2string all together. Such a function makes sense for libxl
where it''s going to be constructing xenstore paths so may need to keep
such things around but xl has no use for this as far as I can see.

Gianni


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2010-08-16 at 13:51 +0100, Gianni Tedesco wrote:
> On Fri, 2010-08-13 at 16:16 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in
uuid to string conversions"):
> > > Fix a few memory leaks in the functions which convert uuids into
> > > strings.
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > (all three)
> > 
> > Ian.
> 
> I''m not sure I agree with this patch, especially 3 of 3. The
callers are
> inconsistent and also pointless. There''s no need to allocate a
uuid
> string when all that''s really needed is a few printf macros as in
> "[PATCH,v2] xl: make libxl_uuid2string internal to libxenlight" -
Also
> UUID_FMT remains duplicated.
> 
> The first two patches are probably fine but then we should just nuke
> libxl_uuid2string all together. Such a function makes sense for libxl
> where it''s going to be constructing xenstore paths so may need to
keep
> such things around but xl has no use for this as far as I can see.
FWIW the reason I didn''t followup further with this thread (other than
it being a Friday afternoon) was that I agreed with your v2 patch which
added the macros and made the libxl_uuid2string fn internal to the
library. AFAICT your patch also solved all the leaks I was seeing.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 16 Aug 2010, Ian Campbell wrote:
> On Mon, 2010-08-16 at 13:51 +0100, Gianni Tedesco wrote:
> > On Fri, 2010-08-13 at 16:16 +0100, Ian Jackson wrote:
> > > Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak
in uuid to string conversions"):
> > > > Fix a few memory leaks in the functions which convert uuids
into
> > > > strings.
> > > 
> > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > 
> > > (all three)
> > > 
> > > Ian.
> > 
> > I''m not sure I agree with this patch, especially 3 of 3. The
callers are
> > inconsistent and also pointless. There''s no need to allocate
a uuid
> > string when all that''s really needed is a few printf macros
as in
> > "[PATCH,v2] xl: make libxl_uuid2string internal to
libxenlight" - Also
> > UUID_FMT remains duplicated.
> > 
> > The first two patches are probably fine but then we should just nuke
> > libxl_uuid2string all together. Such a function makes sense for libxl
> > where it''s going to be constructing xenstore paths so may
need to keep
> > such things around but xl has no use for this as far as I can see.
> 
> FWIW the reason I didn''t followup further with this thread (other
than
> it being a Friday afternoon) was that I agreed with your v2 patch which
> added the macros and made the libxl_uuid2string fn internal to the
> library. AFAICT your patch also solved all the leaks I was seeing.
> 
I also think Gianni''s libuuid series should be applied

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2010-08-16 at 14:51 +0100, Stefano Stabellini
wrote:
> On Mon, 16 Aug 2010, Ian Campbell wrote:
> > On Mon, 2010-08-16 at 13:51 +0100, Gianni Tedesco wrote:
> > > On Fri, 2010-08-13 at 16:16 +0100, Ian Jackson wrote:
> > > > Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl:
leak in uuid to string conversions"):
> > > > > Fix a few memory leaks in the functions which convert
uuids into
> > > > > strings.
> > > > 
> > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > 
> > > > (all three)
> > > > 
> > > > Ian.
> > > 
> > > I''m not sure I agree with this patch, especially 3 of 3.
The callers are
> > > inconsistent and also pointless. There''s no need to
allocate a uuid
> > > string when all that''s really needed is a few printf
macros as in
> > > "[PATCH,v2] xl: make libxl_uuid2string internal to
libxenlight" - Also
> > > UUID_FMT remains duplicated.
> > > 
> > > The first two patches are probably fine but then we should just
nuke
> > > libxl_uuid2string all together. Such a function makes sense for
libxl
> > > where it''s going to be constructing xenstore paths so
may need to keep
> > > such things around but xl has no use for this as far as I can
see.
> > 
> > FWIW the reason I didn''t followup further with this thread
(other than
> > it being a Friday afternoon) was that I agreed with your v2 patch
which
> > added the macros and made the libxl_uuid2string fn internal to the
> > library. AFAICT your patch also solved all the leaks I was seeing.
> > 
> 
> I also think Gianni''s libuuid series should be applied
I think that''s orthogonal to the particular patch we are discussing
here
though, that patch concerns the generation of UUIDs while this one
concerns leaking strings when formatting them...

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 16 Aug 2010, Ian Campbell wrote:
> On Mon, 2010-08-16 at 14:51 +0100, Stefano Stabellini wrote:
> > On Mon, 16 Aug 2010, Ian Campbell wrote:
> > > On Mon, 2010-08-16 at 13:51 +0100, Gianni Tedesco wrote:
> > > > On Fri, 2010-08-13 at 16:16 +0100, Ian Jackson wrote:
> > > > > Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3]
libxl: leak in uuid to string conversions"):
> > > > > > Fix a few memory leaks in the functions which
convert uuids into
> > > > > > strings.
> > > > > 
> > > > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > > > > 
> > > > > (all three)
> > > > > 
> > > > > Ian.
> > > > 
> > > > I''m not sure I agree with this patch, especially 3
of 3. The callers are
> > > > inconsistent and also pointless. There''s no need to
allocate a uuid
> > > > string when all that''s really needed is a few
printf macros as in
> > > > "[PATCH,v2] xl: make libxl_uuid2string internal to
libxenlight" - Also
> > > > UUID_FMT remains duplicated.
> > > > 
> > > > The first two patches are probably fine but then we should
just nuke
> > > > libxl_uuid2string all together. Such a function makes sense
for libxl
> > > > where it''s going to be constructing xenstore paths
so may need to keep
> > > > such things around but xl has no use for this as far as I
can see.
> > > 
> > > FWIW the reason I didn''t followup further with this
thread (other than
> > > it being a Friday afternoon) was that I agreed with your v2 patch
which
> > > added the macros and made the libxl_uuid2string fn internal to
the
> > > library. AFAICT your patch also solved all the leaks I was
seeing.
> > > 
> > 
> > I also think Gianni''s libuuid series should be applied
> 
> I think that''s orthogonal to the particular patch we are
discussing here
> though, that patch concerns the generation of UUIDs while this one
> concerns leaking strings when formatting them...
 
sorry, I meant libuuid2string

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2010-08-16 at 15:05 +0100, Stefano Stabellini
wrote:
> On Mon, 16 Aug 2010, Ian Campbell wrote:
> > On Mon, 2010-08-16 at 14:51 +0100, Stefano Stabellini wrote:
> > > On Mon, 16 Aug 2010, Ian Campbell wrote:
> > > > On Mon, 2010-08-16 at 13:51 +0100, Gianni Tedesco wrote:
> > > > > On Fri, 2010-08-13 at 16:16 +0100, Ian Jackson wrote:
> > > > > > Ian Campbell writes ("[Xen-devel] [PATCH 0 of
3] libxl: leak in uuid to string conversions"):
> > > > > > > Fix a few memory leaks in the functions which
convert uuids into
> > > > > > > strings.
> > > > > > 
> > > > > > Acked-by: Ian Jackson
<ian.jackson@eu.citrix.com>
> > > > > > 
> > > > > > (all three)
> > > > > > 
> > > > > > Ian.
> > > > > 
> > > > > I''m not sure I agree with this patch,
especially 3 of 3. The callers are
> > > > > inconsistent and also pointless. There''s no
need to allocate a uuid
> > > > > string when all that''s really needed is a few
printf macros as in
> > > > > "[PATCH,v2] xl: make libxl_uuid2string internal to
libxenlight" - Also
> > > > > UUID_FMT remains duplicated.
> > > > > 
> > > > > The first two patches are probably fine but then we
should just nuke
> > > > > libxl_uuid2string all together. Such a function makes
sense for libxl
> > > > > where it''s going to be constructing xenstore
paths so may need to keep
> > > > > such things around but xl has no use for this as far as
I can see.
> > > > 
> > > > FWIW the reason I didn''t followup further with this
thread (other than
> > > > it being a Friday afternoon) was that I agreed with your v2
patch which
> > > > added the macros and made the libxl_uuid2string fn internal
to the
> > > > library. AFAICT your patch also solved all the leaks I was
seeing.
> > > > 
> > > 
> > > I also think Gianni''s libuuid series should be applied
> > 
> > I think that''s orthogonal to the particular patch we are
discussing here
> > though, that patch concerns the generation of UUIDs while this one
> > concerns leaking strings when formatting them...
>  
> sorry, I meant libuuid2string
Well, regarding the libuuid thing, I need to re-write that incorporating
Egger''s suggestions about portability so it shouldn''t be
applied as is.

My conclusion on the mini-flame-war regarding mac addresses is that that
stuff is orthogonal to the part where we actually use a proper random
source.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel