[vlc-devel] [PATCH] RFC: gnutls: protect gnutls_session_t with a mutex
Rémi Denis-Courmont
remi at remlab.net
Thu Feb 21 17:24:04 CET 2019
Le torstaina 21. helmikuuta 2019, 15.55.47 EET Thomas Guillem a écrit :
> 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
That commit is supposed to only affect applications that send or receive before
the handshake completes. I don't exactly see how that can occur in VLC.
vlc_tls_ClientSessionCreate() always waits for the handshake to complete
before returning the session, so there are no opportunities for HTTP/2 or any
other protocol layer to send or receive too early.
So if that commit does what it says it does, then it should not make any
difference.
> This gnutls commit is perfecly fine.
So you say. It looks racy to me. The 'internals' value are written by the
receiving thread on receiving handshake messages from the server, but read
from the receiving thread.
If your bisection is correct, then this patch actually looks completely wrong
to me. It leads to UB and breaks well-behaved applications to protect against
hypothetically broken apps.
> The problem come from the new https module that is calling vlc_tls_t API
> from different threads.
Both GnuTLS and VLC TLS documentation very explicitly allow one reading thread
and one writing thread concurrently. I cannot see any violation of those
rules.
GnuTLS: https://gnutls.org/manual/html_node/Thread-safety.html
VLC: <include/vlc_tls.h>
During the initial handshake, which involves both reading and writing, only a
single thread uses the session, see vlc_tls_ClientSessionCreate().
Rehandshakes are ignored as permitted on the client side:
https://www.gnutls.org/manual/html_node/Data-transfer-and-termination.html
> Nothing specify in the documentation if
> vlc_tls_operations should be thread-safe.
I find that statement rather offensive considering how much efforts I spent to
document the transport layer socket subsystem,.
> I saw that securetransport implementation was thread-safe so I assumed that
> the gnutls one should be too.
I am not aware of any thread-safety issue in the VLC GnuTLS client side
bindings.
> 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);
I can't even surmise what the lock is supposed to achieve here.
>
> 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);
> + }
AFAICT, this is pointless unless the caller violates the usage rules.
>
> 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;
Ditto.
> }
>
> @@ -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);
Ditto.
>
> 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;
> + }
I cannot think of a situation where the lock could be contended here, as this
callback follows the usual rule against reentrancy (unlike read and write).
And thus it looks like a useless no-op.
>
> /* 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;
> +}
> +
Looks likewise useless.
> 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,
> };
So the whole patch seems useless.
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list