[vlc-devel] [PATCH] RFC: gnutls: protect gnutls_session_t with a mutex
Thomas Guillem
thomas at gllm.fr
Thu Feb 21 17:55:49 CET 2019
On Thu, Feb 21, 2019, at 17:37, Thomas Guillem wrote:
>
> 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.
initial_negotiation_completed is set from gnutls_handshake().
gnutls_handshake() is either called directly from VLC, or indirectly from gnutls_record_recv().
So we have our data-race here ^^
> >
> > > 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
> _______________________________________________
> 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