On 12/18/2017 12:46 PM, Daniel P. Berrange wrote:
> 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.
*invocation (is the usual way I see it spelled)
>
> 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@xxxxxxxxxx>
> ---
> src/rpc/virnetsocket.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
(minor note below - not really related, but could be addressed too).
> 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
>
In the just because I saw it while reviewing... There's a comment a few
lines earlier "0 = egain" - can we take the opportunity to make that
EAGAIN (could also do it as a separate trivial patch, IDC).
> /* 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.
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
