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