[glib-networking/mcatanzaro/#20: 5/5] Always fire TLS source when buffered data is available to read

[Michael Catanzaro <mcatanzaro src gnome org>]

commit 84501f6f8b3beca15aa92f616df69e13b0f003c9
Author: Michael Catanzaro <mcatanzaro gnome org>
Date:   Wed Jan 8 11:20:31 2020 -0600

    Always fire TLS source when buffered data is available to read
    
    Buffered data is readable data, so if applications that are polling to
    see whether data is readable, we need to check the GnuTLS buffer.
    
    We now need to sync the TLS source after each op, since any op could
    change the state of the GnuTLS buffer, and we need buffered content to
    be reported as I/O ready even if there's no I/O ready on the base stream.
    
    Finally, clean up wonkiness inside the check function. It doesn't seem
    safe to not lock the op_mutex here, and the conditions it's checking
    don't appear to be as comprehensive as what tls_source_sync() is doing.
    
    Fixes #20

 tls/base/gtlsconnection-base.c     | 77 +++++++++++++++++++++++++++++++-------
 tls/base/gtlsconnection-base.h     |  3 ++
 tls/gnutls/gtlsconnection-gnutls.c | 14 +++++++
 3 files changed, 81 insertions(+), 13 deletions(-)
---
diff --git a/tls/base/gtlsconnection-base.c b/tls/base/gtlsconnection-base.c
index 637d785..ea22c04 100644
--- a/tls/base/gtlsconnection-base.c
+++ b/tls/base/gtlsconnection-base.c
@@ -158,8 +158,13 @@ typedef struct
 
   gchar        **advertised_protocols;
   gchar         *negotiated_protocol;
+
+  GList         *sources;
+  GMutex         sources_mutex;
 } GTlsConnectionBasePrivate;
 
+typedef struct _GTlsConnectionBaseSource GTlsConnectionBaseSource;
+
 static void g_tls_connection_base_dtls_connection_iface_init (GDtlsConnectionInterface *iface);
 
 static void g_tls_connection_base_datagram_based_iface_init  (GDatagramBasedInterface  *iface);
@@ -183,6 +188,8 @@ static gboolean g_tls_connection_base_handshake (GTlsConnection   *conn,
                                                  GCancellable     *cancellable,
                                                  GError          **error);
 
+static void tls_source_sync (GTlsConnectionBaseSource *tls_source);
+
 G_DEFINE_ABSTRACT_TYPE_WITH_CODE (GTlsConnectionBase, g_tls_connection_base, G_TYPE_TLS_CONNECTION,
                                   G_ADD_PRIVATE (GTlsConnectionBase);
                                   G_IMPLEMENT_INTERFACE (G_TYPE_DATAGRAM_BASED,
@@ -232,6 +239,7 @@ g_tls_connection_base_init (GTlsConnectionBase *tls)
   g_cond_init (&priv->verify_certificate_condition);
 
   g_mutex_init (&priv->op_mutex);
+  g_mutex_init (&priv->sources_mutex);
 
   priv->waiting_for_op = g_cancellable_new ();
 }
@@ -279,6 +287,9 @@ g_tls_connection_base_finalize (GObject *object)
   g_clear_pointer (&priv->advertised_protocols, g_strfreev);
   g_clear_pointer (&priv->negotiated_protocol, g_free);
 
+  g_clear_pointer (&priv->sources, g_list_free);
+  g_mutex_clear (&priv->sources_mutex);
+
   G_OBJECT_CLASS (g_tls_connection_base_parent_class)->finalize (object);
 }
 
@@ -729,8 +740,14 @@ yield_op (GTlsConnectionBase       *tls,
   if (op != G_TLS_CONNECTION_BASE_OP_READ)
     priv->writing = FALSE;
 
+  g_cancellable_reset (priv->waiting_for_op);
   g_cancellable_cancel (priv->waiting_for_op);
+
   g_mutex_unlock (&priv->op_mutex);
+
+  g_mutex_lock (&priv->sources_mutex);
+  g_list_foreach (priv->sources, (GFunc)tls_source_sync, NULL);
+  g_mutex_unlock (&priv->sources_mutex);
 }
 
 static void
@@ -897,25 +914,40 @@ g_tls_connection_base_check (GTlsConnectionBase  *tls,
 {
   GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
 
-  /* Racy, but worst case is that we just get WOULD_BLOCK back */
+  g_mutex_lock (&priv->op_mutex);
+
+  /* Must wake up to finish implicit handshake. */
   if (priv->need_finish_handshake)
-    return TRUE;
+    {
+      g_mutex_unlock (&priv->op_mutex);
+      return TRUE;
+    }
 
-  /* If a handshake or close is in progress, then tls_istream and
-   * tls_ostream are blocked, regardless of the base stream status.
+  /* If op or close is in progress, then tls_istream and tls_ostream are
+   * blocked, regardless of the base stream status. Note this includes
+   * handshake ops.
    */
-  if (priv->handshaking)
-    return FALSE;
+  if (((condition & G_IO_IN) && (priv->reading || priv->read_closing)) ||
+      ((condition & G_IO_OUT) && (priv->writing || priv->write_closing)))
+    {
+      g_mutex_unlock (&priv->op_mutex);
+      return FALSE;
+    }
 
-  if (((condition & G_IO_IN) && priv->read_closing) ||
-      ((condition & G_IO_OUT) && priv->write_closing))
-    return FALSE;
+  g_mutex_unlock (&priv->op_mutex);
+
+  /* If base class says we are ready, then we are, regardless of the base
+   * stream status. This accounts for TLS-level buffers.
+   */
+  if (G_TLS_CONNECTION_BASE_GET_CLASS (tls)->check &&
+      G_TLS_CONNECTION_BASE_GET_CLASS (tls)->check (tls, condition))
+    return TRUE;
 
   /* Defer to the base stream or GDatagramBased. */
   return g_tls_connection_base_base_check (tls, condition);
 }
 
-typedef struct {
+struct _GTlsConnectionBaseSource {
   GSource             source;
 
   GTlsConnectionBase *tls;
@@ -928,9 +960,9 @@ typedef struct {
   GSource            *child_source;
   GIOCondition        condition;
 
-  gboolean            io_waiting;
   gboolean            op_waiting;
-} GTlsConnectionBaseSource;
+  gboolean            io_waiting;
+};
 
 /* Use a custom dummy callback instead of g_source_set_dummy_callback(), as that
  * uses a GClosure and is slow. (The GClosure is necessary to deal with any
@@ -967,6 +999,15 @@ tls_source_sync (GTlsConnectionBaseSource *tls_source)
     io_waiting = FALSE;
   g_mutex_unlock (&priv->op_mutex);
 
+  /* The child class might have TLS-level buffered data ready for I/O,
+   * even if the base stream does not. But no need to check for this if
+   * we're blocked on another op.
+   */
+  if (!op_waiting &&
+      G_TLS_CONNECTION_BASE_GET_CLASS (tls)->check &&
+      G_TLS_CONNECTION_BASE_GET_CLASS (tls)->check (tls, tls_source->condition))
+    io_waiting = FALSE;
+
   if (op_waiting == tls_source->op_waiting &&
       io_waiting == tls_source->io_waiting)
     return;
@@ -1011,7 +1052,7 @@ tls_source_dispatch (GSource     *source,
   else
     ret = (*pollable_func) (tls_source->base, user_data);
 
-  if (ret)
+  if (ret == G_SOURCE_CONTINUE)
     tls_source_sync (tls_source);
 
   return ret;
@@ -1021,6 +1062,12 @@ static void
 tls_source_finalize (GSource *source)
 {
   GTlsConnectionBaseSource *tls_source = (GTlsConnectionBaseSource *)source;
+  GTlsConnectionBase *tls = tls_source->tls;
+  GTlsConnectionBasePrivate *priv = g_tls_connection_base_get_instance_private (tls);
+
+  g_mutex_lock (&priv->sources_mutex);
+  priv->sources = g_list_remove (priv->sources, source);
+  g_mutex_unlock (&priv->sources_mutex);
 
   g_object_unref (tls_source->tls);
   g_source_unref (tls_source->child_source);
@@ -1142,6 +1189,10 @@ g_tls_connection_base_create_source (GTlsConnectionBase  *tls,
       g_source_unref (cancellable_source);
     }
 
+  g_mutex_lock (&priv->sources_mutex);
+  priv->sources = g_list_prepend (priv->sources, source);
+  g_mutex_unlock (&priv->sources_mutex);
+
   return source;
 }
 
diff --git a/tls/base/gtlsconnection-base.h b/tls/base/gtlsconnection-base.h
index 221bbe6..df44c30 100644
--- a/tls/base/gtlsconnection-base.h
+++ b/tls/base/gtlsconnection-base.h
@@ -78,6 +78,9 @@ struct _GTlsConnectionBaseClass
 
   gboolean                    (*is_session_resumed)         (GTlsConnectionBase   *tls);
 
+  gboolean                    (*check)                      (GTlsConnectionBase   *tls,
+                                                             GIOCondition          direction);
+
   void                        (*push_io)                    (GTlsConnectionBase   *tls,
                                                              GIOCondition          direction,
                                                              gint64                timeout,
diff --git a/tls/gnutls/gtlsconnection-gnutls.c b/tls/gnutls/gtlsconnection-gnutls.c
index 39cc7c5..d944425 100644
--- a/tls/gnutls/gtlsconnection-gnutls.c
+++ b/tls/gnutls/gtlsconnection-gnutls.c
@@ -925,6 +925,19 @@ g_tls_connection_gnutls_is_session_resumed (GTlsConnectionBase *tls)
   return gnutls_session_is_resumed (priv->session);
 }
 
+static gboolean
+g_tls_connection_gnutls_check (GTlsConnectionBase *tls,
+                               GIOCondition        direction)
+{
+  GTlsConnectionGnutls *gnutls = G_TLS_CONNECTION_GNUTLS (tls);
+  GTlsConnectionGnutlsPrivate *priv = g_tls_connection_gnutls_get_instance_private (gnutls);
+
+  if (direction & G_IO_IN)
+    return !!gnutls_record_check_pending (priv->session);
+
+  return FALSE;
+}
+
 static GTlsConnectionBaseStatus
 g_tls_connection_gnutls_read (GTlsConnectionBase  *tls,
                               void                *buffer,
@@ -1149,6 +1162,7 @@ g_tls_connection_gnutls_class_init (GTlsConnectionGnutlsClass *klass)
   base_class->retrieve_peer_certificate                  = g_tls_connection_gnutls_retrieve_peer_certificate;
   base_class->complete_handshake                         = g_tls_connection_gnutls_complete_handshake;
   base_class->is_session_resumed                         = g_tls_connection_gnutls_is_session_resumed;
+  base_class->check                                      = g_tls_connection_gnutls_check;
   base_class->read_fn                                    = g_tls_connection_gnutls_read;
   base_class->read_message_fn                            = g_tls_connection_gnutls_read_message;
   base_class->write_fn                                   = g_tls_connection_gnutls_write;