Re: [Qemu-devel] [PATCH 2/3] qom/object.c: fix string_output_get_string() memory leak

[address@hidden]

On Fri, 2014-08-01 at 23:26 +1000, Peter Crosthwaite wrote: 
> On Fri, Aug 1, 2014 at 8:22 PM, Chen Fan <address@hidden> wrote:
> > string_output_get_string() always return the data the sov->string
> 
> "returns the data sov->string points to and never frees it"
> 
> Although "sov" is a little out of context however of this change, and
> you need to go diving into qapi code to get context on what you mean
> by that. Perhaps just say it uses g_string_free which requires callers
> to free the returned data?
> 
> > point. and never free.
> >
> > Signed-off-by: Chen Fan <address@hidden>
> > ---
> >  hmp.c        |  6 ++++--
> >  qom/object.c | 11 ++++++++---
> >  2 files changed, 12 insertions(+), 5 deletions(-)
> >
> > diff --git a/hmp.c b/hmp.c
> > index 4d1838e..2414cc7 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1687,6 +1687,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
> >      MemdevList *memdev_list = qmp_query_memdev(&err);
> >      MemdevList *m = memdev_list;
> >      StringOutputVisitor *ov;
> > +    char *str;
> >      int i = 0;
> >
> >
> > @@ -1704,10 +1705,11 @@ void hmp_info_memdev(Monitor *mon, const QDict 
> > *qdict)
> >                         m->value->prealloc ? "true" : "false");
> >          monitor_printf(mon, "  policy: %s\n",
> >                         HostMemPolicy_lookup[m->value->policy]);
> > -        monitor_printf(mon, "  host nodes: %s\n",
> > -                       string_output_get_string(ov));
> > +        str = string_output_get_string(ov);
> > +        monitor_printf(mon, "  host nodes: %s\n", str);
> >
> >          string_output_visitor_cleanup(ov);
> > +        g_free(str);
> >          m = m->next;
> >          i++;
> >      }
> > diff --git a/qom/object.c b/qom/object.c
> > index 0e8267b..7ae4f33 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -948,16 +948,18 @@ int object_property_get_enum(Object *obj, const char 
> > *name,
> >  {
> >      StringOutputVisitor *sov;
> >      StringInputVisitor *siv;
> > +    char *str;
> >      int ret;
> >
> >      sov = string_output_visitor_new(false);
> >      object_property_get(obj, string_output_get_visitor(sov), name, errp);
> > -    siv = string_input_visitor_new(string_output_get_string(sov));
> > +    str = string_output_get_string(sov);
> > +    siv = string_input_visitor_new(str);
> 
> Free here (ASAP) instead of below.
here, I think free here is incorrect, the below visit_type_enum() will 
call visit_type_str which strdups the 'str', we should not free it
before that, so let it there.

Thanks,
Chen 
> 
> >      string_output_visitor_cleanup(sov);
> >      visit_type_enum(string_input_get_visitor(siv),
> >                      &ret, strings, NULL, name, errp);
> >      string_input_visitor_cleanup(siv);
> > -
> 
> Don't delete existing blank lines that separate sections like this.
> 
> > +    g_free(str);
> >      return ret;
> >  }
> >
> > @@ -966,15 +968,18 @@ void object_property_get_uint16List(Object *obj, 
> > const char *name,
> >  {
> >      StringOutputVisitor *ov;
> >      StringInputVisitor *iv;
> > +    char *str;
> >
> >      ov = string_output_visitor_new(false);
> >      object_property_get(obj, string_output_get_visitor(ov),
> >                          name, errp);
> > -    iv = string_input_visitor_new(string_output_get_string(ov));
> > +    str = string_output_get_string(ov);
> > +    iv = string_input_visitor_new(str);
> 
> Ditto on the ASAP free
> 
> Regards,
> Peter
> 
> >      visit_type_uint16List(string_input_get_visitor(iv),
> >                            list, NULL, errp);
> >      string_output_visitor_cleanup(ov);
> >      string_input_visitor_cleanup(iv);
> > +    g_free(str);
> >  }
> >
> >  void object_property_parse(Object *obj, const char *string,
> > --
> > 1.9.3
> >
> >