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

[Pavel Hrdina]

> 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

Thanks, I'll push it with mentioning the commit that broke it.

> >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?

No, because the virNetSocketRecvFD() is not called in that case and we will
finish successfully to parse that packet.  It will not be called multiple
times.

Pavel