[PATCH] chardev: close QIOChannel before unref'ing

[PATCH] chardev: close QIOChannel before unref'ing

[Daniel P. Berrangé]

The chardev socket backend will unref the QIOChannel object while
it is still potentially open. When using TLS there could be a
pending TLS handshake taking place. If the channel is left open
then when the TLS handshake callback runs, it can end up accessing
free'd memory in the tcp_chr_tls_handshake method.

Closing the QIOChannel will unregister any pending handshake
source.

Reported-by: jiangyegen <jiangyegen@huawei.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 73947da188..7105753815 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -378,6 +378,10 @@ static void tcp_chr_free_connection(Chardev *chr)
                                  char_socket_yank_iochannel,
                                  QIO_CHANNEL(s->sioc));
     }
+
+    if (s->ioc) {
+        qio_channel_close(s->ioc, NULL);
+    }
     object_unref(OBJECT(s->sioc));
     s->sioc = NULL;
     object_unref(OBJECT(s->ioc));
-- 
2.43.0

Re: [PATCH] chardev: close QIOChannel before unref'ing

[Marc-André Lureau]

Hi

On Wed, Jan 10, 2024 at 3:16 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The chardev socket backend will unref the QIOChannel object while
> it is still potentially open. When using TLS there could be a
> pending TLS handshake taking place. If the channel is left open
> then when the TLS handshake callback runs, it can end up accessing
> free'd memory in the tcp_chr_tls_handshake method.
>
> Closing the QIOChannel will unregister any pending handshake
> source.

ooh oh, one of those little things..
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

(weak refs could be a solution? but it's also sometime tricky, and we
don't have those..)

>
> Reported-by: jiangyegen <jiangyegen@huawei.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  chardev/char-socket.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 73947da188..7105753815 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -378,6 +378,10 @@ static void tcp_chr_free_connection(Chardev *chr)
>                                   char_socket_yank_iochannel,
>                                   QIO_CHANNEL(s->sioc));
>      }
> +
> +    if (s->ioc) {
> +        qio_channel_close(s->ioc, NULL);
> +    }
>      object_unref(OBJECT(s->sioc));
>      s->sioc = NULL;
>      object_unref(OBJECT(s->ioc));
> --
> 2.43.0
>