[vlc-devel] [PATCH] RFC: gnutls: protect gnutls_session_t with a mutex
Hugo Beauzée-Luyssen
hugo at beauzee.fr
Thu Feb 21 15:34:23 CET 2019
On Thu, Feb 21, 2019, at 2:56 PM, Thomas Guillem wrote:
> 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);
Is this lock required? The counterpart write is in gnutls_SessionOpen, so I'd assume the read to be safe since it won't be concurrent
> + 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);
You probably should add a mutex_cleanup_push for this, and other IO related functions
> 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,
Nitpicking, but I think "Handshake/HandsakeLocked" would be better in the name than "Handshake/ServerHandshake"
> .destroy = gnutls_ServerDestroy,
> };
>
> --
> 2.20.1
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
--
Hugo Beauzée-Luyssen
hugo at beauzee.fr
More information about the vlc-devel
mailing list