| Message ID | 1450717720-9627-20-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > 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 = g_malloc0(sizeof(*e)); > > + assert(value); > e->value = value; > if (qobject_type(e->value) == QTYPE_QLIST) { > e->is_list_head = 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 = QTAILQ_FIRST(&qov->stack); > QObject *value; > + > + assert(e); > QTAILQ_REMOVE(&qov->stack, e, node); > value = 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 = 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 = 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 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); > } Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
On 01/05/2016 07:05 AM, Marc-André Lureau wrote: > Hi > > On Mon, Dec 21, 2015 at 6:08 PM, Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> >> Cc: qemu-stable@nongnu.org >> >> +++ 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. I added the comment based on Markus' request that I document how the stack is used; but yes, it does feel like a bit of churn since it changes in the next commit. If there's a reason to respin, I might change it to: Visitor visitor; /* Stack holds two pieces of information: the current root object in * slot 0, then a stack of N objects still being built in slots 1 * through N (for N+1 slots in use). * FIXME: The root object should be stored separately from the * stack, particularly since qmp_output_add_obj() behaves * differently when visiting two top-level scalars in a row than * it does for two objects (the second object is appended to the * first, since the first is placed in both slots 0 and 1 but only * popped from slot 1). */ > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >
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 = g_malloc0(sizeof(*e)); + assert(value); e->value = value; if (qobject_type(e->value) == QTYPE_QLIST) { e->is_list_head = 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 = QTAILQ_FIRST(&qov->stack); QObject *value; + + assert(e); QTAILQ_REMOVE(&qov->stack, e, node); value = 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 = 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 = 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 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); }
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 <eblake@redhat.com> 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(-)