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 Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- io/channel-tls.c | 68 ++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/io/channel-tls.c b/io/channel-tls.c index 07274c12df..940fc3c6d1 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; @@ -252,14 +269,16 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc, func, opaque, destroy); trace_qio_channel_tls_handshake_start(ioc); - qio_channel_tls_handshake_task(ioc, task, context); + if (qio_channel_tls_handshake_task(ioc, task, context)) { + qio_task_free(task); + } } 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 +291,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 +316,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 +332,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.53.0