> 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@xxxxxxxxxx>
> >---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
