[PATCH 2/3] io: fix cleanup for TLS I/O source data on cancellation

Daniel P. Berrangé posted 3 patches 1 month ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>
[PATCH 2/3] io: fix cleanup for TLS I/O source data on cancellation
Posted by Daniel P. Berrangé 1 month ago
The TLS code will create a GSource for tracking completion of the
handshake process, passing a QIOChannelTLSData struct that contains
various data items. The data struct is freed by the callback when
it completes, which means when a source is cancelled, nothing is
free'ing the data struct or its contents.

Switch to provide a data free callback to the GSource, which ensures
the QIOChannelTLSData struct is always freed even when the main event
callback never fires.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3114
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 io/channel-tls.c | 64 ++++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 07274c12df..8b70e95a4b 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -153,13 +153,32 @@ struct QIOChannelTLSData {
 };
 typedef struct QIOChannelTLSData QIOChannelTLSData;
 
+static void qio_channel_tls_io_data_free(gpointer user_data)
+{
+    QIOChannelTLSData *data = user_data;
+    /*
+     * Usually 'task' will be NULL since the GSource
+     * callback will either complete the task or pass
+     * it on to a new GSource. We'll see a non-NULL
+     * task here only if the GSource was released before
+     * its callback triggers
+     */
+    if (data->task) {
+        qio_task_free(data->task);
+    }
+    if (data->context) {
+        g_main_context_unref(data->context);
+    }
+    g_free(data);
+}
+
 static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
                                              GIOCondition condition,
                                              gpointer user_data);
 
-static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
-                                           QIOTask *task,
-                                           GMainContext *context)
+static gboolean qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
+                                               QIOTask *task,
+                                               GMainContext *context)
 {
     Error *err = NULL;
     int status;
@@ -170,8 +189,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
         trace_qio_channel_tls_handshake_fail(ioc);
         qio_task_set_error(task, err);
         qio_task_complete(task);
-        qio_task_free(task);
-        return;
+        return TRUE;
     }
 
     if (status == QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
@@ -184,7 +202,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
             trace_qio_channel_tls_credentials_allow(ioc);
         }
         qio_task_complete(task);
-        qio_task_free(task);
+        return TRUE;
     } else {
         GIOCondition condition;
         QIOChannelTLSData *data = g_new0(typeof(*data), 1);
@@ -208,8 +226,9 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
                                        condition,
                                        qio_channel_tls_handshake_io,
                                        data,
-                                       NULL,
+                                       qio_channel_tls_io_data_free,
                                        context);
+        return FALSE;
     }
 }
 
@@ -225,11 +244,9 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
         qio_task_get_source(task));
 
     tioc->hs_ioc_tag = 0;
-    g_free(data);
-    qio_channel_tls_handshake_task(tioc, task, context);
-
-    if (context) {
-        g_main_context_unref(context);
+    if (!qio_channel_tls_handshake_task(tioc, task, context)) {
+        /* task is kept by new GSource so must not be released yet */
+        data->task = NULL;
     }
 
     return FALSE;
@@ -258,8 +275,8 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc,
 static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition,
                                        gpointer user_data);
 
-static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
-                                     GMainContext *context)
+static gboolean qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
+                                         GMainContext *context)
 {
     GIOCondition condition;
     QIOChannelTLSData *data;
@@ -272,14 +289,12 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
         trace_qio_channel_tls_bye_fail(ioc);
         qio_task_set_error(task, err);
         qio_task_complete(task);
-        qio_task_free(task);
-        return;
+        return TRUE;
     }
 
     if (status == QCRYPTO_TLS_BYE_COMPLETE) {
         qio_task_complete(task);
-        qio_task_free(task);
-        return;
+        return TRUE;
     }
 
     data = g_new0(typeof(*data), 1);
@@ -299,7 +314,10 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
     trace_qio_channel_tls_bye_pending(ioc, status);
     ioc->bye_ioc_tag = qio_channel_add_watch_full(ioc->master, condition,
                                                   qio_channel_tls_bye_io,
-                                                  data, NULL, context);
+                                                  data,
+                                                  qio_channel_tls_io_data_free,
+                                                  context);
+    return FALSE;
 }
 
 
@@ -312,11 +330,9 @@ static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition,
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(qio_task_get_source(task));
 
     tioc->bye_ioc_tag = 0;
