[vlc-devel] [PATCH] RFC: gnutls: protect gnutls_session_t with a mutex
Thomas Guillem
thomas at gllm.fr
Thu Feb 21 17:36:54 CET 2019
On Thu, Feb 21, 2019, at 17:24, Rémi Denis-Courmont wrote:
> 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,.
No offense intended.
>
> > 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.
OK, I got lost in all tls/http2 callbacks and my assumption was false.
The new http module use the tls module correcly: init/open/handashake from a thread, then use it (read/write) from an other thread.
>
> --
> Rémi Denis-Courmont
> http://www.remlab.net/
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list