[glib-networking/wip/deadlock] gnutls: Reject new sync ops when handshake is in progress
- From: Michael Catanzaro <mcatanzaro src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [glib-networking/wip/deadlock] gnutls: Reject new sync ops when handshake is in progress
- Date: Sat, 23 Feb 2019 02:07:37 +0000 (UTC)
commit 964da949ab1a3c6bdfe245402f6762bc4a979803
Author: Michael Catanzaro <mcatanzaro igalia com>
Date: Sun Dec 9 23:47:32 2018 -0600
gnutls: Reject new sync ops when handshake is in progress
When certificate verification was moved into the handshake, we
introduced a deadlock in WebKit when loading TLS error pages. The
problem is described here:
https://bugs.webkit.org/show_bug.cgi?id=107451#c3
We should be robust to such problematic operations, and return an error
instead.
Fixes #46
tls/gnutls/gtlsconnection-gnutls.c | 16 ++++++++++
tls/tests/connection.c | 61 +++++++++++++++++++++++++++++++++++++-
2 files changed, 76 insertions(+), 1 deletion(-)
---
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index 3b2afa4..12a1dec 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -797,6 +797,22 @@ claim_op (GTlsConnectionGnutls *gnutls,
}
}
+ if (priv->handshaking &&
+ timeout != 0 &&
+ g_main_context_is_owner (priv->handshake_context))
+ {
+ /* Cannot perform a blocking operation during a handshake on the
+ * same thread that triggered the handshake. The only way this can
+ * occur is if the application is doing something weird in its
+ * accept-certificate callback. Allowing a blocking op would stall
+ * the handshake (forever, if there's no timeout). Even a close
+ * op would deadlock here.
+ */
+ g_set_error_literal (error, G_IO_ERROR, G_IO_ERROR_FAILED, _("Cannot perform blocking operation during
TLS handshake"));
+ g_mutex_unlock (&priv->op_mutex);
+ return FALSE;
+ }
+
if ((op != G_TLS_CONNECTION_GNUTLS_OP_WRITE && priv->reading) ||
(op != G_TLS_CONNECTION_GNUTLS_OP_READ && priv->writing) ||
(op != G_TLS_CONNECTION_GNUTLS_OP_HANDSHAKE && priv->handshaking))
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index d82d1ba..21f7650 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -2195,6 +2195,64 @@ test_alpn_server_only (TestConnection *test,
test_alpn (test, NULL, server_protocols, NULL);
}
+static gboolean
+on_accept_certificate_with_sync_close (GTlsClientConnection *conn,
+ GTlsCertificate *cert,
+ GTlsCertificateFlags errors,
+ gpointer user_data)
+{
+ GError *error = NULL;
+
+ /* Attempting to perform a sync operation that would block the
+ * handshake should fail, not deadlock.
+ */
+ g_io_stream_close (G_IO_STREAM (conn), NULL, &error);
+ g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED);
+ g_error_free (error);
+
+ /* FIXME: When writing this test, I initially wanted to return FALSE
+ * here to reject the connection. However, this surfaces a bug that I
+ * have not fixed yet. The problem is the server is not seeing the end
+ * of its g_output_stream_write() when the client fails the handshake.
+ * No good. The server's implicit handshake failure should trigger a
+ * write failure as well, instead of stalling. This needs to be fixed.
+ *
+ * Fixing this would allow us to guarantee that this callback is
+ * actually executed by checking test->read_error at the bottom of
+ * test_sync_op_during_handshake(). Currently, this test would still
+ * pass even if this callback were to be improperly skipped.
+ */
+ return TRUE;
+}
+
+static void
+test_sync_op_during_handshake (TestConnection *test,
+ gconstpointer data)
+{
+ GIOStream *connection;
+ GError *error = NULL;
+
+ connection = start_async_server_and_connect_to_it (test, G_TLS_AUTHENTICATION_NONE);
+ test->client_connection = g_tls_client_connection_new (connection, test->identity, &error);
+ g_assert_no_error (error);
+ g_object_unref (connection);
+
+ /* For this test, we need validation to fail to ensure that the
+ * accept-certificate signal gets emitted.
+ */
+ g_tls_client_connection_set_validation_flags (G_TLS_CLIENT_CONNECTION (test->client_connection),
+ G_TLS_CERTIFICATE_VALIDATE_ALL);
+
+ g_signal_connect (test->client_connection, "accept-certificate",
+ G_CALLBACK (on_accept_certificate_with_sync_close), test);
+
+ read_test_data_async (test);
+ g_main_loop_run (test->loop);
+
+ g_assert_no_error (test->read_error);
+ g_assert_no_error (test->server_error);
+}
+
int
main (int argc,
char *argv[])
@@ -2266,7 +2324,6 @@ main (int argc,
setup_connection, test_garbage_database, teardown_connection);
g_test_add ("/tls/connection/readwrite-after-connection-destroyed", TestConnection, NULL,
setup_connection, test_readwrite_after_connection_destroyed, teardown_connection);
-
g_test_add ("/tls/connection/alpn/match", TestConnection, NULL,
setup_connection, test_alpn_match, teardown_connection);
g_test_add ("/tls/connection/alpn/no-match", TestConnection, NULL,
@@ -2275,6 +2332,8 @@ main (int argc,
setup_connection, test_alpn_client_only, teardown_connection);
g_test_add ("/tls/connection/alpn/server-only", TestConnection, NULL,
setup_connection, test_alpn_server_only, teardown_connection);
+ g_test_add ("/tls/connection/sync-op-during-handshake", TestConnection, NULL,
+ setup_connection, test_sync_op_during_handshake, teardown_connection);
ret = g_test_run ();
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]