From: Alexandr Nedvedicky Date: Fri, 19 Jul 2024 21:48:30 +0000 (+0200) Subject: QUIC Concurrency API: Various minor fixes X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2f35c1e75b9c2d3eb793ad6e2b3dfbf2f83601eb;p=thirdparty%2Fopenssl.git QUIC Concurrency API: Various minor fixes Reviewed-by: Matt Caswell Reviewed-by: Neil Horman (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 de8430a5231..1ca362b33e2 100644 --- 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 using a QUIC B. These flags determine the concurrency model which is used for a QUIC domain. A detailed introduction to these concepts can be found in L. -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. =item B -Specifying this flag configures the Thread-Assisted Concurrency Modle (TACM). +Specifying this flag configures the Thread-Assisted Concurrency Model (TACM). It implies B and B. 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 e590adaae7c..03591603321 100644 --- 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 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 is also specified). =item B -Specifying this flag configures the Thread-Assisted Concurrency Modle (TACM). +Specifying this flag configures the Thread-Assisted Concurrency Model (TACM). It implies B. =item B @@ -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 8d20a073431..bc5e5bd5f64 100644 --- 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 a553ef767e5..0c22b86ee7b 100644 --- a/ssl/quic/quic_channel.c +++ b/ssl/quic/quic_channel.c @@ -2099,7 +2099,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 e795df68791..f0be772336e 100644 --- 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 142818923ec..ed867998e8a 100644 --- 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 ce6a0b220b1..84d64ddb74c 100644 --- 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 d364cc27223..602f151c51b 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -4193,7 +4193,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 @@ -4203,11 +4203,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