No issue I hit, the change is only from code observation when I am looking at a TLS premature termination issue.
qio_channel_tls_bye() API needs to be synchronous. When it's not, the previous impl will attach an asynchronous task retrying but only until when the channel gets the relevant GIO event. It may be problematic, because the caller of qio_channel_tls_bye() may have invoked channel close() before that, leading to premature termination of the TLS session. Remove the asynchronous handling, instead retry it immediately. Currently, the only two possible cases that may lead to async task is either INTERRUPT or EAGAIN. It should be suffice to spin retry as of now, until a solid proof showing that a more complicated retry logic is needed. With that, we can remove the whole async model for the bye task. When at it, making the function return bool, which looks like a common pattern in QEMU when errp is used. Side note on the tracepoints: previously the tracepoint bye_complete() isn't used. Start to use it in this patch. bye_pending() and bye_cancel() can be dropped now. Adding bye_retry() instead. Signed-off-by: Peter Xu <[email protected]> --- include/io/channel-tls.h | 5 ++- io/channel-tls.c | 86 +++++----------------------------------- io/trace-events | 3 +- 3 files changed, 15 insertions(+), 79 deletions(-) diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h index 7e9023570d..bcd14ffbd6 100644 --- a/include/io/channel-tls.h +++ b/include/io/channel-tls.h @@ -49,7 +49,6 @@ struct QIOChannelTLS { QCryptoTLSSession *session; QIOChannelShutdown shutdown; guint hs_ioc_tag; - guint bye_ioc_tag; }; /** @@ -60,8 +59,10 @@ struct QIOChannelTLS { * Perform the TLS session termination. This method will return * immediately and the termination will continue in the background, * provided the main loop is running. + * + * Returns: true on success, false on error (with errp set) */ -void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp); +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp); /** * qio_channel_tls_new_server: diff --git a/io/channel-tls.c b/io/channel-tls.c index 5a2c8188ce..8510a187a8 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -253,84 +253,25 @@ void qio_channel_tls_handshake(QIOChannelTLS *ioc, qio_channel_tls_handshake_task(ioc, task, context); } -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) +bool qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp) { - GIOCondition condition; - QIOChannelTLSData *data; int status; - Error *err = NULL; - status = qcrypto_tls_session_bye(ioc->session, &err); + trace_qio_channel_tls_bye_start(ioc); +retry: + status = qcrypto_tls_session_bye(ioc->session, errp); if (status < 0) { trace_qio_channel_tls_bye_fail(ioc); - qio_task_set_error(task, err); - qio_task_complete(task); - return; - } - - if (status == QCRYPTO_TLS_BYE_COMPLETE) { - qio_task_complete(task); - return; - } - - data = g_new0(typeof(*data), 1); - data->task = task; - data->context = context; - - if (context) { - g_main_context_ref(context); - } - - if (status == QCRYPTO_TLS_BYE_SENDING) { - condition = G_IO_OUT; - } else { - condition = G_IO_IN; - } - - 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); -} - - -static gboolean qio_channel_tls_bye_io(QIOChannel *ioc, GIOCondition condition, - gpointer user_data) -{ - QIOChannelTLSData *data = user_data; - QIOTask *task = data->task; - GMainContext *context = data->context; - 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); + return false; + } else if (status != QCRYPTO_TLS_BYE_COMPLETE) { + /* BYE event must be synchronous, retry immediately */ + trace_qio_channel_tls_bye_retry(ioc, status); + goto retry; } - return FALSE; -} - -static void propagate_error(QIOTask *task, gpointer opaque) -{ - qio_task_propagate_error(task, opaque); -} - -void qio_channel_tls_bye(QIOChannelTLS *ioc, Error **errp) -{ - QIOTask *task; - - task = qio_task_new(OBJECT(ioc), propagate_error, errp, NULL); - - trace_qio_channel_tls_bye_start(ioc); - qio_channel_tls_bye_task(ioc, task, NULL); + trace_qio_channel_tls_bye_complete(ioc); + return true; } static void qio_channel_tls_init(Object *obj G_GNUC_UNUSED) @@ -482,11 +423,6 @@ static int qio_channel_tls_close(QIOChannel *ioc, g_clear_handle_id(&tioc->hs_ioc_tag, g_source_remove); } - if (tioc->bye_ioc_tag) { - trace_qio_channel_tls_bye_cancel(ioc); - g_clear_handle_id(&tioc->bye_ioc_tag, g_source_remove); - } - return qio_channel_close(tioc->master, errp); } diff --git a/io/trace-events b/io/trace-events index dc3a63ba1f..67b3814192 100644 --- a/io/trace-events +++ b/io/trace-events @@ -45,10 +45,9 @@ qio_channel_tls_handshake_fail(void *ioc) "TLS handshake fail ioc=%p" qio_channel_tls_handshake_complete(void *ioc) "TLS handshake complete ioc=%p" qio_channel_tls_handshake_cancel(void *ioc) "TLS handshake cancel ioc=%p" qio_channel_tls_bye_start(void *ioc) "TLS termination start ioc=%p" -qio_channel_tls_bye_pending(void *ioc, int status) "TLS termination pending ioc=%p status=%d" +qio_channel_tls_bye_retry(void *ioc, int status) "TLS termination pending ioc=%p status=%d" qio_channel_tls_bye_fail(void *ioc) "TLS termination fail ioc=%p" qio_channel_tls_bye_complete(void *ioc) "TLS termination complete ioc=%p" -qio_channel_tls_bye_cancel(void *ioc) "TLS termination cancel ioc=%p" qio_channel_tls_credentials_allow(void *ioc) "TLS credentials allow ioc=%p" qio_channel_tls_credentials_deny(void *ioc) "TLS credentials deny ioc=%p" -- 2.50.1
