[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH v8 19/35] qmp: Fix reference-counting of qnull on em
From: |
Eric Blake |
Subject: |
[Qemu-devel] [PATCH v8 19/35] qmp: Fix reference-counting of qnull on empty output visit |
Date: |
Mon, 21 Dec 2015 10:08:24 -0700 |
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 <address@hidden>
Cc: address@hidden
---
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;
}
+/* 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);
}
--
2.4.3
- [Qemu-devel] [PATCH v8 11/35] qapi: Consolidate visitor small integer callbacks, (continued)
- [Qemu-devel] [PATCH v8 11/35] qapi: Consolidate visitor small integer callbacks, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 07/35] qapi: Improve generated event use of qapi visitor, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 17/35] qapi: Drop unused 'kind' for struct/enum visit, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 13/35] qom: Use typedef for Visitor, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 20/35] qmp: Don't abuse stack to track qmp-output root, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 18/35] qapi: Drop unused error argument for list and implicit struct, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 27/35] qapi: Fix command with named empty argument type, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 01/35] qobject: Document more shortcomings in our number handling, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 22/35] qapi: Add visit_type_null() visitor, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 25/35] qapi: Simplify excess input reporting in input visitors, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 19/35] qmp: Fix reference-counting of qnull on empty output visit,
Eric Blake <=
- [Qemu-devel] [PATCH v8 21/35] qapi: Document visitor interfaces, add assertions, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 28/35] qapi: Eliminate empty visit_type_FOO_fields, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 24/35] spapr_drc: Expose 'null' in qom-get when there is no fdt, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 31/35] qapi: Rework deallocation of partial struct, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 34/35] qapi: Change visit_type_FOO() to no longer return partial objects, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 32/35] qapi: Split visit_end_struct() into pieces, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 35/35] RFC: qapi: Adjust layout of FooList types, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 23/35] qmp: Tighten output visitor rules, Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 33/35] qapi: Simplify semantics of visit_next_list(), Eric Blake, 2015/12/21
- [Qemu-devel] [PATCH v8 16/35] qapi: Swap 'name' in visit_* callbacks to match public API, Eric Blake, 2015/12/21