From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55589) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKwO6-0001Zw-9Z for qemu-devel@nongnu.org; Thu, 30 Jul 2015 18:33:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKwO2-00016W-Ul for qemu-devel@nongnu.org; Thu, 30 Jul 2015 18:33:18 -0400 Received: from resqmta-po-05v.sys.comcast.net ([2001:558:fe16:19:96:114:154:164]:38941) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKwO2-00015n-O7 for qemu-devel@nongnu.org; Thu, 30 Jul 2015 18:33:14 -0400 From: Eric Blake Date: Thu, 30 Jul 2015 16:33:07 -0600 Message-Id: <1438295587-19069-1-git-send-email-eblake@redhat.com> In-Reply-To: <1435782155-31412-13-git-send-email-armbru@redhat.com> References: <1435782155-31412-13-git-send-email-armbru@redhat.com> Subject: [Qemu-devel] [RFC PATCH 12.5/47] qapi: Document that input visitor semantics are prone to leaks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Markus Armbruster , Michael Roth Most functions that can return a pointer or set an Error ** value are decent enough to guarantee a NULL return when reporting an error. Not so with our generated qapi visitor functions. If the caller is not careful to clean up partially-allocated objects on error, then the caller suffers a memory leak. Properly fixing it is probably complex enough to save for a later day, so merely document it for now. Signed-off-by: Eric Blake --- As mentioned elsewhere in the thread (comments on 29/47 https://lists.gnu.org/archive/html/qemu-devel/2015-07/msg06107.html) it would be worth documenting a FIXME for a future series. I'm submitting this as 12.5/47 due to its relation to other similar shortcoming doc patches; and assuming Markus can rebase the rest of the series on top if he wants to fold it into his v3 posting at this spot. Otherwise, I can wait for his v3 and rebase it to be part of my (growing) followup series. scripts/qapi-visit.py | 4 ++++ tests/test-qmp-input-visitor.c | 2 ++ 2 files changed, 6 insertions(+) diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 73f136f..eec5f1f 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -115,6 +115,10 @@ out: def generate_visit_struct_body(name): + # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to + # *obj, but then visit_type_FOO_fields() fails, we should clean up *obj + # rather than leaving it non-NULL. As currently written, the caller must + # call qapi_free_FOO() to avoid a memory leak of the partial FOO. ret = mcgen(''' Error *err = NULL; diff --git a/tests/test-qmp-input-visitor.c b/tests/test-qmp-input-visitor.c index b7a87ee..a5cfefa 100644 --- a/tests/test-qmp-input-visitor.c +++ b/tests/test-qmp-input-visitor.c @@ -636,6 +636,8 @@ static void test_visitor_in_errors(TestInputVisitorData *data, visit_type_TestStruct(v, &p, NULL, &err); g_assert(err); + /* FIXME - a failed parse should not leave a partially-allocated p + * for us to clean up; this could cause callers to leak memory. */ g_assert(p->string == NULL); error_free(err); -- 2.4.3