Re: [Xen-devel] [PATCH for-4.5] xl: fix two memory leaks

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Mon, 2014-12-01 at 10:10 +0000, Wei Liu wrote:
> On Mon, Dec 01, 2014 at 09:34:47AM +0000, Ian Campbell wrote:
> > On Sun, 2014-11-30 at 21:54 +0000, Wei Liu wrote:
> > > There are two invocations of libxl_basename, which returns a malloc'ed
> > > string. Those strings should be freed after used.
> > > 
> > > Signed-off-by: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> > > ---
> > >  tools/libxl/xl_cmdimpl.c |    9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index 9afef3f..716a865 100644
> > > --- a/tools/libxl/xl_cmdimpl.c
> > > +++ b/tools/libxl/xl_cmdimpl.c
> > > @@ -920,6 +920,7 @@ static void parse_config_data(const char 
> > > *config_source,
> > >      int pci_permissive = 0;
> > >      int pci_seize = 0;
> > >      int i, e;
> > > +    const char *basename;
> > >  
> > >      libxl_domain_create_info *c_info = &d_config->c_info;
> > >      libxl_domain_build_info *b_info = &d_config->b_info;
> > > @@ -1116,13 +1117,16 @@ static void parse_config_data(const char 
> > > *config_source,
> > >  
> > >      switch(b_info->type) {
> > >      case LIBXL_DOMAIN_TYPE_HVM:
> > > -        if (!strcmp(libxl_basename(b_info->kernel), "hvmloader")) {
> > > +        basename = libxl_basename(b_info->kernel);
> > > +        if (!strcmp(basename, "hvmloader")) {
> > >              fprintf(stderr, "WARNING: you seem to be using \"kernel\" "
> > >                      "directive to override HVM guest firmware. Ignore "
> > >                      "that. Use \"firmware_override\" instead if you "
> > >                      "really want a non-default firmware\n");
> > >              b_info->kernel = NULL;
> > >          }
> > > +        free((void*)basename);
> > 
> > I think you should un-const the declaration (in both cases) rather than
> > adding casts.
> > 
> 
> Is it OK to change the declaration of a public API?

Oh god, libxl_basename returns a dynamically allocated string as a
const. That's a bit mad... At least it is clearly documented as being a
strdup'd thing!

I think we may need to use LIBXL_API_VERSION here to conditionally
include the const. We've done that before see
LIBXL_HAVE_NONCONST_EVENT_OCCURS_EVENT_ARG.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel