[vlc-devel] [PATCH] RFC: gnutls: protect gnutls_session_t with a mutex

[Thomas Guillem]

Since recent versions of gnutls, the new https module was failing one time over
10 (or 5 or 20, or 100, depends on machines). After an extensive git bisect on
gnutls, I found out that this was happening since
https://gitlab.com/gnutls/gnutls/commit/6a62ddfc416a4ec2118704f93c97fdd448d66566

This gnutls commit is perfecly fine.

The problem come from the new https module that is calling vlc_tls_t API from
different threads. Nothing specify in the documentation if vlc_tls_operations
should be thread-safe. I saw that securetransport implementation was
thread-safe so I assumed that the gnutls one should be too.

This fix can be done in different places: the https module (if we assume that
vlc_tls_t API is not thread-safe), a layer on top of tls modules
(src/network/tls.c) or in gnutls.
---
 modules/misc/gnutls.c | 46 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/modules/misc/gnutls.c b/modules/misc/gnutls.c
index def0727a17..d34c6efb0b 100644
--- a/modules/misc/gnutls.c
+++ b/modules/misc/gnutls.c
@@ -47,6 +47,7 @@ typedef struct vlc_tls_gnutls
     vlc_tls_t tls;
     gnutls_session_t session;
     vlc_object_t *obj;
+    vlc_mutex_t lock;
 } vlc_tls_gnutls_t;
 
 static void gnutls_Banner(vlc_object_t *obj)
@@ -130,7 +131,10 @@ static ssize_t vlc_gnutls_writev(gnutls_transport_ptr_t ptr,
 static int gnutls_GetFD(vlc_tls_t *tls, short *restrict events)
 {
     vlc_tls_gnutls_t *priv = (vlc_tls_gnutls_t *)tls;
+
+    vlc_mutex_lock(&priv->lock);
     vlc_tls_t *sock = gnutls_transport_get_ptr(priv->session);
+    vlc_mutex_unlock(&priv->lock);
 
     return vlc_tls_GetPollFD(sock, events);
 }
@@ -141,11 +145,15 @@ static ssize_t gnutls_Recv(vlc_tls_t *tls, struct iovec *iov, unsigned count)
     gnutls_session_t session = priv->session;
     size_t rcvd = 0;
 
+    vlc_mutex_lock(&priv->lock);
     while (count > 0)
     {
         ssize_t val = gnutls_record_recv(session, iov->iov_base, iov->iov_len);
         if (val < 0)
+        {
+            vlc_mutex_unlock(&priv->lock);
             return rcvd ? (ssize_t)rcvd : gnutls_Error(priv, val);
+        }
 
         rcvd += val;
 
@@ -155,6 +163,7 @@ static ssize_t gnutls_Recv(vlc_tls_t *tls, struct iovec *iov, unsigned count)
         iov++;
         count--;
     }
+    vlc_mutex_unlock(&priv->lock);
 
     return rcvd;
 }
@@ -166,6 +175,7 @@ static ssize_t gnutls_Send (vlc_tls_t *tls, const struct iovec *iov,
     gnutls_session_t session = priv->session;
     ssize_t val;
 
+    vlc_mutex_lock(&priv->lock);
     if (!gnutls_record_check_corked(session))
     {
         gnutls_record_cork(session);
@@ -182,6 +192,7 @@ static ssize_t gnutls_Send (vlc_tls_t *tls, const struct iovec *iov,
     }
 
     val = gnutls_record_uncork(session, 0);
+    vlc_mutex_unlock(&priv->lock);
     return (val < 0) ? gnutls_Error(priv, val) : val;
 }
 
@@ -192,13 +203,21 @@ static int gnutls_Shutdown(vlc_tls_t *tls, bool duplex)
     ssize_t val;
 
     /* Flush any pending data */
+    vlc_mutex_lock(&priv->lock);
     val = gnutls_record_uncork(session, 0);
     if (val < 0)
+    {
+        vlc_mutex_unlock(&priv->lock);
         return gnutls_Error(priv, val);
+    }
 
     val = gnutls_bye(session, duplex ? GNUTLS_SHUT_RDWR : GNUTLS_SHUT_WR);
     if (val < 0)
+    {
+        vlc_mutex_unlock(&priv->lock);
         return gnutls_Error(priv, val);
+    }
+    vlc_mutex_unlock(&priv->lock);
 
     return 0;
 }
@@ -208,6 +227,7 @@ static void gnutls_Close (vlc_tls_t *tls)
     vlc_tls_gnutls_t *priv = (vlc_tls_gnutls_t *)tls;
 
     gnutls_deinit(priv->session);
+    vlc_mutex_destroy(&priv->lock);
     free(priv);
 }
 
@@ -298,6 +318,7 @@ static vlc_tls_gnutls_t *gnutls_SessionOpen(vlc_object_t *obj, int type,
 
     priv->session = session;
     priv->obj = obj;
+    vlc_mutex_init(&priv->lock);
 
     vlc_tls_t *tls = &priv->tls;
 
@@ -408,9 +429,13 @@ static int gnutls_ClientHandshake(vlc_tls_t *tls,
     vlc_tls_gnutls_t *priv = (vlc_tls_gnutls_t *)tls;
     vlc_object_t *obj = priv->obj;
 
+    vlc_mutex_lock(&priv->lock);
     int val = gnutls_Handshake(tls, alp);
     if (val)
+    {
+        vlc_mutex_unlock(&priv->lock);
         return val;
+    }
 
     /* certificates chain verification */
     gnutls_session_t session = priv->session;
@@ -421,11 +446,15 @@ static int gnutls_ClientHandshake(vlc_tls_t *tls,
     {
         msg_Err(obj, "Certificate verification error: %s",
                 gnutls_strerror(val));
+        vlc_mutex_unlock(&priv->lock);
         goto error;
     }
 
     if (status == 0) /* Good certificate */
+    {
+        vlc_mutex_unlock(&priv->lock);
         return 0;
+    }
 
     /* Bad certificate */
     gnutls_datum_t desc;
@@ -442,13 +471,17 @@ static int gnutls_ClientHandshake(vlc_tls_t *tls,
     status &= ~GNUTLS_CERT_UNEXPECTED_OWNER; /* mismatched hostname */
 
     if (status != 0 || host == NULL)
+    {
+        vlc_mutex_unlock(&priv->lock);
         goto error; /* Really bad certificate */
+    }
 
     /* Look up mismatching certificate in store */
     const gnutls_datum_t *datum;
     unsigned count;
 
     datum = gnutls_certificate_get_peers (session, &count);
+    vlc_mutex_unlock(&priv->lock);
     if (datum == NULL || count == 0)
     {
         msg_Err(obj, "Peer certificate not available");
@@ -620,6 +653,17 @@ static vlc_tls_t *gnutls_ServerSessionOpen(vlc_tls_server_t *crd,
     return (priv != NULL) ? &priv->tls : NULL;
 }
 
+static int gnutls_ServerHandshake(vlc_tls_t *tls,
+                                  char **restrict alp)
+{
+    vlc_tls_gnutls_t *priv = (vlc_tls_gnutls_t *)tls;
+
+    vlc_mutex_lock(&priv->lock);
+    int val = gnutls_Handshake(tls, alp);
+    vlc_mutex_unlock(&priv->lock);
+    return val;
+}
+
 static void gnutls_ServerDestroy(vlc_tls_server_t *crd)
 {
     vlc_tls_creds_sys_t *sys = crd->sys;
@@ -633,7 +677,7 @@ static void gnutls_ServerDestroy(vlc_tls_server_t *crd)
 static const struct vlc_tls_server_operations gnutls_ServerOps =
 {
     .open = gnutls_ServerSessionOpen,
-    .handshake = gnutls_Handshake,
+    .handshake = gnutls_ServerHandshake,
     .destroy = gnutls_ServerDestroy,
 };
 
-- 
2.20.1