[libvirt] [PATCH] rpc: for messages with FDs always decode count of FDs from the message

[Ján Tomko]

On Tue, Sep 26, 2017 at 05:13:37PM +0200, Pavel Hrdina wrote:
>The packet with passed FD has the following format:
>
>    --------------------------
>    | len | header | payload |
>    --------------------------
>
>where "payload" has an additional count of FDs before the actual data:
>
>    ------------------
>    | nfds | payload |
>    ------------------
>
>When the packet is received we parse the "header", which as a side
>effect updates msg->bufferOffset to point to the beginning of "payload".
>If the message call contains FDs, we need to also parse the count of
>FDs, which also updates the msg->bufferOffset.
>
>The issue here is that when we attempt to read the FDs data from the
>socket and we receive EAGAIN we finish the reading and call poll()
>to wait for the data the we need.  When the data arrives we already have
>the packet in our buffer so we read the "header" again but this time
>we don't read the count of FDs because we already have it stored.
>
>That means that the msg->bufferOffset is not updated to point to the
>actual beginning of the payload data, but it points to the count of
>FDs.  After all FDs are processed we dispatch the message to process
>it and decode the payload.  Since the msg->bufferOffset points to wrong
>data, we decode the wrong payload and the API call fails with
>error messages:
>
>    Domain not found: no domain with matching uuid '67656e65-7269-6300-0c87-5003ca6941f2' ()
>

It would be nice to mention the commit that removed the repeated calling
of virNetMessageDecodeNumFDs because of the memleak.

>Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>---
> src/rpc/virnetclient.c       |  3 +--
> src/rpc/virnetmessage.c      | 12 +++++++-----
> src/rpc/virnetserverclient.c |  3 +--
> 3 files changed, 9 insertions(+), 9 deletions(-)
>

ACK regardless of my nosy question below

>diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
>index 5908b074a8..94c4c89e4f 100644
>--- a/src/rpc/virnetmessage.c
>+++ b/src/rpc/virnetmessage.c
>@@ -327,11 +327,13 @@ int virNetMessageDecodeNumFDs(virNetMessagePtr msg)
>         goto cleanup;
>     }
>
>-    msg->nfds = numFDs;
>-    if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)
>-        goto cleanup;
>-    for (i = 0; i < msg->nfds; i++)
>-        msg->fds[i] = -1;
>+    if (msg->nfds == 0) {
>+        msg->nfds = numFDs;
>+        if (VIR_ALLOC_N(msg->fds, msg->nfds) < 0)

Could this leak memory if someone sends us maliciously crafted RPC data
where nfds = 0 and we call the VIR_ALLOC_N multiple times?

Jan

>+            goto cleanup;
>+        for (i = 0; i < msg->nfds; i++)
>+            msg->fds[i] = -1;
>+    }
>
>     VIR_DEBUG("Got %zu FDs from peer", msg->nfds);
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170927/5057b4a1/attachment-0001.sig>