libxl: memory leaks

The following fix a few memory leaks.

The first was discussed on list and it was decided that although it wasn't a real leak, since the memory is allocated exactly once per thread and is correctly cleaned up on pthread_exit(), it was still useful for xl (a non-threaded application) to be able to be completely valgrind clean for the purposes of auditing libxl.

The leaks fixed in xl create are just a subset since I didn't delve into recursively freeing the types defined by libxl yet. There are also leaks in the lexer and parser which I didn't tackle yet.

# HG changeset patch
# User Ian Campbell <[email protected]>
# Date 1280752237 -3600
# Node ID b3e1074b137c03131cea33c5100cbc2fa6cc5d5f
# Parent f5f5949d98f0104ad1422ddacded20875f23d38d
libxc: free thread specific hypercall buffer on xc_interface_close

The per-thread hypercall buffer is usually cleaned up on pthread_exit by the destructor passed to pthread_key_create. However if the calling application is not threaded then the destructor is never called.

This frees the data for the current thread only but that is OK since any other threads will be cleaned up by the destructor.

diff -r f5f5949d98f0 -r b3e1074b137c tools/libxc/xc_private.c
@@ -57,6 +57,8 @@ xc_interface *xc_interface_open(xentooll
     return 0;
 }
 
+static void xc_clean_hcall_buf(void);
+
 int xc_interface_close(xc_interface *xch)
 {
     int rc = 0;

@@ -68,6 +70,9 @@ int xc_interface_close(xc_interface *xch
         rc = xc_interface_close_core(xch, xch->fd);
         if (rc) PERROR("Could not close hypervisor interface");
     }
+
+    xc_clean_hcall_buf();
+
     free(xch);
     return rc;
 }

@@ -180,6 +185,8 @@ int hcall_buf_prep(void **addr, size_t l
 int hcall_buf_prep(void **addr, size_t len) { return 0; }
 void hcall_buf_release(void **addr, size_t len) { }
 
+static void xc_clean_hcall_buf(void) { }
+
 #else /* !__sun__ */
 
 int lock_pages(void *addr, size_t len)

@@ -223,6 +230,14 @@ static void _xc_clean_hcall_buf(void *m)
     }
 
     pthread_setspecific(hcall_buf_pkey, NULL);
+}
+
+static void xc_clean_hcall_buf(void)
+{
+    void *hcall_buf = pthread_getspecific(hcall_buf_pkey);
+
+    if (hcall_buf)
+        _xc_clean_hcall_buf(hcall_buf);
 }
 
 static void _xc_init_hcall_buf(void)

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1280752237 -3600
# Node ID 2a932193a3fac4f861f0c6353a2d251077012a61 # Parent b3e1074b137c03131cea33c5100cbc2fa6cc5d5f
libxl: fix memory leak in libxl_name_to_domid

Found with "valgrind xl destroy ...":
==16272== 53,248 bytes in 1 blocks are definitely lost in loss record 6 of 6

==16272==    at 0x4022249: calloc (vg_replace_malloc.c:467)
==16272==    by 0x403FD4A: libxl_list_domain (libxl.c:490)
==16272==    by 0x404B901: libxl_name_to_domid (libxl_utils.c:65)
==16272==    by 0x804B4D2: domain_qualifier_to_domid (xl_cmdimpl.c:181)
==16272==    by 0x804B50F: find_domain (xl_cmdimpl.c:198)
==16272==    by 0x804D70C: destroy_domain (xl_cmdimpl.c:2104)
==16272==    by 0x8054E4C: main_destroy (xl_cmdimpl.c:2912)
==16272==    by 0x804B2FB: main (xl.c:76)

diff -r b3e1074b137c -r 2a932193a3fa tools/libxl/libxl_utils.c
@@ -61,6 +61,7 @@ int libxl_name_to_domid(libxl_ctx *ctx, 
     int i, nb_domains;
     char *domname;
     libxl_dominfo *dominfo;
+    int ret = -1;
 
     dominfo = libxl_list_domain(ctx, &nb_domains);
     if (!dominfo)

@@ -72,10 +73,12 @@ int libxl_name_to_domid(libxl_ctx *ctx, 
             continue;
         if (strcmp(domname, name) == 0) {
             *domid = dominfo[i].domid;
-            return 0;
+            ret = 0;
+            break;
         }
     }
-    return -1;
+    free(dominfo);
+    return ret;
 }
 
 char *libxl_poolid_to_name(libxl_ctx *ctx, uint32_t poolid)

