From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aGSEv-0006zn-AB for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:05:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aGSEq-0000LH-Qd for qemu-devel@nongnu.org; Tue, 05 Jan 2016 09:05:33 -0500 MIME-Version: 1.0 In-Reply-To: <1450717720-9627-20-git-send-email-eblake@redhat.com> References: <1450717720-9627-1-git-send-email-eblake@redhat.com> <1450717720-9627-20-git-send-email-eblake@redhat.com> Date: Tue, 5 Jan 2016 15:05:27 +0100 Message-ID: From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v8 19/35] 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: Markus Armbruster , qemu-stable , QEMU , Michael Roth Hi On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake wrote: > 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. > > Signed-off-by: Eric Blake > Cc: qemu-stable@nongnu.org > > --- > 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). */ I skipped checking thoroughly this comment, since it's a bit off-topic, although it looks ok. Later, oh well, it's fixed in next commit. Imho it's not strictly necessary in this commit. > 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; > } > > +/* Grab the root QObject, if any, 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; > } > > +/* 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(); > } > return obj; > } > 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); > } Reviewed-by: Marc-Andr=C3=A9 Lureau --=20 Marc-Andr=C3=A9 Lureau