From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35990) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9G3r-0000In-Gr for qemu-devel@nongnu.org; Thu, 29 Sep 2011 08:50:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R9G3n-0007ky-C6 for qemu-devel@nongnu.org; Thu, 29 Sep 2011 08:49:59 -0400 Received: from e6.ny.us.ibm.com ([32.97.182.146]:43445) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R9G3n-0007kd-9g for qemu-devel@nongnu.org; Thu, 29 Sep 2011 08:49:55 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by e6.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p8TCPgBi030738 for ; Thu, 29 Sep 2011 08:25:42 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p8TCnqb6168566 for ; Thu, 29 Sep 2011 08:49:52 -0400 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p8TCnofZ013669 for ; Thu, 29 Sep 2011 06:49:52 -0600 Message-ID: <4E846968.7010706@us.ibm.com> Date: Thu, 29 Sep 2011 07:49:44 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1317221085-5825-1-git-send-email-lcapitulino@redhat.com> <1317221085-5825-7-git-send-email-lcapitulino@redhat.com> In-Reply-To: <1317221085-5825-7-git-send-email-lcapitulino@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 06/21] qapi: dealloc visitor, fix premature free and iteration logic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: kwolf@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com On 09/28/2011 09:44 AM, Luiz Capitulino wrote: > From: Michael Roth > > 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). > > Signed-off-by: Michael Roth > Signed-off-by: Luiz Capitulino Reviewed-by: Anthony Liguori Regards, Anthony Liguori > --- > qapi/qapi-dealloc-visitor.c | 20 +++++++++++++++----- > 1 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c > index f629061..6b586ad 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)