Markus Armbruster <arm...@redhat.com> writes: > Eric Blake <ebl...@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 <ebl...@redhat.com> >> Cc: qemu-sta...@nongnu.org >> Reviewed-by: Marc-André Lureau <marcandre.lur...@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; >> } [...]