QUIC Thread Assisted mode: miscellaneous fixes

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20348)

diff --git a/crypto/thread/arch/thread_win.c b/crypto/thread/arch/thread_win.c
index f647c68c0a29c120c807c1943c3b219b2366c8bb..7f44d1f5cfd3ff02c04f5c098f72cc8526d5444c 100644 (file)
--- a/crypto/thread/arch/thread_win.c
+++ b/crypto/thread/arch/thread_win.c
@@ -160,7 +160,6 @@ static int determine_timeout(OSSL_TIME deadline, DWORD *w_timeout_p)
 
     ms = ossl_time2ms(delta);
 
-
     /*
      * Amount of time we want to wait is too long for the 32-bit argument to
      * the Win32 API, so just wait as long as possible.
@@ -288,7 +287,7 @@ void ossl_crypto_condvar_free(CRYPTO_CONDVAR **cv)
     *cv_p = NULL;
 }
 
-#endif
+# endif
 
 void ossl_crypto_mem_barrier(void)
 {

diff --git a/doc/designs/quic-design/quic-thread-assist.md b/doc/designs/quic-design/quic-thread-assist.md
index 81a6e28a3068131652b3171548f38aa8d25b6845..e26a00a7cb1c0d76ded305d2f3c301072438737d 100644 (file)
--- a/doc/designs/quic-design/quic-thread-assist.md
+++ b/doc/designs/quic-design/quic-thread-assist.md
@@ -1,7 +1,7 @@
 QUIC Thread Assisted Mode Synchronisation Requirements
 ======================================================
 
-In thread assisted mode, we spin up a background thread to ensure that periodic
+In thread assisted mode, we create a background thread to ensure that periodic
 QUIC processing is handled in a timely fashion regardless of whether an
 application is frequently calling (or blocked in) SSL API I/O functions.
 

diff --git a/include/internal/quic_channel.h b/include/internal/quic_channel.h
index bba7256d037b5d43f41b66d964142f108d62014b..d60e4010bdfe2a0bffa45fa6e7f4be64799025fc 100644 (file)
--- a/include/internal/quic_channel.h
+++ b/include/internal/quic_channel.h
@@ -81,7 +81,6 @@
  *
  *   Precondition: must not hold channel mutex (unchecked)
  *   Postcondition: channel mutex is not held (by calling thread)
- *
  */
 #  define QUIC_TAKES_LOCK
 

diff --git a/ssl/quic/quic_channel.c b/ssl/quic/quic_channel.c
index 9d8acd17fd1fc18cc1f3cf51dcf341728f4ed988..a873054f906e90b80f630b20977b68552081e3c6 100644 (file)
--- a/ssl/quic/quic_channel.c
+++ b/ssl/quic/quic_channel.c
@@ -28,6 +28,13 @@
 #define INIT_CRYPTO_BUF_LEN     8192
 #define INIT_APP_BUF_LEN        8192
 
+/*
+ * Interval before we force a PING to ensure NATs don't timeout. This is based
+ * on the lowest commonly seen value of 30 seconds as cited in  RFC 9000 s.
+ * 10.1.2.
+ */
+#define MAX_NAT_INTERVAL (ossl_ms2time(25000))
+
 static void ch_rx_pre(QUIC_CHANNEL *ch);
 static int ch_rx(QUIC_CHANNEL *ch);
 static int ch_tx(QUIC_CHANNEL *ch);
