libxl: leak in uuid to string conversions

PATCH In review Ian Campbell 5 participants 11 messages Aug 13, 2010 – Aug 16, 2010

lore

Share & tools

lore

Ian Campbell

author

2010-08-13 14:30 UTC #0 lore raw

PATCH 1/1 libxl: leak in uuid to string conversions

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

Ian Campbell

author

2010-08-13 14:30 UTC #1 lore raw

PATCH 1/1 libxl: do not leak uuid strings used internally

# HG changeset patch
# User Ian Campbell <[email protected]> # 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 <[email protected]>

tools/libxl/libxl.c +3 -3

diff -r adefd8ef5999 -r 72d92574410d tools/libxl/libxl.c
@@ -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)

tools/libxl/libxl_internal.h +2 -2

diff -r adefd8ef5999 -r 72d92574410d tools/libxl/libxl_internal.h
@@ -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])

Ian Campbell

author

2010-08-13 14:30 UTC #2 lore raw

PATCH 1/1 libxl: avoid multiple allocations in libxl_uuid2string

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # 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 <[email protected]>

tools/libxl/libxl_dom.c +16 ~~-11~~

diff -r 72d92574410d -r cf21a4ad0e79 tools/libxl/libxl_dom.c
@@ -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,

Ian Campbell

author

2010-08-13 14:30 UTC #3 lore raw

PATCH 1/1 xl: don't leak uuid string on xl list

In reply to Ian Campbell

# HG changeset patch
# User Ian Campbell <[email protected]> # 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 <[email protected]>

tools/libxl/xl_cmdimpl.c +1 -0

diff -r cf21a4ad0e79 -r 0a55168059b6 tools/libxl/xl_cmdimpl.c
@@ -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');
     }

Ian Jackson

Acked-by on v1 1/1

2010-08-13 15:16 UTC #4 lore raw

In reply to Ian Campbell

Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to string conversions"):

Quoted 2 lines

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

Acked-by Ian Jackson <[email protected]>

(all three)

Ian.

Gianni Tedesco

2010-08-16 12:51 UTC #5 lore raw

Quoted 9 lines · Ian Jackson

Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to string conversions"):
Quoted 2 lines
Fix a few memory leaks in the functions which convert uuids into strings.
Acked-by Ian Jackson <[email protected]>
(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

Ian Campbell

author

2010-08-16 13:06 UTC #6 lore raw

Quoted 17 lines · Gianni Tedesco

Quoted 9 lines · Ian Jackson
Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to string conversions"):
Quoted 2 lines
Fix a few memory leaks in the functions which convert uuids into strings.
Acked-by Ian Jackson <[email protected]>
(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.

Quoted 4 lines

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.

Stefano Stabellini

2010-08-16 13:51 UTC #7 lore raw

Quoted 23 lines · Ian Campbell

Quoted 17 lines · Gianni Tedesco
Quoted 9 lines · Ian Jackson
Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to string conversions"):
Quoted 2 lines
Fix a few memory leaks in the functions which convert uuids into strings.
Acked-by Ian Jackson <[email protected]>
(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.
Quoted 4 lines
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.

Quoted 5 lines

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

Ian Campbell

2010-08-16 14:01 UTC #8 lore raw

Quoted 24 lines · Stefano Stabellini

Quoted 23 lines · Ian Campbell
Quoted 21 lines · Gianni Tedesco
Quoted 9 lines · Ian Jackson
Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to string conversions"):
Quoted 2 lines
Fix a few memory leaks in the functions which convert uuids into strings.
Acked-by Ian Jackson <[email protected]>
(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.

Quoted 7 lines

Quoted 5 lines
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.

Stefano Stabellini

2010-08-16 14:05 UTC #9 lore raw

Quoted 31 lines · Ian Campbell

Quoted 30 lines · Stefano Stabellini
Quoted 28 lines · Ian Campbell
Quoted 17 lines · Gianni Tedesco
Quoted 9 lines · Ian Jackson
Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to string conversions"):
Quoted 2 lines
Fix a few memory leaks in the functions which convert uuids into strings.
Acked-by Ian Jackson <[email protected]>
(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.
Quoted 4 lines
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.

Quoted 5 lines

Quoted 1 line
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

Gianni Tedesco

2010-08-16 14:08 UTC #10 lore raw

Quoted 32 lines · Stefano Stabellini

Quoted 31 lines · Ian Campbell
Quoted 30 lines · Stefano Stabellini
Quoted 28 lines · Ian Campbell
Quoted 17 lines · Gianni Tedesco
Quoted 9 lines · Ian Jackson
Ian Campbell writes ("[Xen-devel] [PATCH 0 of 3] libxl: leak in uuid to string conversions"):
Quoted 2 lines
Fix a few memory leaks in the functions which convert uuids into strings.
Acked-by Ian Jackson <[email protected]>
(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.
Quoted 4 lines
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.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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.