[libvirt] [PATCH] rpc: fix race sending and encoding sasl data

The virNetSocketWriteSASL method has to encode the buffer it is given and then write it to the underlying socket. This write is not guaranteed to send the full amount of data that was encoded by SASL. We cache the SASL encoded data so that on the next invokation of virNetSocketWriteSASL we carry on sending it. The subtle problem is that the 'len' value passed into virNetSocketWriteSASL on the 2nd call may be larger than the original value. So when we've completed sending the SASL encoded data we previously cached, we must return the original length we encoded, not the new length. This flaw means we could potentially have been discarded queued data without sending it. This would have exhibited itself as a libvirt client never receiving the reply to a method it invokes, async events silently going missing, or worse stream data silently getting dropped. For this to be a problem libvirtd would have to be queued data to send to the client, while at the same time the TCP socket send buffer is full (due to a very slow client). This is quite unlikely so if this bug was ever triggered by a real world user it would be almost impossible to reproduce or diagnose, if indeed it was ever noticed at all. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; --- src/rpc/virnetsocket.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 23089afef4..2d41a716ba 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -107,6 +107,7 @@ struct _virNetSocket { const char *saslEncoded; size_t saslEncodedLength; + size_t saslEncodedRawLength; size_t saslEncodedOffset; #endif #if WITH_SSH2 @@ -1927,6 +1928,7 @@ static ssize_t virNetSocketWriteSASL(virNetSocketPtr sock, const char *buf, size &sock->saslEncodedLength) < 0) return -1; + sock->saslEncodedRawLength = tosend; sock->saslEncodedOffset = 0; } @@ -1943,11 +1945,20 @@ static ssize_t virNetSocketWriteSASL(virNetSocketPtr sock, const char *buf, size /* Sent all encoded, so update raw buffer to indicate completion */ if (sock->saslEncodedOffset == sock->saslEncodedLength) { + ssize_t done = sock->saslEncodedRawLength; sock->saslEncoded = NULL; - sock->saslEncodedOffset = sock->saslEncodedLength = 0; - - /* Mark as complete, so caller detects completion */ - return tosend; + sock->saslEncodedOffset = sock->saslEncodedLength = sock->saslEncodedRawLength = 0; + + /* Mark as complete, so caller detects completion. + * + * Note that 'done' is possibly less than our current + * 'tosend' value, since if virNetSocketWriteWire + * only partially sent the data, we might have been + * called a 2nd time to write remaining cached + * encoded data. This means that the caller might + * also have further raw data pending that's included + * in 'tosend' */ + return done; } else { /* Still have stuff pending in saslEncoded buffer. * Pretend to caller that we didn't send any yet. -- 2.14.3