QUIC Concurrency API: Various minor fixes

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/24971)

diff --git a/doc/man3/SSL_CTX_set_domain_flags.pod b/doc/man3/SSL_CTX_set_domain_flags.pod
index de8430a52315de8b126b307c875abfda8d53ea4c..1ca362b33e215a4f20e157c199ac1650e2ef6c49 100644 (file)
--- a/doc/man3/SSL_CTX_set_domain_flags.pod
+++ b/doc/man3/SSL_CTX_set_domain_flags.pod
@@ -32,7 +32,7 @@ domain flags on a B<SSL_CTX> using a QUIC B<SSL_METHOD>. These flags determine
 the concurrency model which is used for a QUIC domain. A detailed introduction
 to these concepts can be found in L<openssl-quic-concurrency(7)>.
 
-The flags comprise zero or more of the following flags:
+Applications may use either one the flags here:
 
 =over 4
 
@@ -50,7 +50,7 @@ B<SSL_DOMAIN_FLAG_SINGLE_THREAD>.
 
 =item B<SSL_DOMAIN_FLAG_THREAD_ASSISTED>
 
-Specifying this flag configures the Thread-Assisted Concurrency Modle (TACM).
+Specifying this flag configures the Thread-Assisted Concurrency Model (TACM).
 It implies B<SSL_DOMAIN_FLAG_MULTI_THREAD> and B<SSL_DOMAIN_FLAG_BLOCKING>.
 
 This concurrency model is not available if OpenSSL was built without thread

diff --git a/doc/man7/openssl-quic-concurrency.pod b/doc/man7/openssl-quic-concurrency.pod
index e590adaae7c13a26fab1cdb1f065f391e3ebf126..03591603321e161b73fcf1d3d1c22b19f67ee977 100644 (file)
--- a/doc/man7/openssl-quic-concurrency.pod
+++ b/doc/man7/openssl-quic-concurrency.pod
@@ -79,7 +79,7 @@ The merits of these models are as follows:
 
 The B<Single-Threaded Concurrency Model (SCM)> performs no locking or
 synchronisation. It is entirely up to the application to synchronise access to
-the QUIC domain and its subsidary SSL objects.
+the QUIC domain and its subsidiary SSL objects.
 
 This concurrency model is also useful for an application which wants to use the
 OpenSSL QUIC implementation as a pure state machine.
@@ -201,7 +201,7 @@ B<SSL_DOMAIN_FLAG_THREAD_ASSISTED> is also specified).
 
 =item B<SSL_DOMAIN_FLAG_THREAD_ASSISTED>
 
-Specifying this flag configures the Thread-Assisted Concurrency Modle (TACM).
+Specifying this flag configures the Thread-Assisted Concurrency Model (TACM).
 It implies B<SSL_DOMAIN_FLAG_MULTI_THREAD>.
 
 =item B<SSL_DOMAIN_FLAG_BLOCKING>
@@ -266,7 +266,7 @@ Otherwise, the Contentive Concurrency Model (CCM) is used.
 =back
 
 The default concurrency model may vary between releases of OpenSSL. An
-application may specify one of more of the domain flags above to ensure
+application may specify one or more of the domain flags above to ensure
 consistent usage of a specific concurrency model between releases.
 
 =head2 Configuration of Concurrency Models with Implicit QUIC Domains

diff --git a/include/internal/quic_reactor.h b/include/internal/quic_reactor.h
index 8d20a073431c632c72c108a7497ce9e338d74f4f..bc5e5bd5f643d9aebd2e2667f8ca04e20b9adab7 100644 (file)
--- a/include/internal/quic_reactor.h
+++ b/include/internal/quic_reactor.h
@@ -71,10 +71,10 @@
  * the reactor interface.
  */
 struct quic_tick_result_st {
+    OSSL_TIME   tick_deadline;
     char        net_read_desired;
     char        net_write_desired;
     char        notify_other_threads;
-    OSSL_TIME   tick_deadline;
 };
 
 static ossl_inline ossl_unused void
