[PATCH 0 of 3] libxl: memory leaks

[PATCH 0 of 3] libxl: memory leaks

[Ian Campbell]

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.

[PATCH 1 of 3] libxc: free thread specific hypercall buffer on xc_interface_close

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# 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.

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

diff -r f5f5949d98f0 -r b3e1074b137c tools/libxc/xc_private.c
--- a/tools/libxc/xc_private.c	Fri Jul 30 09:13:11 2010 +0100
+++ b/tools/libxc/xc_private.c	Mon Aug 02 13:30:37 2010 +0100
@@ -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)

[PATCH 2 of 3] libxl: fix memory leak in libxl_name_to_domid

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# 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)

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

diff -r b3e1074b137c -r 2a932193a3fa tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c	Mon Aug 02 13:30:37 2010 +0100
+++ b/tools/libxl/libxl_utils.c	Mon Aug 02 13:30:37 2010 +0100
@@ -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)

[PATCH 3 of 3] xl: fix memory leaks in xl create

[Ian Campbell]

# HG changeset patch
# User Ian Campbell <ian.campbell@citrix.com>
# 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)

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

diff -r 2a932193a3fa -r e2e90fac665e tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Mon Aug 02 13:30:37 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Mon Aug 02 13:30:37 2010 +0100
@@ -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);
     }

Re: [PATCH 0 of 3] libxl: memory leaks

[Gianni Tedesco]

On Mon, 2010-08-02 at 13:31 +0100, Ian Campbell wrote:
> 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.

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

Re: [PATCH 0 of 3] libxl: memory leaks

[Vincent Hanquez]

On 02/08/10 14:11, Gianni Tedesco wrote:
> 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... :)
>    
what's this plan ?

-- 
Vincent

Re: [PATCH 0 of 3] libxl: memory leaks

[Gianni Tedesco]

On Mon, 2010-08-02 at 14:20 +0100, Vincent Hanquez wrote:
> On 02/08/10 14:11, Gianni Tedesco wrote:
> > 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... :)
> >    
> what's this plan ?

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

Re: [PATCH 0 of 3] libxl: memory leaks

[Vincent Hanquez]

On 02/08/10 15:05, Gianni Tedesco (3P) wrote:
> 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...

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

Re: [PATCH 0 of 3] libxl: memory leaks

[Gianni Tedesco]

On Tue, 2010-08-03 at 08:59 +0100, Vincent Hanquez wrote:
> On 02/08/10 15:05, Gianni Tedesco (3P) wrote:
> > 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...
> 
> 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.

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() :)

Re: [PATCH 0 of 3] libxl: memory leaks

[Vincent Hanquez]

On 03/08/10 11:18, Gianni Tedesco (3P) wrote:
> I wasn't aware that was the original design. It's certainly not the case
> right now.

it has unfortunately diverged in some calls indeed.

> 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.

This scheme is already in place in the ocaml binding

-- 
Vincent

Re: [PATCH 0 of 3] libxl: memory leaks

[Gianni Tedesco]

On Tue, 2010-08-03 at 11:51 +0100, Vincent Hanquez wrote:
> On 03/08/10 11:18, Gianni Tedesco (3P) wrote:
> > I wasn't aware that was the original design. It's certainly not the case
> > right now.
> 
> it has unfortunately diverged in some calls indeed.
> 
> > 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.
> 
> This scheme is already in place in the ocaml binding

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?

Re: [PATCH 0 of 3] libxl: memory leaks

[Vincent Hanquez]

On 03/08/10 13:16, Gianni Tedesco (3P) wrote:
> 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.
>    
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.

> 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?
>    
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

Re: [PATCH 0 of 3] libxl: memory leaks

[Gianni Tedesco]

On Tue, 2010-08-03 at 14:37 +0100, Vincent Hanquez wrote:
> On 03/08/10 13:16, Gianni Tedesco (3P) wrote:
> > 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.
> >    
> 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 ?

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 also save having to generate freeing code.
> 
> > 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?
> >    
> 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.

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

Re: [PATCH 0 of 3] libxl: memory leaks

[Ian Campbell]

On Tue, 2010-08-03 at 14:37 +0100, Vincent Hanquez wrote:
> On 03/08/10 13:16, Gianni Tedesco (3P) wrote:
> > 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.
> >    
> 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;

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

>   return pyval;
> }

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.

Re: [PATCH 0 of 3] libxl: memory leaks

[Stefano Stabellini]

On Tue, 3 Aug 2010, Ian Campbell wrote:
> On Tue, 2010-08-03 at 14:37 +0100, Vincent Hanquez wrote:
> > On 03/08/10 13:16, Gianni Tedesco (3P) wrote:
> > > 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.
> > >    
> > 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;
> 
>     free_structure(&structure); // free stuff dynamically allocated by fill_structure 
> 
> >   return pyval;
> > }
> 
> 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.
> 

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.

Re: [PATCH 0 of 3] libxl: memory leaks

[Stefano Stabellini]

On Mon, 2 Aug 2010, Ian Campbell wrote:
> 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.
> 

all applied, thanks