From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40222) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aMCSZ-0001l4-4n for qemu-devel@nongnu.org; Thu, 21 Jan 2016 05:27:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aMCSV-0000EH-Rg for qemu-devel@nongnu.org; Thu, 21 Jan 2016 05:27:23 -0500 From: Markus Armbruster References: <1453219845-30939-1-git-send-email-eblake@redhat.com> <1453219845-30939-20-git-send-email-eblake@redhat.com> Date: Thu, 21 Jan 2016 11:27:16 +0100 In-Reply-To: <1453219845-30939-20-git-send-email-eblake@redhat.com> (Eric Blake's message of "Tue, 19 Jan 2016 09:10:27 -0700") Message-ID: <87powvi5x7.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v9 19/37] qmp: Fix reference-counting of qnull on empty output visit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: marcandre.lureau@redhat.com, qemu-stable@nongnu.org, qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > 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 > Reviewed-by: Marc-Andr=C3=A9 Lureau > > --- > 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 | 39 ++++++++++++++++++++++++++++++++---= ---- > tests/test-qmp-output-visitor.c | 2 ++ > 2 files changed, 34 insertions(+), 7 deletions(-) > > diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c > index d367148..316f4e4 100644 > --- a/qapi/qmp-output-visitor.c > +++ b/qapi/qmp-output-visitor.c > @@ -29,6 +29,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; > }; > > @@ -41,10 +50,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 =3D g_malloc0(sizeof(*e)); > > + assert(value); > e->value =3D value; > if (qobject_type(e->value) =3D=3D QTYPE_QLIST) { > e->is_list_head =3D true; > @@ -52,44 +63,51 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov= , QObject *value) > QTAILQ_INSERT_HEAD(&qov->stack, e, node); > } > > +/* Grab and remove the most recent QObject from the stack */ > static QObject *qmp_output_pop(QmpOutputVisitor *qov) > { > QStackEntry *e =3D QTAILQ_FIRST(&qov->stack); > QObject *value; > + > + assert(e); > QTAILQ_REMOVE(&qov->stack, e, node); > value =3D e->value; > g_free(e); > return value; @value cannot be null here, because we never push nulls. Worth an assertion, just to help readers? > } > > +/* Grab the root QObject, if any, in preparation to empty the stack */ Why is this "in preparation to empty the stack"? > static QObject *qmp_output_first(QmpOutputVisitor *qov) > { > QStackEntry *e =3D 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; > } Two callers to check: qmp_output_get_qobject() and qmp_output_visitor_cleanup(). See below. > > +/* Grab the most recent QObject from the stack, which must exist */ > static QObject *qmp_output_last(QmpOutputVisitor *qov) > { > QStackEntry *e =3D 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; > } > @@ -98,13 +116,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 +227,14 @@ static void qmp_output_type_any(Visitor *v, const c= har *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 =3D qmp_output_first(qov); > if (obj) { > qobject_incref(obj); > + } else { > + obj =3D qnull(); > } qmp_output_first() now returns NULL instead of qnull(). This changes compensates; taken together, the beavior is the same, except we avoid the qobject_incref() in the qnull() case. That's the bug fix. Aside: I believe obj could not be null before this patch, so the conditional was redundant. No need to complicate the commit message with this, I think. > return obj; > } Unchanged: void qmp_output_visitor_cleanup(QmpOutputVisitor *v) { QStackEntry *e, *tmp; /* The bottom QStackEntry, if any, owns the root QObject. See the * qmp_output_push_obj() invocations in qmp_output_add_obj(). */ QObject *root =3D QTAILQ_EMPTY(&v->stack) ? NULL : qmp_output_first(= v); Calls qmp_output_first() only when the stack isn't empty, and then qmp_output_first() behaves exactly as before this patch. Good. QTAILQ_FOREACH_SAFE(e, &v->stack, node, tmp) { QTAILQ_REMOVE(&v->stack, e, node); g_free(e); } qobject_decref(root); g_free(v); } > diff --git a/tests/test-qmp-output-visitor.c b/tests/test-qmp-output-visi= tor.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(TestOutputVisitorD= ata *data, > > arg =3D qmp_output_get_qobject(data->qov); > g_assert(qobject_type(arg) =3D=3D QTYPE_QNULL); > + /* Check that qnull reference counting is sane */ > + g_assert(arg->refcnt =3D=3D 2); > qobject_decref(arg); > }