# HG changeset patch
# User Ian Campbell <[email protected]> # Date 1280752237 -3600
# Node ID e2e90fac665ede04ef44d16c7df82f12b55f3931 # Parent 2a932193a3fac4f861f0c6353a2d251077012a61
xl: fix memory leaks in xl create

Found using "valgrind xl create -n ..." and "valgrind xl create -e ..."

freeing config_data solves:
==18276== 944 bytes in 1 blocks are definitely lost in loss record 12 of 12

==18276==    at 0x4022F0A: malloc (vg_replace_malloc.c:236)
==18276==    by 0x404AEC1: libxl_read_file_contents (libxl_utils.c:258)
==18276==    by 0x8056865: create_domain (xl_cmdimpl.c:1314)
==18276==    by 0x8057E2D: main_create (xl_cmdimpl.c:3135)
==18276==    by 0x804B2FB: main (xl.c:76)

==18276==

Adding free_domain_config() solves the following (plus presumably others which didn't trigger because I have no devices of that type).

d_config->disks:
==18276== 61 (32 direct, 29 indirect) bytes in 1 blocks are definitely lost in loss record 9 of 12

==18276==    at 0x4022F0A: malloc (vg_replace_malloc.c:236)
==18276==    by 0x4022F94: realloc (vg_replace_malloc.c:525)
==18276==    by 0x804E2D3: parse_config_data (xl_cmdimpl.c:715)
==18276==    by 0x8056A7C: create_domain (xl_cmdimpl.c:1347)
==18276==    by 0x8057E2D: main_create (xl_cmdimpl.c:3135)
==18276==    by 0x804B2FB: main (xl.c:76)

d_config->vifs:
==18276== 76 (48 direct, 28 indirect) bytes in 1 blocks are definitely lost in loss record 10 of 12

==18276==    at 0x4022F0A: malloc (vg_replace_malloc.c:236)
==18276==    by 0x4022F94: realloc (vg_replace_malloc.c:525)
==18276==    by 0x804E665: parse_config_data (xl_cmdimpl.c:779)
==18276==    by 0x8056A7C: create_domain (xl_cmdimpl.c:1347)
==18276==    by 0x8057E2D: main_create (xl_cmdimpl.c:3135)
==18276==    by 0x804B2FB: main (xl.c:76)

diff -r 2a932193a3fa -r e2e90fac665e tools/libxl/xl_cmdimpl.c
@@ -142,6 +142,16 @@ struct domain_config {
     enum action_on_shutdown on_watchdog;
     enum action_on_shutdown on_crash;
 };
+
+static void free_domain_config(struct domain_config *d_config)
+{
+    free(d_config->disks);
+    free(d_config->vifs);
+    free(d_config->vif2s);
+    free(d_config->pcidevs);
+    free(d_config->vfbs);
+    free(d_config->vkbs);
+}
 
 /* Optional data, in order:
  *   4 bytes uint32_t  config file size

@@ -1346,8 +1356,9 @@ static int create_domain(struct domain_c
 
     parse_config_data(config_file, config_data, config_len, &d_config, &dm_info);
 
+    ret = 0;
     if (dom_info->dryrun)
-        return 0;
+        goto out;
 
     if (migrate_fd >= 0) {
         if (d_config.c_info.name) {

@@ -1477,8 +1488,9 @@ start:
     if (!paused)
         libxl_domain_unpause(&ctx, domid);
 
+    ret = domid; /* caller gets success in parent */
     if (!daemonize)
-        return domid; /* caller gets success in parent */
+        goto out;
 
     if (need_daemon) {
         char *fullname, *name;

@@ -1605,6 +1617,12 @@ error_out:
 error_out:
     if (domid)
         libxl_domain_destroy(&ctx, domid, 0);
+out:
+
+    free_domain_config(&d_config);
+
+    free(config_data);
+
     return ret;
 }
 

@@ -2143,6 +2161,7 @@ void list_domains_details(const libxl_do
         memset(&d_config, 0x00, sizeof(d_config));
         parse_config_data(config_file, (char *)data, len, &d_config, &dm_info);
         printf_info(info[i].domid, &d_config, &dm_info);
+        free_domain_config(&d_config);
         free(data);
         free(config_file);
     }

These patches are all good news. You should be aware (if not already) that we have a plan for dealing with the majority of remaining leaks, I was going to implement it after my current PCI work. So if you are going to go all the way with this we should talk... :)

