[libvirt] [PATCH] Fix up a bogus TLS message.

[Daniel P. Berrange]

On Fri, Oct 23, 2009 at 01:01:32PM +0200, Chris Lalancette wrote:
> When working with the TLS transport, I noticed that every single time
> a remote stream was closed, I would get a message like:
> 
> 09:09:40.793: error : remoteIOReadBuffer:7328 : failed to read from TLS socket A TLS packet with unexpected length was received.
> libvir: QEMU error : failed to read from TLS socket A TLS packet with unexpected length was received.
> 
> in the logs.  This happens because of a race in libvirtd; one thread
> handles the doRemoteClose(), which calls gnutls_bye() followed by close()
> while another thread is poll()'ing on the same fd.  Once the close()
> happens, the poll returns with revents set to POLLIN, and we would poll
> one more time for data from the now-closed fd.  Fix this situation by
> setting poll->session to NULL when we clean up, and check for that in
> remoteIOHandleInput().

I'm not sure that this is correct. If the connection is being closed
while another thread is still using it, that sounds like a bug that
should be fixed, because whatever just called doRemoteClose() has
also free'd 'priv', and so whatever other thread is in remoteIOReadBuffer()
is now accessing free'd memory. 

> 
> Signed-off-by: Chris Lalancette <clalance at redhat.com>
> ---
>  src/remote/remote_driver.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index bf001eb..335f44b 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -1389,6 +1389,7 @@ doRemoteClose (virConnectPtr conn, struct private_data *priv)
>      if (priv->uses_tls && priv->session) {
>          gnutls_bye (priv->session, GNUTLS_SHUT_RDWR);
>          gnutls_deinit (priv->session);
> +        priv->session = NULL;
>      }
>  #if HAVE_SASL
>      if (priv->saslconn)
> @@ -7223,6 +7224,12 @@ remoteIOReadBuffer(virConnectPtr conn,
>      int ret;
>  
>      if (priv->uses_tls) {
> +        if (!priv->session) {
> +            /* we may have reached here one more time after gnutls_bye()
> +             * was called, so just return here
> +             */
> +            return 0;
> +        }

If gnutls_bye() was just called, then 'priv' has also just been 
freed. So this must surely be accessing freed memory.

>      tls_resend:
>          ret = gnutls_record_recv (priv->session, bytes, len);
>          if (ret == GNUTLS_E_INTERRUPTED)
> -- 
> 1.6.0.6

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|