From patchwork Fri Jan 29 13:49:00 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eric Blake X-Patchwork-Id: 8163491 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id DD8019F440 for ; Fri, 29 Jan 2016 13:59:21 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 16BB820131 for ; Fri, 29 Jan 2016 13:59:21 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0C5F5202FE for ; Fri, 29 Jan 2016 13:59:20 +0000 (UTC) Received: from localhost ([::1]:34324 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP9a3-0005aE-HZ for patchwork-qemu-devel@patchwork.kernel.org; Fri, 29 Jan 2016 08:59:19 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41554) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP9QR-0003tB-5v for qemu-devel@nongnu.org; Fri, 29 Jan 2016 08:49:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aP9QP-0001Mj-ON for qemu-devel@nongnu.org; Fri, 29 Jan 2016 08:49:23 -0500 Received: from mx1.redhat.com ([209.132.183.28]:41796) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aP9QP-0001Ma-Hh; Fri, 29 Jan 2016 08:49:21 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 20EBC7AEAE; Fri, 29 Jan 2016 13:49:21 +0000 (UTC) Received: from red.redhat.com (ovpn-113-130.phx2.redhat.com [10.3.113.130]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0TDn5FY019157; Fri, 29 Jan 2016 08:49:20 -0500 From: Eric Blake To: qemu-devel@nongnu.org Date: Fri, 29 Jan 2016 06:49:00 -0700 Message-Id: <1454075341-13658-25-git-send-email-eblake@redhat.com> In-Reply-To: <1454075341-13658-1-git-send-email-eblake@redhat.com> References: <1454075341-13658-1-git-send-email-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: Michael Roth , marcandre.lureau@redhat.com, armbru@redhat.com, qemu-stable@nongnu.org Subject: [Qemu-devel] [PATCH v10 24/25] qmp: Fix reference-counting of qnull on empty output visit 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+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Commit 6c2f9a15 ensured that we would not return NULL when the caller used an output visitor but had nothing to visit. But in doing so, it added a FIXME about a reference count leak that could abort qemu in the (unlikely) case of SIZE_MAX such visits (more plausible on 32-bit). (Although that commit suggested we might fix it in time for 2.5, we ran out of time; fortunately, it is unlikely enough to bite that it was not worth worrying about during the 2.5 release.) This fixes things by documenting the internal contracts, and explaining why the internal function can return NULL and only the public facing interface needs to worry about qnull(), thus avoiding over-referencing the qnull_ global object. It does not, however, fix the stupidity of the stack mixing up two separate pieces of information; add a FIXME to explain that issue, which will be fixed shortly in a future patch. Signed-off-by: Eric Blake Cc: qemu-stable@nongnu.org --- v10: tweak comments and add assert, drop R-b v9: enhance commit message v8: rebase to earlier changes v7: cc qemu-stable, tweak some asserts, drop stale comment, add more comments v6: no change --- qapi/qmp-output-visitor.c | 41 ++++++++++++++++++++++++++++++++++------- tests/test-qmp-output-visitor.c | 2 ++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c index 6ab2e04..b0e3980 100644 --- a/qapi/qmp-output-visitor.c +++ b/qapi/qmp-output-visitor.c @@ -30,6 +30,15 @@ typedef QTAILQ_HEAD(QStack, QStackEntry) QStack; struct QmpOutputVisitor { Visitor visitor; + /* FIXME: we are abusing stack to hold two separate pieces of + * information: the current root object in slot 0, and the stack + * of N objects still being built in slots 1 through N (for N+1 + * slots in use). Worse, our behavior is inconsistent: + * qmp_output_add_obj() visiting two top-level scalars in a row + * discards the first in favor of the second, but visiting two + * top-level objects in a row tries to append the second object + * into the first (since the first object was placed in the stack + * in both slot 0 and 1, but only popped from slot 1). */ QStack stack; }; @@ -42,10 +51,12 @@ static QmpOutputVisitor *to_qov(Visitor *v) return container_of(v, QmpOutputVisitor, visitor); } +/* Push @value onto the stack of current QObjects being built */ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value) { QStackEntry *e = g_malloc0(sizeof(*e)); + assert(value); e->value = value; if (qobject_type(e->value) == QTYPE_QLIST) { e->is_list_head = true; @@ -53,44 +64,53 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov, QObject *value) QTAILQ_INSERT_HEAD(&qov->stack, e, node); } +/* Pop a value off the stack of QObjects being built, and return it. */ static QObject *qmp_output_pop(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_FIRST(&qov->stack); QObject *value; + + assert(e); QTAILQ_REMOVE(&qov->stack, e, node); value = e->value; + assert(value); g_free(e); return value; } +/* Grab the root QObject, if any */ static QObject *qmp_output_first(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_LAST(&qov->stack, QStack); - /* - * FIXME Wrong, because qmp_output_get_qobject() will increment - * the refcnt *again*. We need to think through how visitors - * handle null. - */ if (!e) { - return qnull(); + /* No root */ + return NULL; } - + assert(e->value); return e->value; } +/* Peek at the top of the stack of QObjects being built. + * The stack must not be empty. */ static QObject *qmp_output_last(QmpOutputVisitor *qov) { QStackEntry *e = QTAILQ_FIRST(&qov->stack); + + assert(e && e->value); return e->value; } +/* Add @value to the current QObject being built. + * If the stack is visiting a dictionary or list, @value is now owned + * by that container. Otherwise, @value is now the root. */ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, QObject *value) { QObject *cur; if (QTAILQ_EMPTY(&qov->stack)) { + /* Stack was empty, track this object as root */ qmp_output_push_obj(qov, value); return; } @@ -99,13 +119,17 @@ static void qmp_output_add_obj(QmpOutputVisitor *qov, const char *name, switch (qobject_type(cur)) { case QTYPE_QDICT: + assert(name); qdict_put_obj(qobject_to_qdict(cur), name, value); break; case QTYPE_QLIST: qlist_append_obj(qobject_to_qlist(cur), value); break; default: + /* The previous root was a scalar, replace it with a new root */ + /* FIXME this is abusing the stack; see comment above */ qobject_decref(qmp_output_pop(qov)); + assert(QTAILQ_EMPTY(&qov->stack)); qmp_output_push_obj(qov, value); break; } @@ -205,11 +229,14 @@ static void qmp_output_type_any(Visitor *v, const char *name, QObject **obj, qmp_output_add_obj(qov, name, *obj); } +/* Finish building, and return the root object. Will not be NULL. */ QObject *qmp_output_get_qobject(QmpOutputVisitor *qov) { QObject *obj = qmp_output_first(qov); if (obj) { qobject_incref(obj); + } else { + obj = qnull(); } return obj; } diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visitor.c index 4df94bc..26dc752 100644 --- a/tests/test-qmp-output-visitor.c +++ b/tests/test-qmp-output-visitor.c @@ -461,6 +461,8 @@ static void test_visitor_out_empty(TestOutputVisitorData *data, arg = qmp_output_get_qobject(data->qov); g_assert(qobject_type(arg) == QTYPE_QNULL); + /* Check that qnull reference counting is sane */ + g_assert(arg->refcnt == 2); qobject_decref(arg); }