Re: [PATCH] qapi: record the last element in order to avoid memory leak

[Markus Armbruster]

Li Qiang <liq3ea@163.com> writes:

> While executing 'tests/test-qobject-input-visitor'. I got
> following error:
>
> /visitor/input/fail/alternate: OK
> /visitor/input/fail/union-list: OK
>
> =================================================================
> ==4353==ERROR: LeakSanitizer: detected memory leaks
>
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x7f192d0c5d28 in __interceptor_calloc 
> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xded28)
>     #1 0x7f192cd21b10 in g_malloc0 
> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x51b10)
>     #2 0x556725f6bbee in visit_next_list qapi/qapi-visit-core.c:86
>     #3 0x556725f49e15 in visit_type_UserDefOneList tests/test-qapi-visit.c:474
>     #4 0x556725f4489b in test_visitor_in_fail_struct_in_list 
> tests/test-qobject-input-visitor.c:1086
>     #5 0x7f192cd42f29  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72f29)
>
> SUMMARY: AddressSanitizer: 16 byte(s) leaked in 1 allocation(s).

Good catch!

Regressed in commit cdd2b228b9 "qapi: Smooth visitor error checking in
generated code".

> This is because in 'visit_type_UserDefOneList' function when
> 'visit_type_UserDefOne' failed and we go to out_obj. And have
> no chance to process the last element. The path is:
> visit_type_UserDefOneList
>     ->visit_type_UserDefOne(error occured)
>         ->qapi_free_UserDefOneList
>             -->visit_type_UserDefOneList(again)
>
> In the last 'visit_type_UserDefOneList' we will free the elements
> allocated in the first 'visit_type_UserDefOneList'. However we delete
> the element in 'qapi_dealloc_next_list'. If then 'visit_type_UserDefOne'
> return false we will skip the element that still in the 'obj' linked
> list. This is why the ASAN complains this memory leak.
> This patch store the recent processing elements in 'QapiDeallocVisitor'.
> In 'qapi_dealloc_end_list' if it is not NULL, we free it.
>
> Signed-off-by: Li Qiang <liq3ea@163.com>

Before commit cdd2b228b9, visit_type_UserDefOne() succeeded for the last
element with the dealloc visitor.  Since then, it fails.  That's wrong.

> ---
>  qapi/qapi-dealloc-visitor.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c
> index ef283f2966..6335cadd9c 100644
> --- a/qapi/qapi-dealloc-visitor.c
> +++ b/qapi/qapi-dealloc-visitor.c
> @@ -20,8 +20,14 @@
>  struct QapiDeallocVisitor
>  {
>      Visitor visitor;
> +    void *last;
>  };
>  
> +static QapiDeallocVisitor *to_qdv(Visitor *v)
> +{
> +    return container_of(v, QapiDeallocVisitor, visitor);
> +}
> +
>  static bool qapi_dealloc_start_struct(Visitor *v, const char *name, void 
> **obj,
>                                        size_t unused, Error **errp)
>  {
> @@ -46,19 +52,25 @@ static bool qapi_dealloc_start_list(Visitor *v, const 
> char *name,
>                                      GenericList **list, size_t size,
>                                      Error **errp)
>  {
> +    QapiDeallocVisitor *qdv = to_qdv(v);
> +    qdv->last = *list;
>      return true;
>  }
>  
>  static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList *tail,
>                                             size_t size)
>  {
> +    QapiDeallocVisitor *qdv = to_qdv(v);
>      GenericList *next = tail->next;
> +    qdv->last = next;
>      g_free(tail);
>      return next;
>  }
>  
>  static void qapi_dealloc_end_list(Visitor *v, void **obj)
>  {
> +    QapiDeallocVisitor *qdv = to_qdv(v);
> +    g_free(qdv->last);
>  }
>  
>  static bool qapi_dealloc_type_str(Visitor *v, const char *name, char **obj,

I'm sure your patch plugs the leak.  But we should really make
visit_type_UserDefOne() behave as it did before I broke it.  This should
do:

diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 3fb2f30510..cdabc5fa28 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -249,6 +249,7 @@ bool visit_type_%(c_name)s(Visitor *v, const char *name, 
%(c_name)s **obj, Error
     if (!*obj) {
         /* incomplete */
         assert(visit_is_dealloc(v));
+        ok = true;
         goto out_obj;
     }
     if (!visit_type_%(c_name)s_members(v, *obj, errp)) {