[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] qapi: record the last element in order to avoid memory leak
From: |
Markus Armbruster |
Subject: |
Re: [PATCH] qapi: record the last element in order to avoid memory leak |
Date: |
Thu, 16 Jul 2020 14:42:16 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
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)) {