[PATCH 0 of 3] libxl: leak in uuid to string conversions

[PATCH 0 of 3] libxl: leak in uuid to string conversions

[Ian Campbell]

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

[PATCH 1 of 3] libxl: do not leak uuid strings used internally

[Ian Campbell]

# 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])

[PATCH 2 of 3] libxl: avoid multiple allocations in libxl_uuid2string

[Ian Campbell]

# 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,

[PATCH 3 of 3] xl: don't leak uuid string on xl list

[Ian Campbell]

# 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');
     }

Re: [PATCH 0 of 3] libxl: leak in uuid to string conversions

[Ian Jackson]

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.

Re: [PATCH 0 of 3] libxl: leak in uuid to string conversions

[Gianni Tedesco]

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

Re: [PATCH 0 of 3] libxl: leak in uuid to string conversions

[Ian Campbell]

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.

Re: [PATCH 0 of 3] libxl: leak in uuid to string conversions

[Stefano Stabellini]

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

Re: [PATCH 0 of 3] libxl: leak in uuid to string conversions

[Ian Campbell]

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.

Re: [PATCH 0 of 3] libxl: leak in uuid to string conversions

[Stefano Stabellini]

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

Re: [PATCH 0 of 3] libxl: leak in uuid to string conversions

[Gianni Tedesco]

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.