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

[Peter Crosthwaite]

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.

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