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

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

[Li Qiang]

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

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

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