@@ -1221,7 +1228,7 @@ static void ch_tick(QUIC_TICK_RESULT *res, void *arg, uint32_t flags)
 {
     OSSL_TIME now, deadline;
     QUIC_CHANNEL *ch = arg;
-    int channel_only = ((flags & QUIC_REACTOR_TICK_FLAG_CHANNEL_ONLY) != 0);
+    int channel_only = (flags & QUIC_REACTOR_TICK_FLAG_CHANNEL_ONLY) != 0;
 
     /*
      * When we tick the QUIC connection, we do everything we need to do
@@ -2092,15 +2099,15 @@ static void ch_update_ping_deadline(QUIC_CHANNEL *ch)
 {
     if (ch->max_idle_timeout > 0) {
         /*
-         * Maximum amount of time without traffic before we send a PING to
-         * keep the connection open. Usually we use max_idle_timeout/2, but
-         * ensure the period never exceeds 25 seconds to ensure NAT devices
-         * don't have their state time out (RFC 9000 s. 10.1.2).
+         * Maximum amount of time without traffic before we send a PING to keep
+         * the connection open. Usually we use max_idle_timeout/2, but ensure
+         * the period never exceeds the assumed NAT interval to ensure NAT
+         * devices don't have their state time out (RFC 9000 s. 10.1.2).
          */
         OSSL_TIME max_span
             = ossl_time_divide(ossl_ms2time(ch->max_idle_timeout), 2);
 
-        max_span = ossl_time_min(max_span, ossl_ms2time(25000));
+        max_span = ossl_time_min(max_span, MAX_NAT_INTERVAL);
 
         ch->ping_deadline = ossl_time_add(get_time(ch), max_span);
     } else {

diff --git a/ssl/quic/quic_impl.c b/ssl/quic/quic_impl.c
index b293bf7bd93d629e4dfba8d4d4ad4c2e35e11953..e510918bf7184899793ba7e966507ece6e448b23 100644 (file)
--- a/ssl/quic/quic_impl.c
+++ b/ssl/quic/quic_impl.c
@@ -202,7 +202,7 @@ void ossl_quic_free(SSL *s)
     /* Note: SSL_free calls OPENSSL_free(qc) for us */
 
     SSL_free(qc->tls);
-    ossl_crypto_mutex_free(&qc->mutex);
+    ossl_crypto_mutex_free(&qc->mutex); /* freed while still locked */
 }
 
 /* SSL method init */
@@ -529,8 +529,10 @@ int ossl_quic_get_net_read_desired(QUIC_CONNECTION *qc)
 
     quic_lock(qc);
 
-    if (qc->ch == NULL)
+    if (qc->ch == NULL) {
+        quic_unlock(qc);
         return 0;
+    }
 
     ret = ossl_quic_reactor_net_read_desired(ossl_quic_channel_get_reactor(qc->ch));
     quic_unlock(qc);
@@ -545,8 +547,10 @@ int ossl_quic_get_net_write_desired(QUIC_CONNECTION *qc)
 
     quic_lock(qc);
 
-    if (qc->ch == NULL)
+    if (qc->ch == NULL) {
+        quic_unlock(qc);
         return 0;
+    }
 
     ret = ossl_quic_reactor_net_write_desired(ossl_quic_channel_get_reactor(qc->ch));
     quic_unlock(qc);

