From 5666dd19dd95d966be00d0c3e7efdfaecc096ff3 Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Thu, 15 Sep 2011 14:39:52 -0500 Subject: qapi: dealloc visitor, fix premature free and iteration logic Currently we do 3 things wrong: 1) The list iterator, in practice, is used in a manner where the pointer we pass in is the same as the pointer we assign the output to from visit_next_list(). This causes an infinite loop where we keep freeing the same structures. 2) We attempt to free list->value rather than list. visit_type_ handles this. We should only be concerned with the containing list. 3) We free prematurely: iterator function will continue accessing values we've already freed. This patch should fix all of these issues. QmpOutputVisitor also suffers from 1). Reviewed-by: Anthony Liguori Signed-off-by: Michael Roth Signed-off-by: Luiz Capitulino --- qapi/qapi-dealloc-visitor.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) (limited to 'qapi/qapi-dealloc-visitor.c') diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index f6290611fe..6b586ade13 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -26,6 +26,7 @@ struct QapiDeallocVisitor { Visitor visitor; QTAILQ_HEAD(, StackEntry) stack; + bool is_list_head; }; static QapiDeallocVisitor *to_qov(Visitor *v) @@ -70,15 +71,24 @@ static void qapi_dealloc_end_struct(Visitor *v, Error **errp) static void qapi_dealloc_start_list(Visitor *v, const char *name, Error **errp) { + QapiDeallocVisitor *qov = to_qov(v); + qov->is_list_head = true; } -static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **list, +static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp, Error **errp) { - GenericList *retval = *list; - g_free(retval->value); - *list = retval->next; - return retval; + GenericList *list = *listp; + QapiDeallocVisitor *qov = to_qov(v); + + if (!qov->is_list_head) { + *listp = list->next; + g_free(list); + return *listp; + } + + qov->is_list_head = false; + return list; } static void qapi_dealloc_end_list(Visitor *v, Error **errp) -- cgit 1.4.1 From 0b9d854230737b214c01e89f0b679ea36fd59e5e Mon Sep 17 00:00:00 2001 From: Michael Roth Date: Mon, 19 Sep 2011 19:03:10 -0500 Subject: qapi: dealloc visitor, support freeing of nested lists Previously our logic for keeping track of when we're visiting the head of a list was done via a global bool. This can be overwritten if dealing with nested lists, so use stack entries to track this instead. Reviewed-by: Anthony Liguori Signed-off-by: Michael Roth Signed-off-by: Luiz Capitulino --- qapi/qapi-dealloc-visitor.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) (limited to 'qapi/qapi-dealloc-visitor.c') diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 6b586ade13..a154523731 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -19,6 +19,7 @@ typedef struct StackEntry { void *value; + bool is_list_head; QTAILQ_ENTRY(StackEntry) node; } StackEntry; @@ -39,6 +40,11 @@ static void qapi_dealloc_push(QapiDeallocVisitor *qov, void *value) StackEntry *e = g_malloc0(sizeof(*e)); e->value = value; + + /* see if we're just pushing a list head tracker */ + if (value == NULL) { + e->is_list_head = true; + } QTAILQ_INSERT_HEAD(&qov->stack, e, node); } @@ -72,7 +78,7 @@ static void qapi_dealloc_end_struct(Visitor *v, Error **errp) static void qapi_dealloc_start_list(Visitor *v, const char *name, Error **errp) { QapiDeallocVisitor *qov = to_qov(v); - qov->is_list_head = true; + qapi_dealloc_push(qov, NULL); } static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp, @@ -80,19 +86,27 @@ static GenericList *qapi_dealloc_next_list(Visitor *v, GenericList **listp, { GenericList *list = *listp; QapiDeallocVisitor *qov = to_qov(v); + StackEntry *e = QTAILQ_FIRST(&qov->stack); - if (!qov->is_list_head) { - *listp = list->next; - g_free(list); - return *listp; + if (e && e->is_list_head) { + e->is_list_head = false; + return list; } - qov->is_list_head = false; - return list; + if (list) { + list = list->next; + g_free(*listp); + return list; + } + + return NULL; } static void qapi_dealloc_end_list(Visitor *v, Error **errp) { + QapiDeallocVisitor *qov = to_qov(v); + void *obj = qapi_dealloc_pop(qov); + assert(obj == NULL); /* should've been list head tracker with no payload */ } static void qapi_dealloc_type_str(Visitor *v, char **obj, const char *name, -- cgit 1.4.1