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

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(a)redhat.com&gt; >--- > 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?