The split between xl.c and xl_cmdimpl.c made this a bit odd, I chose to add a
destructor to xl_cmdimpl.c which is called from xl.c instead of making the
variable non-static since that seems like a nicer encapsulation.
==5915== 6 bytes in 1 blocks are still reachable in loss record 1 of 1
==5915== at 0x4025755: malloc (vg_replace_malloc.c:291)
==5915== by 0x424FDF3: read_message (xs.c:1145)
==5915== by 0x425017E: xs_talkv (xs.c:428)
==5915== by 0x4250445: xs_single (xs.c:546)
==5915== by 0x42504CE: xs_read (xs.c:592)
==5915== by 0x4074E2C: libxl_domid_to_name (libxl_utils.c:54)
==5915== by 0x804E3F2: find_domain (xl_cmdimpl.c:186)
==5915== by 0x8055B7E: main_destroy (xl_cmdimpl.c:3942)
==5915== by 0x804DD60: main (xl.c:371)
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: George.Dunlap@citrix.com
---
tools/libxl/xl.c | 2 ++
tools/libxl/xl.h | 2 ++
tools/libxl/xl_cmdimpl.c | 7 +++++--
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 0f3acb9..3201180 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -291,6 +291,8 @@ static void xl_ctx_free(void)
free(default_gatewaydev);
default_gatewaydev = NULL;
}
+
+ xl_cmdimpl_atexit();
}
int main(int argc, char **argv)
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 771b4af..5ae8d11 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -113,6 +113,8 @@ extern int cmdtable_len;
/* Look up a command in the table, allowing unambiguous truncation */
struct cmd_spec *cmdtable_lookup(const char *s);
+extern void xl_cmdimpl_atexit(void);
+
extern libxl_ctx *ctx;
extern xentoollog_logger_stdiostream *logger;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c1a969b..325b257 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -68,7 +68,7 @@ libxl_ctx *ctx;
xlchild children[child_max];
#define INVALID_DOMID ~0
-static const char *common_domname;
+static char *common_domname = NULL;
static int fd_lock = -1;
/* Stash for specific vcpu to pcpu mappping */
@@ -139,7 +139,10 @@ struct domain_create {
char **migration_domname_r; /* from malloc */
};
-
+void xl_cmdimpl_atexit(void)
+{
+ free(common_domname); common_domname = NULL;
+}
static int qualifier_to_id(const char *p, uint32_t *id_r)
{
--
1.7.2.5
On Wed, Jun 12, 2013 at 2:35 PM, Ian Campbell <ian.campbell@citrix.com> wrote:> The split between xl.c and xl_cmdimpl.c made this a bit odd, I chose to add a > destructor to xl_cmdimpl.c which is called from xl.c instead of making the > variable non-static since that seems like a nicer encapsulation. > > ==5915== 6 bytes in 1 blocks are still reachable in loss record 1 of 1 > ==5915== at 0x4025755: malloc (vg_replace_malloc.c:291) > ==5915== by 0x424FDF3: read_message (xs.c:1145) > ==5915== by 0x425017E: xs_talkv (xs.c:428) > ==5915== by 0x4250445: xs_single (xs.c:546) > ==5915== by 0x42504CE: xs_read (xs.c:592) > ==5915== by 0x4074E2C: libxl_domid_to_name (libxl_utils.c:54) > ==5915== by 0x804E3F2: find_domain (xl_cmdimpl.c:186) > ==5915== by 0x8055B7E: main_destroy (xl_cmdimpl.c:3942) > ==5915== by 0x804DD60: main (xl.c:371) > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: George.Dunlap@citrix.com > --- > tools/libxl/xl.c | 2 ++ > tools/libxl/xl.h | 2 ++ > tools/libxl/xl_cmdimpl.c | 7 +++++-- > 3 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c > index 0f3acb9..3201180 100644 > --- a/tools/libxl/xl.c > +++ b/tools/libxl/xl.c > @@ -291,6 +291,8 @@ static void xl_ctx_free(void) > free(default_gatewaydev); > default_gatewaydev = NULL; > } > + > + xl_cmdimpl_atexit(); > } > > int main(int argc, char **argv) > diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h > index 771b4af..5ae8d11 100644 > --- a/tools/libxl/xl.h > +++ b/tools/libxl/xl.h > @@ -113,6 +113,8 @@ extern int cmdtable_len; > /* Look up a command in the table, allowing unambiguous truncation */ > struct cmd_spec *cmdtable_lookup(const char *s); > > +extern void xl_cmdimpl_atexit(void); > + > extern libxl_ctx *ctx; > extern xentoollog_logger_stdiostream *logger; > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index c1a969b..325b257 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -68,7 +68,7 @@ libxl_ctx *ctx; > xlchild children[child_max]; > > #define INVALID_DOMID ~0 > -static const char *common_domname; > +static char *common_domname = NULL;Why is "const" being removed? -George
On Wed, 2013-06-12 at 16:12 +0100, George Dunlap wrote:> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > > index c1a969b..325b257 100644 > > --- a/tools/libxl/xl_cmdimpl.c > > +++ b/tools/libxl/xl_cmdimpl.c > > @@ -68,7 +68,7 @@ libxl_ctx *ctx; > > xlchild children[child_max]; > > > > #define INVALID_DOMID ~0 > > -static const char *common_domname; > > +static char *common_domname = NULL; > > Why is "const" being removed?free takes a non-const void *, so it has to be non-const to free it. I had checked that all the places which assigned here were dynamically allocated and not e.g. string literals but double checking that I missed that the one in create_domain takes it from the c_info without dupping it, which would lead to a double free. There''s probably a leak of the original name in there too. With that in mind I nack my own patch ;-) From what I can tell the right fix is too big for 4.3 so I''ll leave this until 4.4. Ian.