[glib-networking/openssl-read-write-nonblocking: 2/3] openssl: reimplement read/write to be always non blocking

[Ignacio Casal Quinteiro <icq src gnome org>]

commit e696f31fb3a2686369cc08aaa7939d1599f5db33
Author: Ignacio Casal Quinteiro <icq gnome org>
Date:   Thu May 23 12:40:57 2019 +0200

    openssl: reimplement read/write to be always non blocking
    
    And we block ourselves until the socket is available. This is to
    be able to lock over the ssl object and avoid deadlocks when writing
    and reading at the same time.

 tls/openssl/gtlsbio.c                |  52 +++++++++++++++++
 tls/openssl/gtlsbio.h                |   4 ++
 tls/openssl/gtlsconnection-openssl.c | 108 ++++++++++++++++++++++++++++-------
 3 files changed, 142 insertions(+), 22 deletions(-)
---
diff --git a/tls/openssl/gtlsbio.c b/tls/openssl/gtlsbio.c
index 02ccf21..4dfd058 100644
--- a/tls/openssl/gtlsbio.c
+++ b/tls/openssl/gtlsbio.c
@@ -35,6 +35,8 @@ typedef struct {
   gboolean write_blocking;
   GError **read_error;
   GError **write_error;
+  GMainContext *context;
+  GMainLoop *loop;
 } GTlsBio;
 
 static void
@@ -43,6 +45,8 @@ free_gbio (gpointer user_data)
   GTlsBio *bio = (GTlsBio *)user_data;
 
   g_object_unref (bio->io_stream);
+  g_main_context_unref (bio->context);
+  g_main_loop_unref (bio->loop);
   g_free (bio);
 }
 
@@ -290,6 +294,8 @@ g_tls_bio_new (GIOStream *io_stream)
 
   gbio = g_new0 (GTlsBio, 1);
   gbio->io_stream = g_object_ref (io_stream);
+  gbio->context = g_main_context_new ();
+  gbio->loop = g_main_loop_new (gbio->context, FALSE);
 
 #if OPENSSL_VERSION_NUMBER < 0x10100000L || defined (LIBRESSL_VERSION_NUMBER)
   ret->ptr = gbio;
@@ -397,3 +403,49 @@ g_tls_bio_set_write_error (BIO     *bio,
 #endif
   gbio->write_error = error;
 }