diff --git a/ssl/quic/quic_local.h b/ssl/quic/quic_local.h
index 9aad63b742eb1aac2ee9e1e1a9ebde88de630136..4d6d18ae37de7e4bacd12b02e3c378186690e051 100644 (file)
--- a/ssl/quic/quic_local.h
+++ b/ssl/quic/quic_local.h
@@ -66,10 +66,10 @@ struct quic_conn_st {
     /* Initial peer L4 address. */
     BIO_ADDR                        init_peer_addr;
 
-#ifndef OPENSSL_NO_QUIC_THREAD_ASSIST
+#  ifndef OPENSSL_NO_QUIC_THREAD_ASSIST
     /* Manages thread for QUIC thread assisted mode. */
     QUIC_THREAD_ASSIST              thread_assist;
-#endif
+#  endif
 
     /* If non-NULL, used instead of ossl_time_now(). Used for testing. */
     OSSL_TIME                       (*override_now_cb)(void *arg);

diff --git a/ssl/quic/quic_reactor.c b/ssl/quic/quic_reactor.c
index b6490f2459b7a66c82dc561a5accb07452e551d6..853d62f94706e73824bc284268b77e4a4525aef6 100644 (file)
--- a/ssl/quic/quic_reactor.c
+++ b/ssl/quic/quic_reactor.c
@@ -176,7 +176,7 @@ static int poll_two_fds(int rfd, int rfd_want_read,
         return 0;
 
     if (mutex != NULL)
-        CRYPTO_THREAD_unlock(mutex);
+        ossl_crypto_mutex_unlock(mutex);
 
     do {
         /*
@@ -200,8 +200,8 @@ static int poll_two_fds(int rfd, int rfd_want_read,
         pres = select(maxfd + 1, &rfd_set, &wfd_set, &efd_set, ptv);
     } while (pres == -1 && get_last_socket_error_is_eintr());
 
-    if (mutex != NULL && !CRYPTO_THREAD_write_lock(mutex))
-        return 0;
+    if (mutex != NULL)
+        ossl_crypto_mutex_lock(mutex);
 
     return pres < 0 ? 0 : 1;
 #else
@@ -233,7 +233,7 @@ static int poll_two_fds(int rfd, int rfd_want_read,
         return 0;
 
     if (mutex != NULL)
-        CRYPTO_THREAD_unlock(mutex);
+        ossl_crypto_mutex_unlock(mutex);
 
     do {
         if (ossl_time_is_infinite(deadline)) {
@@ -247,8 +247,8 @@ static int poll_two_fds(int rfd, int rfd_want_read,
         pres = poll(pfds, npfd, timeout_ms);
     } while (pres == -1 && get_last_socket_error_is_eintr());
 
-    if (mutex != NULL && !CRYPTO_THREAD_write_lock(mutex))
-        return 0;
+    if (mutex != NULL)
+        ossl_crypto_mutex_lock(mutex);
 
     return pres < 0 ? 0 : 1;
 #endif

diff --git a/ssl/quic/quic_tserver.c b/ssl/quic/quic_tserver.c
index a0660458d497848a44bb97391b1754e840867c0b..a94a6b9dbee25bd4c4c64e8d59128795c4c123eb 100644 (file)
--- a/ssl/quic/quic_tserver.c
+++ b/ssl/quic/quic_tserver.c
@@ -70,7 +70,7 @@ QUIC_TSERVER *ossl_quic_tserver_new(const QUIC_TSERVER_ARGS *args,
 
     srv->args = *args;
 
-    if ((srv->mutex = CRYPTO_THREAD_lock_new()) == NULL)
+    if ((srv->mutex = ossl_crypto_mutex_new()) == NULL)
         goto err;
 
     srv->ctx = SSL_CTX_new_ex(srv->args.libctx, srv->args.propq, TLS_method());
@@ -111,8 +111,10 @@ QUIC_TSERVER *ossl_quic_tserver_new(const QUIC_TSERVER_ARGS *args,
     return srv;
 
 err:
-    if (srv != NULL)
+    if (srv != NULL) {
         ossl_quic_channel_free(srv->ch);
+        ossl_crypto_mutex_free(&srv->mutex);
+    }
 
     OPENSSL_free(srv);
     return NULL;
@@ -128,7 +130,7 @@ void ossl_quic_tserver_free(QUIC_TSERVER *srv)
     BIO_free(srv->args.net_wbio);
     SSL_free(srv->tls);
     SSL_CTX_free(srv->ctx);
-    CRYPTO_THREAD_lock_free(srv->mutex);
+    ossl_crypto_mutex_free(&srv->mutex);
     OPENSSL_free(srv);
 }
 

diff --git a/test/quic_tserver_test.c b/test/quic_tserver_test.c
index 359b66c1b251b44077af0523314bd253fc114f27..a385381716b8d4b250198f772daa164177ab5415 100644 (file)
--- a/test/quic_tserver_test.c
+++ b/test/quic_tserver_test.c
@@ -374,29 +374,20 @@ err:
 
 static int test_tserver(int idx)
 {
-    int use_thread_assist, use_inject;
+    int thread_assisted, use_fake_time, use_inject;
 
-    use_thread_assist = idx % 2;
+    thread_assisted = idx % 2;
     idx /= 2;
 
     use_inject = idx % 2;
+    idx /= 2;
 
-    return test_tserver_actual(use_thread_assist, use_inject);
-}
-
-static int test_tserver_simple(void)
-{
-    return do_test(/*thread_assisted=*/0, /*fake_time=*/0, /*use_inject=*/0);
-}
+    use_fake_time = idx % 2;
 
-static int test_tserver_thread(void)
-{
-    return do_test(/*thread_assisted=*/1, /*fake_time=*/0, /*use_inject=*/0);
-}
+    if (use_fake_time && !thread_assisted)
+        return 1;
 
-static int test_tserver_thread_fake_time(void)
-{
-    return do_test(/*thread_assisted=*/1, /*fake_time=*/1, /*use_inject=*/0);
+    return do_test(thread_assisted, use_fake_time, use_inject);
 }
 
 OPT_TEST_DECLARE_USAGE("certfile privkeyfile\n")
@@ -415,8 +406,6 @@ int setup_tests(void)
     if ((fake_time_lock = CRYPTO_THREAD_lock_new()) == NULL)
         return 0;
 
-    ADD_TEST(test_tserver_simple);
-    ADD_TEST(test_tserver_thread);
-    ADD_TEST(test_tserver_thread_fake_time);
+    ADD_ALL_TESTS(test_tserver, 2 * 2 * 2);
     return 1;
 }