[glib-networking/wip/deadlock] gnutls: Reject new sync ops when handshake is in progress

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 27025e010a6cbf9368253b77c9d493f31dd9a53f
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 | 18 ++++++++++++
 tls/tests/connection.c             | 56 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 73 insertions(+), 1 deletion(-)
---
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index 3b2afa4..d7f2b5b 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))
@@ -2039,7 +2055,9 @@ handshake_thread (GTask        *task,
     }
 
   BEGIN_GNUTLS_IO (gnutls, G_IO_IN | G_IO_OUT, timeout, cancellable);
+GTLS_DEBUG(gnutls, "gnutls_handshake starting");
   ret = gnutls_handshake (priv->session);
+GTLS_DEBUG(gnutls, "gnutls_handshake finished=%d (%s)", ret, gnutls_strerror (ret));
   if (ret == GNUTLS_E_GOT_APPLICATION_DATA)
     {
       guint8 buf[1024];
diff --git a/tls/tests/connection.c b/tls/tests/connection.c
index d82d1ba..9a4a7e4 100644
--- a/tls/tests/connection.c
+++ b/tls/tests/connection.c
@@ -2195,6 +2195,59 @@ 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.
+   */
+  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 +2319,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 +2327,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 ();