+
+static gboolean
+on_source_ready (GObject *pollable_stream,
+                 gpointer user_data)
+{
+  GMainLoop *loop = user_data;
+
+  g_main_loop_quit (loop);
+
+  return G_SOURCE_REMOVE;
+}
+
+void
+g_tls_bio_wait_available (BIO          *bio,
+                          GIOCondition  condition,
+                          GCancellable *cancellable)
+{
+  GTlsBio *gbio;
+  GSource *source;
+
+  g_return_if_fail (bio != NULL);
+
+#if OPENSSL_VERSION_NUMBER < 0x10100000L || defined (LIBRESSL_VERSION_NUMBER)
+  gbio = (GTlsBio *)bio->ptr;
+#else
+  gbio = BIO_get_data (bio);
+#endif
+
+  g_main_context_push_thread_default (gbio->context);
+
+  if (condition & G_IO_IN)
+    source = g_pollable_input_stream_create_source (G_POLLABLE_INPUT_STREAM (g_io_stream_get_input_stream 
(gbio->io_stream)),
+                                                    cancellable);
+  else
+    source = g_pollable_output_stream_create_source (G_POLLABLE_OUTPUT_STREAM (g_io_stream_get_output_stream 
(gbio->io_stream)),
+                                                     cancellable);
+
+  g_source_set_callback (source, (GSourceFunc)on_source_ready, gbio->loop, NULL);
+  g_source_attach (source, gbio->context);
+
+  g_main_loop_run (gbio->loop);
+  g_main_context_pop_thread_default (gbio->context);
+
+  g_source_destroy (source);
+  g_source_unref (source);
+}
diff --git a/tls/openssl/gtlsbio.h b/tls/openssl/gtlsbio.h
index b6812fe..4809c17 100644
--- a/tls/openssl/gtlsbio.h
+++ b/tls/openssl/gtlsbio.h
@@ -51,6 +51,10 @@ void       g_tls_bio_set_write_blocking    (BIO          *bio,
 void       g_tls_bio_set_write_error       (BIO          *bio,
                                             GError      **error);
 
+void       g_tls_bio_wait_available        (BIO          *bio,
+                                            GIOCondition  condition,
+                                            GCancellable *cancellable);
+
 G_END_DECLS
 
 #endif /* __G_TLS_BIO_H__ */
diff --git a/tls/openssl/gtlsconnection-openssl.c b/tls/openssl/gtlsconnection-openssl.c
index 6eb803b..96dcd16 100644
--- a/tls/openssl/gtlsconnection-openssl.c
+++ b/tls/openssl/gtlsconnection-openssl.c
@@ -41,6 +41,7 @@
 typedef struct _GTlsConnectionOpensslPrivate
 {
   BIO *bio;
+  GMutex ssl_mutex;
 
   gboolean shutting_down;
 } GTlsConnectionOpensslPrivate;
@@ -52,10 +53,24 @@ G_DEFINE_ABSTRACT_TYPE_WITH_CODE (GTlsConnectionOpenssl, g_tls_connection_openss
                                   G_IMPLEMENT_INTERFACE (G_TYPE_INITABLE,
                                                          g_tls_connection_openssl_initable_iface_init))
 
+static void
+g_tls_connection_openssl_finalize (GObject *object)
+{
+  GTlsConnectionOpenssl *openssl = G_TLS_CONNECTION_OPENSSL (object);
+  GTlsConnectionOpensslPrivate *priv;
+
+  priv = g_tls_connection_openssl_get_instance_private (openssl);
+
+  g_mutex_clear (&priv->ssl_mutex);
+
+  G_OBJECT_CLASS (g_tls_connection_openssl_parent_class)->finalize (object);
+}
+
 static GTlsConnectionBaseStatus
 end_openssl_io (GTlsConnectionOpenssl  *openssl,
                 GIOCondition            direction,
                 int                     ret,
+                gboolean                blocking,
                 GError                **error,
                 const char             *err_prefix,
                 const char             *err_str)
@@ -75,17 +90,9 @@ end_openssl_io (GTlsConnectionOpenssl  *openssl,
 
   status = g_tls_connection_base_pop_io (tls, direction, ret > 0, &my_error);
 
-  /* NOTE: this is tricky! The tls bio will set to retry if the operation
-   * would block, and we would get an error code with WANT_READ or WANT_WRITE,
-   * though if in that case we try again we would end up in an infinite loop
-   * since we will not let the glib main loop to do its stuff and we would
-   * be getting a would block forever. Instead we need to also check the error
-   * we get from the socket operation to understand whether to try again. See
-   * that we propagate the WOULD_BLOCK error a bit more down.
-   */
   if ((err_code == SSL_ERROR_WANT_READ ||
        err_code == SSL_ERROR_WANT_WRITE) &&
-      status != G_TLS_CONNECTION_BASE_WOULD_BLOCK)
+      blocking)
     {
       if (my_error)
         g_error_free (my_error);
@@ -200,9 +207,9 @@ end_openssl_io (GTlsConnectionOpenssl  *openssl,
     g_tls_connection_base_push_io (G_TLS_CONNECTION_BASE (openssl),         \
                                    direction, timeout, cancellable);
 
-#define END_OPENSSL_IO(openssl, direction, ret, status, errmsg, err)        \
+#define END_OPENSSL_IO(openssl, direction, ret, timeout, status, errmsg, err) \
     ERR_error_string_n (SSL_get_error (ssl, ret), error_str, sizeof(error_str)); \
-    status = end_openssl_io (openssl, direction, ret, err, errmsg, error_str); \
+    status = end_openssl_io (openssl, direction, ret, timeout == -1, err, errmsg, error_str); \
   } while (status == G_TLS_CONNECTION_BASE_TRY_AGAIN);
 
 static GTlsConnectionBaseStatus
@@ -246,7 +253,7 @@ g_tls_connection_openssl_request_rehandshake (GTlsConnectionBase  *tls,
 
   BEGIN_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, timeout, cancellable);
   ret = SSL_renegotiate (ssl);
-  END_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, ret, status,
+  END_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, ret, timeout, status,
                   _("Error performing TLS handshake"), error);
 
   return status;
@@ -296,7 +303,7 @@ g_tls_connection_openssl_handshake_thread_handshake (GTlsConnectionBase  *tls,
 
   BEGIN_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, timeout, cancellable);
   ret = SSL_do_handshake (ssl);
-  END_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, ret, status,
+  END_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, ret, timeout, status,
                   _("Error performing TLS handshake"), error);
 
   if (ret > 0)
@@ -343,6 +350,8 @@ g_tls_connection_openssl_push_io (GTlsConnectionBase *tls,
       g_clear_error (error);
       g_tls_bio_set_write_error (priv->bio, error);
     }
+
+  g_mutex_lock (&priv->ssl_mutex);
 }
 
 static GTlsConnectionBaseStatus