-    g_free(data);
-    qio_channel_tls_bye_task(tioc, task, context);
-
-    if (context) {
-        g_main_context_unref(context);
+    if (!qio_channel_tls_bye_task(tioc, task, context)) {
+        /* task is kept by new GSource so must not be released yet */
+        data->task = NULL;
     }
 
     return FALSE;
-- 
2.52.0


Re: [PATCH 2/3] io: fix cleanup for TLS I/O source data on cancellation
Posted by Marc-André Lureau 1 month ago
On Wed, Jan 7, 2026 at 7:36 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The TLS code will create a GSource for tracking completion of the
> handshake process, passing a QIOChannelTLSData struct that contains
> various data items. The data struct is freed by the callback when
> it completes, which means when a source is cancelled, nothing is
> free'ing the data struct or its contents.
>
> Switch to provide a data free callback to the GSource, which ensures
> the QIOChannelTLSData struct is always freed even when the main event
> callback never fires.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3114
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  io/channel-tls.c | 64 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 40 insertions(+), 24 deletions(-)
>
> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 07274c12df..8b70e95a4b 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -153,13 +153,32 @@ struct QIOChannelTLSData {
>  };
>  typedef struct QIOChannelTLSData QIOChannelTLSData;
>
> +static void qio_channel_tls_io_data_free(gpointer user_data)
> +{
> +    QIOChannelTLSData *data = user_data;
> +    /*
> +     * Usually 'task' will be NULL since the GSource
> +     * callback will either complete the task or pass
> +     * it on to a new GSource. We'll see a non-NULL
> +     * task here only if the GSource was released before
> +     * its callback triggers
> +     */
> +    if (data->task) {
> +        qio_task_free(data->task);
> +    }
> +    if (data->context) {
> +        g_main_context_unref(data->context);
> +    }
> +    g_free(data);
> +}
> +
>  static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
>                                               GIOCondition condition,
>                                               gpointer user_data);
>
> -static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
> -                                           QIOTask *task,
> -                                           GMainContext *context)
> +static gboolean qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
> +                                               QIOTask *task,
> +                                               GMainContext *context)
>  {
>      Error *err = NULL;
>      int status;
> @@ -170,8 +189,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
>          trace_qio_channel_tls_handshake_fail(ioc);
>          qio_task_set_error(task, err);
>          qio_task_complete(task);
> -        qio_task_free(task);
> -        return;
> +        return TRUE;
>      }
>
>      if (status == QCRYPTO_TLS_HANDSHAKE_COMPLETE) {
> @@ -184,7 +202,7 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
>              trace_qio_channel_tls_credentials_allow(ioc);
>          }
>          qio_task_complete(task);
> -        qio_task_free(task);
> +        return TRUE;
>      } else {
>          GIOCondition condition;
>          QIOChannelTLSData *data = g_new0(typeof(*data), 1);
> @@ -208,8 +226,9 @@ static void qio_channel_tls_handshake_task(QIOChannelTLS *ioc,
>                                         condition,
>                                         qio_channel_tls_handshake_io,
>                                         data,
> -                                       NULL,
> +                                       qio_channel_tls_io_data_free,
>                                         context);
> +        return FALSE;
>      }
>  }
>
> @@ -225,11 +244,9 @@ static gboolean qio_channel_tls_handshake_io(QIOChannel *ioc,
>          qio_task_get_source(task));
>
>      tioc->hs_ioc_tag = 0;
> -    g_free(data);
> -    qio_channel_tls_handshake_task(tioc, task, context);
> -
> -    if (context) {
> -        g_main_context_unref(context);
> +    if (!qio_channel_tls_handshake_task(tioc, task, context)) {
> +        /* task is kept by new GSource so must not be released yet */
> +        data->task = NULL;
>      }
>
>      return FALSE;
> @@ -258,8 +275,8 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc,
>  static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition,
>                                         gpointer user_data);
>
> -static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
> -                                     GMainContext *context)
> +static gboolean qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
> +                                         GMainContext *context)
>  {
>      GIOCondition condition;
>      QIOChannelTLSData *data;
> @@ -272,14 +289,12 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
>          trace_qio_channel_tls_bye_fail(ioc);
>          qio_task_set_error(task, err);
>          qio_task_complete(task);
> -        qio_task_free(task);
> -        return;
> +        return TRUE;
>      }
>
>      if (status == QCRYPTO_TLS_BYE_COMPLETE) {
>          qio_task_complete(task);
> -        qio_task_free(task);
> -        return;
> +        return TRUE;
>      }
>
>      data = g_new0(typeof(*data), 1);
> @@ -299,7 +314,10 @@ static void qio_channel_tls_bye_task(QIOChannelTLS *ioc, QIOTask *task,
>      trace_qio_channel_tls_bye_pending(ioc, status);
>      ioc->bye_ioc_tag = qio_channel_add_watch_full(ioc->master, condition,
>                                                    qio_channel_tls_bye_io,
> -                                                  data, NULL, context);
> +                                                  data,
> +                                                  qio_channel_tls_io_data_free,
> +                                                  context);
> +    return FALSE;
>  }
>
>
> @@ -312,11 +330,9 @@ static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition,
>      QIOChannelTLS *tioc = QIO_CHANNEL_TLS(qio_task_get_source(task));
>
>      tioc->bye_ioc_tag = 0;
> -    g_free(data);
> -    qio_channel_tls_bye_task(tioc, task, context);
> -
> -    if (context) {
> -        g_main_context_unref(context);
> +    if (!qio_channel_tls_bye_task(tioc, task, context)) {
> +        /* task is kept by new GSource so must not be released yet */
> +        data->task = NULL;
>      }
>
>      return FALSE;
> --
> 2.52.0
>
>


-- 
Marc-André Lureau