| Message ID | 1453219845-30939-20-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > Cc: qemu-stable@nongnu.org > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > 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 = 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; @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 = 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 = 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(); > } 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 = 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-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); > }
Markus Armbruster <armbru@redhat.com> writes: > Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> >> Cc: qemu-stable@nongnu.org >> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> --- >> 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 [...] >> @@ -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 */ Let's stick to the stack terminology you used for qmp_output_push_obj(): /* 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; >> g_free(e); >> return value; > > @value cannot be null here, because we never push nulls. Worth an > assertion, just to help readers? > >> } [...] >> +/* Grab the most recent QObject from the stack, which must exist */ Stack terminology: /* * Peek at the top of the stack of QObject 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; >> } [...]
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); }