@@ -356,6 +365,8 @@ g_tls_connection_openssl_pop_io (GTlsConnectionBase  *tls,
 
   priv = g_tls_connection_openssl_get_instance_private (openssl);
 
+  g_mutex_unlock (&priv->ssl_mutex);
+
   if (direction & G_IO_IN)
     g_tls_bio_set_read_cancellable (priv->bio, NULL);
 
@@ -376,16 +387,38 @@ g_tls_connection_openssl_read (GTlsConnectionBase    *tls,
                                GError               **error)
 {
   GTlsConnectionOpenssl *openssl = G_TLS_CONNECTION_OPENSSL (tls);
+  GTlsConnectionOpensslPrivate *priv;
   GTlsConnectionBaseStatus status;
   SSL *ssl;
   gssize ret;
 
+  priv = g_tls_connection_openssl_get_instance_private (openssl);
+
   ssl = g_tls_connection_openssl_get_ssl (openssl);
 
-  BEGIN_OPENSSL_IO (openssl, G_IO_IN, timeout, cancellable);
-  ret = SSL_read (ssl, buffer, count);
-  END_OPENSSL_IO (openssl, G_IO_IN, ret, status,
-                  _("Error reading data from TLS socket"), error);
+  /* FIXME: revert back to use BEGIN/END_OPENSSL_IO once we move all the ssl
+   * operations into a worker thread
+   */
+  while (TRUE)
+    {
+      char error_str[256];
+
+      /* We want to always be non blocking here to avoid deadlocks */
+      g_tls_connection_base_push_io (G_TLS_CONNECTION_BASE (openssl),
+                                     G_IO_IN, 0, cancellable);
+
+      ret = SSL_read (ssl, buffer, count);
+
+      ERR_error_string_n (SSL_get_error (ssl, ret), error_str, sizeof (error_str));
+      status = end_openssl_io (openssl, G_IO_IN, ret, timeout == -1, error,
+                               _("Error reading data from TLS socket"), error_str);
+
+      if (status != G_TLS_CONNECTION_BASE_TRY_AGAIN)
+        break;
+
+      /* Wait for the socket to be available again to avoid an infinite loop */
+      g_tls_bio_wait_available (priv->bio, G_IO_IN, cancellable);
+    }
 
   if (ret >= 0)
     *nread = ret;
@@ -402,16 +435,35 @@ g_tls_connection_openssl_write (GTlsConnectionBase    *tls,
                                 GError               **error)
 {
   GTlsConnectionOpenssl *openssl = G_TLS_CONNECTION_OPENSSL (tls);
+  GTlsConnectionOpensslPrivate *priv;
   GTlsConnectionBaseStatus status;
   SSL *ssl;
   gssize ret;
 
+  priv = g_tls_connection_openssl_get_instance_private (openssl);
+
   ssl = g_tls_connection_openssl_get_ssl (openssl);
 
-  BEGIN_OPENSSL_IO (openssl, G_IO_OUT, timeout, cancellable);
-  ret = SSL_write (ssl, buffer, count);
-  END_OPENSSL_IO (openssl, G_IO_OUT, ret, status,
-                  _("Error writing data to TLS socket"), error);
+  while (TRUE)
+    {
+      char error_str[256];
+
+      /* We want to always be non blocking here to avoid deadlocks */
+      g_tls_connection_base_push_io (G_TLS_CONNECTION_BASE (openssl),
+                                     G_IO_OUT, 0, cancellable);
+
+      ret = SSL_write (ssl, buffer, count);
+
+      ERR_error_string_n (SSL_get_error (ssl, ret), error_str, sizeof (error_str));
+      status = end_openssl_io (openssl, G_IO_OUT, ret, timeout == -1, error,
+                               _("Error writing data to TLS socket"), error_str);
+
+      if (status != G_TLS_CONNECTION_BASE_TRY_AGAIN)
+        break;
+
+      /* Wait for the socket to be available again to avoid an infinite loop */
+      g_tls_bio_wait_available (priv->bio, G_IO_OUT, cancellable);
+    }
 
   if (ret >= 0)
     *nwrote = ret;
@@ -437,7 +489,11 @@ g_tls_connection_openssl_close (GTlsConnectionBase  *tls,
 
   BEGIN_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, timeout, cancellable);
   ret = SSL_shutdown (ssl);
-  END_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, ret, status,
+  /* Note it is documented that getting 0 is correct when shutting down since
+   * it means it will close the write direction
+   */
+  ret = ret == 0 ? 1 : ret;
+  END_OPENSSL_IO (openssl, G_IO_IN | G_IO_OUT, ret, timeout, status,
                   _("Error performing TLS close"), error);
 
   return status;
@@ -446,8 +502,11 @@ g_tls_connection_openssl_close (GTlsConnectionBase  *tls,
 static void
 g_tls_connection_openssl_class_init (GTlsConnectionOpensslClass *klass)
 {
+  GObjectClass *object_class = G_OBJECT_CLASS (klass);
   GTlsConnectionBaseClass *base_class = G_TLS_CONNECTION_BASE_CLASS (klass);
 
+  object_class->finalize                 = g_tls_connection_openssl_finalize;
+
   base_class->request_rehandshake        = g_tls_connection_openssl_request_rehandshake;
   base_class->handshake_thread_handshake = g_tls_connection_openssl_handshake_thread_handshake;
   base_class->retrieve_peer_certificate  = g_tls_connection_openssl_retrieve_peer_certificate;
@@ -502,6 +561,11 @@ g_tls_connection_openssl_initable_iface_init (GInitableIface *iface)
 static void
 g_tls_connection_openssl_init (GTlsConnectionOpenssl *openssl)
 {
+  GTlsConnectionOpensslPrivate *priv;
+
+  priv = g_tls_connection_openssl_get_instance_private (openssl);
+
+  g_mutex_init (&priv->ssl_mutex);
 }
 
 SSL *