From patchwork Tue Feb 16 00:20:56 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 583114 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id ECFD51402D6 for ; Tue, 16 Feb 2016 11:25:26 +1100 (AEDT) Received: from localhost ([::1]:37610 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVTSG-00064Q-WD for incoming@patchwork.ozlabs.org; Mon, 15 Feb 2016 19:25:25 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44743) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVTO8-0006Sg-P2 for qemu-devel@nongnu.org; Mon, 15 Feb 2016 19:21:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aVTO7-0002Q9-5x for qemu-devel@nongnu.org; Mon, 15 Feb 2016 19:21:08 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41207) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aVTO6-0002Pn-Tu for qemu-devel@nongnu.org; Mon, 15 Feb 2016 19:21:07 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 9B15577339; Tue, 16 Feb 2016 00:21:06 +0000 (UTC) Received: from red.redhat.com (ovpn-113-167.phx2.redhat.com [10.3.113.167]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u1G0L0sx011124; Mon, 15 Feb 2016 19:21:06 -0500 From: Eric Blake To: qemu-devel@nongnu.org Date: Mon, 15 Feb 2016 17:20:56 -0700 Message-Id: <1455582057-27565-13-git-send-email-eblake@redhat.com> In-Reply-To: <1455582057-27565-1-git-send-email-eblake@redhat.com> References: <1455582057-27565-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: armbru@redhat.com, Michael Roth Subject: [Qemu-devel] [PATCH v10 12/13] qapi: Delete unused visit_start_union() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Commit cee2dedb noticed that if you have a partial flat union (such as if an input parse failed due to a missing discriminator), calling the dealloc visitor could result in trying to dereference a NULL pointer if we attempted to visit an object branch without an earlier successful call to visit_start_implicit_struct() allocating the pointer for that branch. But the "fix" it implemented requires the use of a '.data' member in the union, which may or may not be the same size as other branches of the union (consider a 32-bit platform where one of the branches is an int64), which feels fairly dirty. Plus, as mentioned in that commit, it only works if you can assume that '.data' would be zero-initialized even if '.kind' was uninitialized, which is rather poor logic: our usage of visit_start_struct() happens to zero-initialize both fields, which means '.kind' is never truly uninitialized - but if we changed visit_start_struct() to use g_new() instead of g_new0(), then '.data' would not be any more reliable as a condition on whether to visit the branch matching '.kind', regardless of whether '.kind' was 0). Menawhile, now that we have just inlined the fields of all flat unions, there is no longer the possibility of a null pointer to dereference in the first place. Where the branch structure used to be separately allocated by visit_start_implicit_struct(), it is now just pointing to a subset of the memory already zero-allocated by visit_start_struct(). Thus, we can instead fix things to delete the misguided visit_start_union(), as it is no longer providing any benefit. And it finishes the cleanup we started in commit 7c91aabd when we deleted visit_end_union(). Generated code changes as follows: |@@ -2366,9 +2363,6 @@ void visit_type_ChardevBackend(Visitor * | if (err) { | goto out_obj; | } |- if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { |- goto out_obj; |- } | switch ((*obj)->type) { | case CHARDEV_BACKEND_KIND_FILE: | visit_type_ChardevFile(v, "data", &(*obj)->u.file, &err); Signed-off-by: Eric Blake --- v10: retitle, hoist earlier in series, rebase, drop R-b v9: no change v8: rebase to 'name' motion v7: rebase to earlier context changes, simplify 'obj && !*obj' condition based on contract v6: rebase due to deferring 7/46, and gen_err_check() improvements; rewrite gen_visit_implicit_struct() more like other patterns --- include/qapi/visitor.h | 1 - include/qapi/visitor-impl.h | 2 -- scripts/qapi-visit.py | 3 --- qapi/qapi-visit-core.c | 8 -------- qapi/qapi-dealloc-visitor.c | 26 -------------------------- 5 files changed, 40 deletions(-) diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h index c131a32..b8ae1b5 100644 --- a/include/qapi/visitor.h +++ b/include/qapi/visitor.h @@ -80,6 +80,5 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp); void visit_type_number(Visitor *v, const char *name, double *obj, Error **errp); void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp); -bool visit_start_union(Visitor *v, bool data_present, Error **errp); #endif diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h index 7905a28..c4af3e0 100644 --- a/include/qapi/visitor-impl.h +++ b/include/qapi/visitor-impl.h @@ -58,8 +58,6 @@ struct Visitor /* May be NULL; most useful for input visitors. */ void (*optional)(Visitor *v, const char *name, bool *present); - - bool (*start_union)(Visitor *v, bool data_present, Error **errp); }; void input_type_enum(Visitor *v, const char *name, int *obj, diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 68354d8..02f0122 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -246,9 +246,6 @@ void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj, Error if variants: ret += gen_err_check(label='out_obj') ret += mcgen(''' - if (!visit_start_union(v, !!(*obj)->u.data, &err) || err) { - goto out_obj; - } switch ((*obj)->%(c_name)s) { ''', c_name=c_name(variants.tag_member.name)) diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c index 6fa66f1..976106e 100644 --- a/qapi/qapi-visit-core.c +++ b/qapi/qapi-visit-core.c @@ -60,14 +60,6 @@ void visit_end_list(Visitor *v) v->end_list(v); } -bool visit_start_union(Visitor *v, bool data_present, Error **errp) -{ - if (v->start_union) { - return v->start_union(v, data_present, errp); - } - return true; -} - bool visit_optional(Visitor *v, const char *name, bool *present) { if (v->optional) { diff --git a/qapi/qapi-dealloc-visitor.c b/qapi/qapi-dealloc-visitor.c index 6667e8c..4eae555 100644 --- a/qapi/qapi-dealloc-visitor.c +++ b/qapi/qapi-dealloc-visitor.c @@ -169,31 +169,6 @@ static void qapi_dealloc_type_enum(Visitor *v, const char *name, int *obj, { } -/* If there's no data present, the dealloc visitor has nothing to free. - * Thus, indicate to visitor code that the subsequent union fields can - * be skipped. This is not an error condition, since the cleanup of the - * rest of an object can continue unhindered, so leave errp unset in - * these cases. - * - * NOTE: In cases where we're attempting to deallocate an object that - * may have missing fields, the field indicating the union type may - * be missing. In such a case, it's possible we don't have enough - * information to differentiate data_present == false from a case where - * data *is* present but happens to be a scalar with a value of 0. - * This is okay, since in the case of the dealloc visitor there's no - * work that needs to done in either situation. - * - * The current inability in QAPI code to more thoroughly verify a union - * type in such cases will likely need to be addressed if we wish to - * implement this interface for other types of visitors in the future, - * however. - */ -static bool qapi_dealloc_start_union(Visitor *v, bool data_present, - Error **errp) -{ - return data_present; -} - Visitor *qapi_dealloc_get_visitor(QapiDeallocVisitor *v) { return &v->visitor; @@ -224,7 +199,6 @@ QapiDeallocVisitor *qapi_dealloc_visitor_new(void) v->visitor.type_str = qapi_dealloc_type_str; v->visitor.type_number = qapi_dealloc_type_number; v->visitor.type_any = qapi_dealloc_type_anything; - v->visitor.start_union = qapi_dealloc_start_union; QTAILQ_INIT(&v->stack);