virNetMessageQueuePush() appends by walking to the tail of the queue. If it is handed a message which is already in that queue, and that message happens to be the tail, it links the message to itself. The cycle then makes virNetMessageQueueServe() return the same pointer on consecutive calls, and callers which free what they are served - such as virNetServerClientDispatchWrite() - free it twice, aborting the process with "free(): invalid pointer". Refuse such a push and warn instead, so that a caller bug shows up as a diagnosable log message rather than as heap corruption some time later. The check has to walk the queue rather than just test msg->next, because virNetMessageClear() memsets the whole message: a queued message which has been cleared appears unlinked while still being referenced. Also break the cycle in virNetMessageQueueServe() if one is somehow already present, rather than handing out the same message indefinitely, and clear msg->next in virNetMessageFree() so a stale reference held by a queue is detectable instead of dangling. The accompanying test reproduces the self-cycle deterministically: with the check removed it fails with "Message linked to itself". Signed-off-by: Ross Golder <ross@golder.org> --- src/rpc/virnetmessage.c | 43 +++++++++++++++++++ tests/virnetmessagetest.c | 87 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+) diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index e66df5c9e2..eda8430f70 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -102,15 +102,48 @@ void virNetMessageFree(virNetMessage *msg) msg->cb(msg, msg->opaque); virNetMessageClearPayload(msg); + + /* Make a stale reference from a queue detectable rather than dangling */ + msg->next = NULL; + g_free(msg); } +static bool +virNetMessageQueueContains(virNetMessage *queue, virNetMessage *msg) +{ + virNetMessage *tmp; + + for (tmp = queue; tmp; tmp = tmp->next) { + if (tmp == msg) + return true; + } + + return false; +} + + void virNetMessageQueuePush(virNetMessage **queue, virNetMessage *msg) { virNetMessage *tmp = *queue; VIR_DEBUG("queue=%p msg=%p", queue, msg); + /* A message which is already linked into a queue must never be pushed + * again. If it happens to be the tail of this very queue, the loop + * below would link it to itself, and virNetMessageQueueServe() would + * then hand out the same message repeatedly - which the callers go on + * to free more than once. + * + * Note that virNetMessageClear() memsets ->next, so a message can be + * queued and yet appear unlinked; the queue has to be walked. + */ + if (msg->next || virNetMessageQueueContains(*queue, msg)) { + VIR_WARN("Refusing to queue message %p which is already queued (queue=%p *queue=%p msg->next=%p)", + msg, queue, *queue, msg->next); + return; + } + if (tmp) { while (tmp->next) tmp = tmp->next; @@ -129,6 +162,16 @@ virNetMessage *virNetMessageQueueServe(virNetMessage **queue) if (tmp) { *queue = g_steal_pointer(&tmp->next); + + /* A message linked to itself means the queue was corrupted by a + * duplicate push; serving it would hand out the same pointer + * indefinitely. Break the cycle rather than looping on it. + */ + if (*queue == tmp) { + VIR_WARN("Detected self-referencing message %p on queue %p, breaking cycle", + tmp, queue); + *queue = NULL; + } } VIR_DEBUG("queue serve end queue=%p *queue=%p", queue, *queue); diff --git a/tests/virnetmessagetest.c b/tests/virnetmessagetest.c index e426bc7791..72ec4c0ce7 100644 --- a/tests/virnetmessagetest.c +++ b/tests/virnetmessagetest.c @@ -511,6 +511,90 @@ static int testMessagePayloadStreamEncode(const void *args G_GNUC_UNUSED) } +static size_t +testMessageQueueLength(virNetMessage *queue) +{ + virNetMessage *tmp; + size_t len = 0; + + /* Bounded so a corrupted (cyclic) queue cannot hang the test */ + for (tmp = queue; tmp && len < 100; tmp = tmp->next) + len++; + + return len; +} + + +static int testMessageQueueDuplicatePush(const void *args G_GNUC_UNUSED) +{ + virNetMessage *queue = NULL; + virNetMessage *msgA = virNetMessageNew(false); + virNetMessage *msgB = virNetMessageNew(false); + int ret = -1; + + if (!msgA || !msgB) + goto cleanup; + + /* Pushing the same message twice must not corrupt the queue. Without + * the check in virNetMessageQueuePush() this links msgA to itself, + * and serving the queue then returns it forever - the callers going + * on to free it more than once. + */ + virNetMessageQueuePush(&queue, msgA); + virNetMessageQueuePush(&queue, msgA); + + if (queue != msgA) { + VIR_TEST_DEBUG("Expected queue head %p, got %p", msgA, queue); + goto cleanup; + } + + if (msgA->next != NULL) { + VIR_TEST_DEBUG("Message linked to itself: msgA->next=%p", msgA->next); + goto cleanup; + } + + if (testMessageQueueLength(queue) != 1) { + VIR_TEST_DEBUG("Expected queue length 1, got %zu", + testMessageQueueLength(queue)); + goto cleanup; + } + + /* A distinct message must still append normally, and re-pushing an + * already queued non-tail message must also be refused. + */ + virNetMessageQueuePush(&queue, msgB); + virNetMessageQueuePush(&queue, msgA); + + if (testMessageQueueLength(queue) != 2) { + VIR_TEST_DEBUG("Expected queue length 2, got %zu", + testMessageQueueLength(queue)); + goto cleanup; + } + + /* Serving must hand out each message exactly once, then empty */ + if (virNetMessageQueueServe(&queue) != msgA) { + VIR_TEST_DEBUG("Expected msgA to be served first"); + goto cleanup; + } + + if (virNetMessageQueueServe(&queue) != msgB) { + VIR_TEST_DEBUG("Expected msgB to be served second"); + goto cleanup; + } + + if (queue != NULL || virNetMessageQueueServe(&queue) != NULL) { + VIR_TEST_DEBUG("Expected queue to be empty"); + goto cleanup; + } + + ret = 0; + cleanup: + virNetMessageFree(msgA); + virNetMessageFree(msgB); + return ret; +} + + static int mymain(void) { @@ -535,6 +619,9 @@ mymain(void) if (virTestRun("Message Payload Stream Encode", testMessagePayloadStreamEncode, NULL) < 0) ret = -1; + if (virTestRun("Message Queue Duplicate Push", testMessageQueueDuplicatePush, NULL) < 0) + ret = -1; + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.53.0