[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PATCH 24/35] qmp: Fix reference-counting of qnull on empty
From: |
Michael Roth |
Subject: |
[Qemu-devel] [PATCH 24/35] qmp: Fix reference-counting of qnull on empty output visit |
Date: |
Mon, 21 Mar 2016 12:28:22 -0500 |
From: Eric Blake <address@hidden>
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 <address@hidden>
Cc: address@hidden
Message-Id: <address@hidden>
Signed-off-by: Markus Armbruster <address@hidden>
(cherry picked from commit a86156401559cb4401cf9ecc704faeab6fc8bb19)
Signed-off-by: Michael Roth <address@hidden>
---
qapi/qmp-output-visitor.c | 41 ++++++++++++++++++++++++++++++++++-------
tests/test-qmp-output-visitor.c | 2 ++
2 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/qapi/qmp-output-visitor.c b/qapi/qmp-output-visitor.c
index 29899ac..43cfebf 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,53 @@ static void qmp_output_push_obj(QmpOutputVisitor *qov,
QObject *value)
QTAILQ_INSERT_HEAD(&qov->stack, e, node);
}
+/* 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;
+ assert(value);
g_free(e);
return value;
}
+/* Grab the root QObject, if any */
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;
}
+/* Peek at the top of the stack of QObjects 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;
}
+/* 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 +118,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;
}
@@ -198,11 +222,14 @@ static void qmp_output_type_any(Visitor *v, QObject
**obj, const char *name,
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 0d0c859..5609bb8 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);
}
--
1.9.1
- [Qemu-devel] [PATCH 11/35] net: rocker: fix an incorrect array bounds check, (continued)
- [Qemu-devel] [PATCH 11/35] net: rocker: fix an incorrect array bounds check, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 17/35] net: set endianness on all backend devices, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 13/35] blockdev: Fix 'change' for slot devices, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 14/35] net/dump: fix nfds->filename leak, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 18/35] ehci: update irq on reset, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 19/35] block/raw-posix: avoid bogus fixup for cylinders on DASD disks, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 01/35] ehci: make idt processing more robust, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 21/35] s390x/css: fix control flags during csch, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 15/35] net/filter: fix nf->netdev_id leak, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 16/35] net: ne2000: check ring buffer control registers, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 24/35] qmp: Fix reference-counting of qnull on empty output visit,
Michael Roth <=
- [Qemu-devel] [PATCH 23/35] cpus: use broadcast on qemu_pause_cond, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 20/35] s390x/ioinst: set type and len for SEI response, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 27/35] e1000: eliminate infinite loops on out-of-bounds transfer start, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 26/35] block: qemu-iotests - add test for snapshot, commit, snapshot bug, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 25/35] block: set device_list.tqe_prev to NULL on BDS removal, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 29/35] hw/virtio: fix double use of a virtio flag, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 32/35] vhost-user: don't merge regions with different fds, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 30/35] hw/virtio: group virtio flags into an enum, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 31/35] fw_cfg: unbreak migration compatibility for 2.4 and earlier machines, Michael Roth, 2016/03/21
- [Qemu-devel] [PATCH 22/35] fw_cfg: avoid calculating invalid current entry pointer, Michael Roth, 2016/03/21