[gnutls-devel] GnuTLS | WIP: fixed operation under multiple threads (!935)

[Development of GNU's TLS library]

Hugo Beauzée-Luyssen commented on a discussion on lib/record.c:

>  		 * data. We allow sending however, if we are in false start handshake
>  		 * state. */
>  		if (session->internals.recv_state != RECV_STATE_FALSE_START &&
> +		    session->internals.recv_state != RECV_STATE_FALSE_START_HANDLING &&
>  		    session->internals.recv_state != RECV_STATE_EARLY_START &&
> -		    !(session->internals.hsk_flags & HSK_EARLY_DATA_IN_FLIGHT))
> -			return gnutls_assert_val(GNUTLS_E_UNAVAILABLE_DURING_HANDSHAKE);
> +		    session->internals.recv_state != RECV_STATE_EARLY_START_HANDLING &&
> +		    !(session->internals.hsk_flags & HSK_EARLY_DATA_IN_FLIGHT)) {
> +			/* this additional check is to avoid a mutex for applications sending
> +			 * and receiving in parallel. */
> +			if (!session->internals.initial_negotiation_completed)

If you can get away with waiting for the negotiation to complete, maybe an atomic variable for the `initial_negotiation_completed` flag & a semaphore to delay a call to recv while send is completing the handshake, or the other way around?

If the values of `recv_state` & `initial_negotiation_completed` are dependent on each other, I'm not sure you can go away without a mutex though. If `recv_state` doesn't change after the initial call to `gnutls_handshake` then the checks above will work fine.

AFAICS `initial_negotiation_completed` will be updated from a subsequent call to recv/send, so you need to synchronize read/writes to it. If that value doesn't need to be updated based on recv_state, an atomic variable is enough, but it seems the sequence goes something like this:
```
recv_handshake_final()
{
   switch ( final_state )
   {
       //.....
       case STATE31:
            _gnutls_recv_finished() // initial_negotiation_completed = 1;
            final_state = STATE0;
   }
}
```
That kind of logic requires a lock IMO. Having `final_state` equal to `STATE31` at the begining of the switch doesn't mean it will still be equal to `STATE31` by the time `_gnutls_recv_finished` returns. The way I see it you need a lock to ensure that 2 racing threads would not perform the same steps simultaneously.

That being said, I do not know the GnuTLS sources enough (let alone TLS itself), and I might be completely missing the point, misunderstanding the call graph, [...], so please forgive me if some comments are completely out of touch with reality

-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/merge_requests/935#note_144817381
You're receiving this email because of your account on gitlab.com.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnupg.org/pipermail/gnutls-devel/attachments/20190225/4ac45ad9/attachment.html>