* [PATCH] qapi: record the last element in order to avoid memory leak
@ 2020-07-15 15:11 Li Qiang
2020-07-16 12:42 ` Markus Armbruster
0 siblings, 1 reply; 2+ messages in thread
From: Li Qiang @ 2020-07-15 15:11 UTC (permalink / raw)
To: armbru, mdroth, pbonzini; +Cc: Li Qiang, liq3ea, qemu-devel
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
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] qapi: record the last element in order to avoid memory leak
2020-07-15 15:11 [PATCH] qapi: record the last element in order to avoid memory leak Li Qiang
@ 2020-07-16 12:42 ` Markus Armbruster
0 siblings, 0 replies; 2+ messages in thread
From: Markus Armbruster @ 2020-07-16 12:42 UTC (permalink / raw)
To: Li Qiang; +Cc: qemu-devel, pbonzini, liq3ea, armbru, mdroth
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)) {
^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-16 12:44 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 15:11 [PATCH] qapi: record the last element in order to avoid memory leak Li Qiang
2020-07-16 12:42 ` Markus Armbruster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).