Thanks

what's this plan ?

Vincent

It's no big secret or mystery - I only mentioned it because I had planned to start work on it quite soon :)

Basically it is to implement properly the current pointer tracking code in libxl such that allocations via libxl_(sprintf|malloc) and so on are automatically free'd when returning out of the library to a caller. Objects returned to callers will still be expected to be free()'d...

Of course, this doesn't address xl tool itself, so fixes there shouldn't clash or duplicate effort.

Gianni

What about, what's wrong with the original design ?
the original design being you stuff everything in the context memtrack and expect all the objects allocated by libxl (internal AND returned to the caller) to be free by a ctx_free. This provides a strong proven guarantee that *everything* has been free.

Vincent

I wasn't aware that was the original design. It's certainly not the case right now.

AFAICS that scheme would only guarantee everything has been freed if the caller calls ctx_free() at appropriate points. If libxl were used in a daemon, for example, it would not be simple to come up with a scheme that guarantees memory bounds that are independent from uptime.

Also, a simpler way to free everything is exit() :)

it has unfortunately diverged in some calls indeed.

This scheme is already in place in the ocaml binding

Vincent

I actually prefer explicit free's on the returned objects. That gives callers a lot more control. Have you seen Ians patch auto-generating that code? I think this approach combined with automatic-freeing of scratch data used in libxl calls is the best of both worlds.

I don't know about ocaml but assume it's trivial to call a libxl_*_free function when an object which encapsulates a libxl returned object is destroyed?

How much control do you actually need ?

In python you'ld have the approach:

my_function_xl_binded()
{
fill_structure(&structure);
CTX_INIT;
do_xl_call(&structure);
pyval= convert_to_python_values(&structure);
CTX_FREE;
return pyval;
}

in OCaml exactly the same.

how is that an improvement for python and ocaml bindings that you have to insert the right call in some functions to do some more freeing ?

It also save having to generate freeing code.

it's possible and not very hard, it doesn't mean that should be done though.
I really like the braindead approch of after i called libxl_ctx_free, i don't have anything to do with memory by design. You're advocating for having to put the right call in the right functions in the place that need it.

Vincent

I don't suppose it's an improvement just not a significant complication. It does improve the situation for C callers who aren't going to want to copy the returned values that they want to keep needlessly, it's not the usual behaviour of libraries which return pointers to things.

Typically the compound data structures which require freeing are encapsulated in an object in the wrapper. Just use the destructor to free it. It also means you can use PyStaticString_* instead of PyString_* for example.

It prevents multiple threads using the same context safely without serialising on those free calls. Let libxl free it's internal scratch data, let the callers free things they've explicitly requested...

Gianni

free_structure(&structure); // free stuff dynamically allocated by fill_structure

This gets more complicated if do_xl_call may have potentially reallocated or otherwise changed some members of &structure (I think we had a similar conversation to this one in the context of libxl_run_bootloader)

How does libxl know how it should free the existing value before replacing it (or should it just add it to the context instead?). How does the caller know if/when this has happened and how does it then go about freeing it or not as necessary on cleanup?

Mandating that all data passed over the libxl call boundary must be explicitly freed avoids all of this, is more in keeping with how people expect a library to behave and isn't really any more lines of code than the above (at least given the presence of free_foo() style helpers, which it turns out are needed anyway).

The alternative is to go completely the other way and mandate all data passed of the boundary must be registered with the context, which would entail passing a ctx to fill_structure (and maybe convert_to_... as well), sprinkling libxl_ptr_add around the place, exporting libxl_strdup etc etc. I'm not sure where the win is here.

Mixing and matching these two schemes (plus mixing in static data), which is what xl and the ocaml bindings both do today, just isn't workable if we expect people to consistently have a decent chance of writing correct programs using the library.

Ian.

Libxl should be as easy to use as possible, even by people not following xen-devel, for this reason I really like the explicit frees, simply because it is how everybody else does it, no risks of being misunderstood.

all applied, thanks