@@ -116,7 +116,7 @@ struct quic_reactor_st {
 
     /*
      * Count of the current number of blocking waiters. Like everything else,
-     * this is protected the caller's mutex (i.e., the engine mutex).
+     * this is protected by the caller's mutex (i.e., the engine mutex).
      */
     size_t cur_blocking_waiters;
 

diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
index b586fb32ae968f1ab421b9997195ae4eb9d2fd2f..04fb1c3ad9765574b927b699848657fb3ee3a018 100644 (file)
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -2119,7 +2119,7 @@ static int ch_rx(QUIC_CHANNEL *ch, int channel_only, int *notify_other_threads)
 
     ch_rx_check_forged_pkt_limit(ch);
 
-    if (handled_any)
+    if (handled_any && notify_other_threads != NULL)
         *notify_other_threads = 1;
 
     /*

diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index e795df68791bee4e66885bc004bf0f55d504298d..f0be772336ef6312a80fe895e73f1abda38b801f 100644 (file)
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -237,8 +237,9 @@ static int wrong_type(const SSL *s, uint32_t flags)
 }
 
 /*
- * Given a QCSO, QSSO or QLSO, initialises a QCTX, determining the contextually
- * applicable QUIC_LISTENER, QUIC_CONNECTION and QUIC_XSO pointers.
+ * Given a QDSO, QCSO, QSSO or QLSO, initialises a QCTX, determining the
+ * contextually applicable QUIC_LISTENER, QUIC_CONNECTION and QUIC_XSO
+ * pointers.
  *
  * After this returns 1, all fields of the passed QCTX are initialised.
  * Returns 0 on failure. This function is intended to be used to provide API
@@ -710,10 +711,9 @@ static void quic_free_listener(QCTX *ctx)
 #if defined(OPENSSL_THREADS)
         ossl_crypto_mutex_free(&ctx->ql->mutex);
 #endif
-    }
-
-    if (ctx->ql->domain != NULL)
+    } else {
         SSL_free(&ctx->ql->domain->obj.ssl);
+    }
 }
 
 /* SSL_free */
@@ -4290,7 +4290,6 @@ SSL *ossl_quic_new_listener_from(SSL *ssl, uint64_t flags)
     QCTX ctx;
     QUIC_LISTENER *ql = NULL;
     QUIC_PORT_ARGS port_args = {0};
-    int reffed = 0;
 
     if (!expect_quic_domain(ssl, &ctx))
         return NULL;
@@ -4298,7 +4297,6 @@ SSL *ossl_quic_new_listener_from(SSL *ssl, uint64_t flags)
     if (!SSL_up_ref(&ctx.qd->obj.ssl))
         return NULL;
 
-    reffed = 1;
     qctx_lock(&ctx);
 
     if ((ql = OPENSSL_zalloc(sizeof(*ql))) == NULL) {
@@ -4338,8 +4336,7 @@ err:
 
     OPENSSL_free(ql);
     qctx_unlock(&ctx);
-    if (reffed)
-        SSL_free(&ctx.qd->obj.ssl);
+    SSL_free(&ctx.qd->obj.ssl);
 
     return NULL;
 }
@@ -4554,7 +4551,7 @@ SSL *ossl_quic_new_domain(SSL_CTX *ctx, uint64_t flags)
 
     if ((qd = OPENSSL_zalloc(sizeof(*qd))) == NULL) {
         QUIC_RAISE_NON_NORMAL_ERROR(NULL, ERR_R_CRYPTO_LIB, NULL);
-        goto err;
+        return NULL;
     }
 
 #if defined(OPENSSL_THREADS)
@@ -4586,9 +4583,7 @@ SSL *ossl_quic_new_domain(SSL_CTX *ctx, uint64_t flags)
     return &qd->obj.ssl;
 
 err:
-    if (qd != NULL)
-        ossl_quic_engine_free(qd->engine);
-
+    ossl_quic_engine_free(qd->engine);
 #if defined(OPENSSL_THREADS)
     ossl_crypto_mutex_free(&qd->mutex);
 #endif

diff --git a/ssl/quic/quic_reactor.c b/ssl/quic/quic_reactor.c
index 142818923ec3a2a71c6fa281f0daeb9f59ab3711..ed867998e8afee2241b356faaad4613d6d230040 100644 (file)
--- a/ssl/quic/quic_reactor.c
+++ b/ssl/quic/quic_reactor.c
@@ -231,19 +231,19 @@ static int poll_two_fds(int rfd, int rfd_want_read,
     FD_ZERO(&wfd_set);
     FD_ZERO(&efd_set);
 
-    if (rfd != -1 && rfd_want_read)
+    if (rfd != INVALID_SOCKET && rfd_want_read)
         openssl_fdset(rfd, &rfd_set);
-    if (wfd != -1 && wfd_want_write)
+    if (wfd != INVALID_SOCKET && wfd_want_write)
         openssl_fdset(wfd, &wfd_set);
 
     /* Always check for error conditions. */
-    if (rfd != -1)
+    if (rfd != INVALID_SOCKET)
         openssl_fdset(rfd, &efd_set);
-    if (wfd != -1)
+    if (wfd != INVALID_SOCKET)
         openssl_fdset(wfd, &efd_set);
 
     /* Check for notifier FD readability. */
-    if (notify_rfd != -1) {
+    if (notify_rfd != INVALID_SOCKET) {
         openssl_fdset(notify_rfd, &rfd_set);
         openssl_fdset(notify_rfd, &efd_set);
     }
@@ -254,11 +254,17 @@ static int poll_two_fds(int rfd, int rfd_want_read,
     if (notify_rfd > maxfd)
         maxfd = notify_rfd;
 
-    if (!ossl_assert(rfd != -1 || wfd != -1
+    if (!ossl_assert(rfd != INVALID_SOCKET || wfd != INVALID_SOCKET
                      || !ossl_time_is_infinite(deadline)))
         /* Do not block forever; should not happen. */
         return 0;
 
+    /*
+     * The mutex dance (unlock/re-locak after poll/seclect) is
+     * potentially problematic. This may create a situation when
+     * two threads arrive to select/poll with the same file
+     * descriptors. We just need to be aware of this.
+     */
 # if defined(OPENSSL_THREADS)
     if (mutex != NULL)
         ossl_crypto_mutex_unlock(mutex);
@@ -460,7 +466,8 @@ int ossl_quic_reactor_block_until_pred(QUIC_REACTOR *rtor,
     OSSL_TIME tick_deadline;
 
     notifier_fd
-        = (rtor->have_notifier ? ossl_rio_notifier_as_fd(&rtor->notifier) : -1);
+        = (rtor->have_notifier ? ossl_rio_notifier_as_fd(&rtor->notifier)
+                               : INVALID_SOCKET);
 
     for (;;) {
         if ((flags & SKIP_FIRST_TICK) != 0)
@@ -529,7 +536,7 @@ int ossl_quic_reactor_block_until_pred(QUIC_REACTOR *rtor,
          *   Second, the thread which happened to be the one which decremented
          *   cur_blocking_waiters to 0 unsignals the notifier and is then
          *   responsible for broadcasting to a CV to indicate to the other
-         *   threads that the synchronised wakeup has been cmpleted. Other
+         *   threads that the synchronised wakeup has been completed. Other
          *   threads wait for this CV to be signalled.
          *
          */

diff --git a/ssl/rio/rio_notifier.c b/ssl/rio/rio_notifier.c
index ce6a0b220b109796d8984025c46722fb35353e55..84d64ddb74cd423bbb8775aceb5a3418e68f9287 100644 (file)
--- a/ssl/rio/rio_notifier.c
+++ b/ssl/rio/rio_notifier.c
@@ -32,7 +32,7 @@ static int create_socket(int domain, int socktype, int protocol)
      */
     fd = (int)WSASocketA(domain, socktype, protocol, NULL, 0,
                          WSA_FLAG_NO_HANDLE_INHERIT);
-    if (fd < 0)
+    if (fd == INVALID_SOCKET)
         return -1;
 
     /* Prevent interference with the socket from other processes on Windows. */
@@ -47,7 +47,7 @@ static int create_socket(int domain, int socktype, int protocol)
 # endif
 
     fd = BIO_socket(domain, socktype, protocol, 0);
-    if (fd < 0)
+    if (fd == INVALID_SOCKET)
         return -1;
 
     /*
@@ -80,7 +80,7 @@ int ossl_rio_notifier_init(RIO_NOTIFIER *nfy)
 
     /* Create a close-on-exec socket. */
     lfd = create_socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
-    if (lfd < 0)
+    if (lfd == INVALID_SOCKET)
         return 0;
 
     /* Bind the socket to a random loopback port. */

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c1a51e23448ef971b015563ca7a7573516c6832f..a3216a9e11bf97944e19a06ebfcd62ff24c54db7 100644 (file)
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -4216,7 +4216,7 @@ SSL_CTX *SSL_CTX_new_ex(OSSL_LIB_CTX *libctx, const char *propq,
 # ifndef OPENSSL_NO_QUIC
     ret->domain_flags = 0;
     if (IS_QUIC_METHOD(meth)) {
-# if defined(OPENSSL_THREADS)
+#  if defined(OPENSSL_THREADS)
         if (meth == OSSL_QUIC_client_thread_method())
             ret->domain_flags
                 = SSL_DOMAIN_FLAG_MULTI_THREAD
@@ -4226,11 +4226,11 @@ SSL_CTX *SSL_CTX_new_ex(OSSL_LIB_CTX *libctx, const char *propq,
             ret->domain_flags
                 = SSL_DOMAIN_FLAG_MULTI_THREAD
                 | SSL_DOMAIN_FLAG_LEGACY_BLOCKING;
-# else
+#  else
         ret->domain_flags
             = SSL_DOMAIN_FLAG_SINGLE_THREAD
             | SSL_DOMAIN_FLAG_LEGACY_BLOCKING;
-# endif
+#  endif
     }
 # endif