From e98c7350bfaf0ae1f2b72d68d4c5721de24a478f Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Thu, 28 May 2020 15:16:45 +0200 Subject: [PATCH] Improve BIO_socket_wait(), BIO_wait(), BIO_connect_retry(), and their docs Add/extend range check for 'fd' argument of BIO_socket_wait() and bio_wait() Correct nap time calculations in bio_wait(), thus correcting also BIO_wait() Update a type cast from 'unsigned long' to 'unsigned int' Extend the comments and documentation of BIO_wait() Rename BIO_connect_retry() to BIO_do_connect_retry() Make its 'timeout' argument < 0 lead to BIO_do_connect() tried only once Add optional 'nap_milliseconds' parameter determining the polling granularity Correct and generalize the retry case checking Extend the comments and documentation of BIO_do_connect_retry() Reviewed-by: Bernd Edlinger Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/11986) --- crypto/bio/b_sock.c | 2 + crypto/bio/bio_lib.c | 116 ++++++++++++++++++-------------- crypto/http/http_client.c | 4 +- doc/man3/BIO_socket_wait.pod | 43 +++++++----- doc/man3/OSSL_CMP_CTX_new.pod | 2 +- doc/man3/OSSL_HTTP_transfer.pod | 2 +- include/openssl/bio.h | 4 +- util/libcrypto.num | 2 +- 8 files changed, 103 insertions(+), 72 deletions(-) diff --git a/crypto/bio/b_sock.c b/crypto/bio/b_sock.c index f5d2a627cb..79f7743b2f 100644 --- a/crypto/bio/b_sock.c +++ b/crypto/bio/b_sock.c @@ -387,6 +387,8 @@ int BIO_socket_wait(int fd, int for_read, time_t max_time) struct timeval tv; time_t now; + if (fd < 0 || fd >= FD_SETSIZE) + return -1; if (max_time == 0) return 1; diff --git a/crypto/bio/bio_lib.c b/crypto/bio/bio_lib.c index 1579f7c366..c3c798d4b4 100644 --- a/crypto/bio/bio_lib.c +++ b/crypto/bio/bio_lib.c @@ -786,41 +786,49 @@ void bio_cleanup(void) } /* Internal variant of the below BIO_wait() not calling BIOerr() */ -static int bio_wait(BIO *bio, time_t max_time, unsigned int milliseconds) +static int bio_wait(BIO *bio, time_t max_time, unsigned int nap_milliseconds) { #ifndef OPENSSL_NO_SOCK int fd; #endif + long sec_diff; - if (max_time == 0) + if (max_time == 0) /* no timeout */ return 1; #ifndef OPENSSL_NO_SOCK - if (BIO_get_fd(bio, &fd) > 0) + if (BIO_get_fd(bio, &fd) > 0 && fd < FD_SETSIZE) return BIO_socket_wait(fd, BIO_should_read(bio), max_time); #endif - if (milliseconds > 1000) { - long sec_diff = (long)(max_time - time(NULL)); /* might overflow */ + /* fall back to polling since no sockets are available */ - if (sec_diff <= 0) - return 0; /* timeout */ - if ((unsigned long)sec_diff < milliseconds / 1000) - milliseconds = (unsigned long)sec_diff * 1000; + sec_diff = (long)(max_time - time(NULL)); /* might overflow */ + if (sec_diff < 0) + return 0; /* clearly timeout */ + + /* now take a nap at most the given number of milliseconds */ + if (sec_diff == 0) { /* we are below the 1 seconds resolution of max_time */ + if (nap_milliseconds > 1000) + nap_milliseconds = 1000; + } else { /* for sec_diff > 0, take min(sec_diff * 1000, nap_milliseconds) */ + if ((unsigned long)sec_diff * 1000 < nap_milliseconds) + nap_milliseconds = (unsigned int)sec_diff * 1000; } - ossl_sleep(milliseconds); + ossl_sleep(nap_milliseconds); return 1; } -/* +/*- * Wait on (typically socket-based) BIO at most until max_time. - * Succeed immediately if max_time == 0. If sockets are not available succeed - * after waiting at most given milliseconds in order to avoid a tight busy loop. - * Call BIOerr(...) unless success. + * Succeed immediately if max_time == 0. + * If sockets are not available support polling: succeed after waiting at most + * the number of nap_milliseconds in order to avoid a tight busy loop. + * Call BIOerr(...) on timeout or error. * Returns -1 on error, 0 on timeout, and 1 on success. */ -int BIO_wait(BIO *bio, time_t max_time, unsigned int milliseconds) +int BIO_wait(BIO *bio, time_t max_time, unsigned int nap_milliseconds) { - int rv = bio_wait(bio, max_time, milliseconds); + int rv = bio_wait(bio, max_time, nap_milliseconds); if (rv <= 0) BIOerr(0, rv == 0 ? BIO_R_TRANSFER_TIMEOUT : BIO_R_TRANSFER_ERROR); @@ -828,13 +836,16 @@ int BIO_wait(BIO *bio, time_t max_time, unsigned int milliseconds) } /* - * Connect via given BIO using BIO_do_handshake() until success/timeout/error. - * Parameter timeout == 0 means infinite, < 0 leads to immediate timeout error. + * Connect via given BIO using BIO_do_connect() until success/timeout/error. + * Parameter timeout == 0 means no timeout, < 0 means exactly one try. + * For non-blocking and potentially even non-socket BIOs perform polling with + * the given density: between polls sleep nap_milliseconds using BIO_wait() + * in order to avoid a tight busy loop. * Returns -1 on error, 0 on timeout, and 1 on success. */ -int BIO_connect_retry(BIO *bio, int timeout) +int BIO_do_connect_retry(BIO *bio, int timeout, int nap_milliseconds) { - int blocking = timeout == 0; + int blocking = timeout <= 0; time_t max_time = timeout > 0 ? time(NULL) + timeout : 0; int rv; @@ -843,40 +854,47 @@ int BIO_connect_retry(BIO *bio, int timeout) return -1; } - if (timeout < 0) { - BIOerr(0, BIO_R_CONNECT_TIMEOUT); - return 0; - } - - if (!blocking) - BIO_set_nbio(bio, 1); - - retry: /* it does not help here to set SSL_MODE_AUTO_RETRY */ - rv = BIO_do_handshake(bio); /* This indirectly calls ERR_clear_error(); */ - - if (rv <= 0) { - if (get_last_sys_error() == ETIMEDOUT) { - /* - * if blocking, despite blocking BIO, BIO_do_handshake() timed out - * when non-blocking, BIO_do_handshake() timed out early - * with rv == -1 and get_last_sys_error() == 0 - */ - ERR_clear_error(); - (void)BIO_reset(bio); - /* - * unless using BIO_reset(), blocking next connect() may crash and - * non-blocking next BIO_do_handshake() will fail - */ - goto retry; - } else if (BIO_should_retry(bio)) { - /* will not actually wait if timeout == 0 (i.e., blocking BIO) */ - rv = bio_wait(bio, max_time, 100 /* milliseconds */); + if (nap_milliseconds < 0) + nap_milliseconds = 100; + BIO_set_nbio(bio, !blocking); + + retry: + rv = BIO_do_connect(bio); /* This may indirectly call ERR_clear_error(); */ + + if (rv <= 0) { /* could be timeout or retryable error or fatal error */ + int err = ERR_peek_last_error(); + int reason = ERR_GET_REASON(err); + int do_retry = BIO_should_retry(bio); /* may be 1 only if !blocking */ + + if (ERR_GET_LIB(err) == ERR_LIB_BIO) { + switch (reason) { + case ERR_R_SYS_LIB: + /* + * likely retryable system error occurred, which may be + * EAGAIN (resource temporarily unavailable) some 40 secs after + * calling getaddrinfo(): Temporary failure in name resolution + * or a premature ETIMEDOUT, some 30 seconds after connect() + */ + case BIO_R_CONNECT_ERROR: + case BIO_R_NBIO_CONNECT_ERROR: + /* some likely retryable connection error occurred */ + (void)BIO_reset(bio); /* often needed to avoid retry failure */ + do_retry = 1; + break; + default: + break; + } + } + if (timeout >= 0 && do_retry) { + ERR_clear_error(); /* using ERR_pop_to_mark() would be cleaner */ + /* will not actually wait if timeout == 0 (i.e., blocking BIO): */ + rv = bio_wait(bio, max_time, nap_milliseconds); if (rv > 0) goto retry; BIOerr(0, rv == 0 ? BIO_R_CONNECT_TIMEOUT : BIO_R_CONNECT_ERROR); } else { rv = -1; - if (ERR_peek_error() == 0) /* missing error queue entry */ + if (err == 0) /* missing error queue entry */ BIOerr(0, BIO_R_CONNECT_ERROR); /* workaround: general error */ } } diff --git a/crypto/http/http_client.c b/crypto/http/http_client.c index 64f877abed..a8dda0050a 100644 --- a/crypto/http/http_client.c +++ b/crypto/http/http_client.c @@ -814,7 +814,7 @@ static int update_timeout(int timeout, time_t start_time) * BIO *(*OSSL_HTTP_bio_cb_t) (BIO *bio, void *arg, int conn, int detail); * The callback may modify the HTTP BIO provided in the bio argument, * whereby it may make use of any custom defined argument 'arg'. - * During connection establishment, just after BIO_connect_retry(), + * During connection establishment, just after BIO_do_connect_retry(), * the callback function is invoked with the 'conn' argument being 1 * 'detail' indicating whether a HTTPS (i.e., TLS) connection is requested. * On disconnect 'conn' is 0 and 'detail' indicates that no error occurred. @@ -873,7 +873,7 @@ BIO *OSSL_HTTP_transfer(const char *server, const char *port, const char *path, /* remaining parameters are checked indirectly by the functions called */ (void)ERR_set_mark(); /* prepare removing any spurious libssl errors */ - if (rbio == NULL && BIO_connect_retry(cbio, timeout) <= 0) + if (rbio == NULL && BIO_do_connect_retry(cbio, timeout, -1) <= 0) goto end; /* now timeout is guaranteed to be >= 0 */ diff --git a/doc/man3/BIO_socket_wait.pod b/doc/man3/BIO_socket_wait.pod index d105dfe698..b00a878c9d 100644 --- a/doc/man3/BIO_socket_wait.pod +++ b/doc/man3/BIO_socket_wait.pod @@ -4,8 +4,8 @@ BIO_socket_wait, BIO_wait, -BIO_connect_retry -- BIO socket utility functions +BIO_do_connect_retry +- BIO connection utility functions =head1 SYNOPSIS @@ -14,8 +14,8 @@ BIO_connect_retry #ifndef OPENSSL_NO_SOCK int BIO_socket_wait(int fd, int for_read, time_t max_time); #endif - int BIO_wait(BIO *bio, time_t max_time, unsigned int milliseconds); - int BIO_connect_retry(BIO *bio, long timeout); + int BIO_wait(BIO *bio, time_t max_time, unsigned int nap_milliseconds); + int BIO_do_connect_retry(BIO *bio, int timeout, int nap_milliseconds); =head1 DESCRIPTION @@ -23,27 +23,38 @@ BIO_socket_wait() waits on the socket B for reading if B is not 0, else for writing, at most until B. It succeeds immediately if B == 0 (which means no timeout given). -BIO_wait() waits at most until B on the given B, -which is typically socket-based, -for reading if B is supposed to read, else for writing. +BIO_wait() waits at most until B on the given (typically socket-based) +B, for reading if B is supposed to read, else for writing. +It is used by BIO_do_connect_retry() and can be used together L. It succeeds immediately if B == 0 (which means no timeout given). -If sockets are not available it succeeds after waiting at most given -B in order to help avoiding a tight busy loop at the caller. - -BIO_connect_retry() connects via the given B, retrying BIO_do_connect() -until success or a timeout or error condition is reached. +If sockets are not available it supports polling by succeeding after sleeping +at most the given B in order to avoid a tight busy loop. +Via B the caller determines the polling granularity. + +BIO_do_connect_retry() connects via the given B. +It retries BIO_do_connect() as far as needed to reach a definite outcome, +i.e., connection succeeded, timeout has been reached, or an error occurred. +For non-blocking and potentially even non-socket BIOs it polls +every B and sleeps in between using BIO_wait(). +If B is < 0 then a default value of 100 ms is used. If the B parameter is > 0 this indicates the maximum number of seconds -to wait until the connection is established. A value of 0 enables waiting -indefinitely, while a value < 0 immediately leads to a timeout condition. +to wait until the connection is established or a definite error occurred. +A value of 0 enables waiting indefinitely (i.e, no timeout), +while a value < 0 means that BIO_do_connect() is tried only once. +The function may, directly or indirectly, invoke ERR_clear_error(). =head1 RETURN VALUES -BIO_socket_wait(), BIO_wait(), and BIO_connect_retry() +BIO_socket_wait(), BIO_wait(), and BIO_do_connect_retry() return -1 on error, 0 on timeout, and 1 on success. +=head1 SEE ALSO + +L, L + =head1 HISTORY -BIO_socket_wait(), BIO_wait(), and BIO_connect_retry() +BIO_socket_wait(), BIO_wait(), and BIO_do_connect_retry() were added in OpenSSL 3.0. =head1 COPYRIGHT diff --git a/doc/man3/OSSL_CMP_CTX_new.pod b/doc/man3/OSSL_CMP_CTX_new.pod index 97927fb45e..e8237b46e7 100644 --- a/doc/man3/OSSL_CMP_CTX_new.pod +++ b/doc/man3/OSSL_CMP_CTX_new.pod @@ -337,7 +337,7 @@ function, which has the prototype The callback may modify the BIO B provided by OSSL_CMP_MSG_http_perform(), whereby it may make use of a custom defined argument B stored in the OSSL_CMP_CTX by means of OSSL_CMP_CTX_set_http_cb_arg(). -During connection establishment, just after calling BIO_connect_retry(), +During connection establishment, just after calling BIO_do_connect_retry(), the function is invoked with the B argument being 1 and the B argument being 1 if HTTPS is requested, i.e., SSL/TLS should be enabled. On disconnect B is 0 and B is 1 in case no error occurred, else 0. diff --git a/doc/man3/OSSL_HTTP_transfer.pod b/doc/man3/OSSL_HTTP_transfer.pod index e0adb2a1d1..4459f541f3 100644 --- a/doc/man3/OSSL_HTTP_transfer.pod +++ b/doc/man3/OSSL_HTTP_transfer.pod @@ -154,7 +154,7 @@ B is a BIO connect/disconnect callback function with prototype The callback may modify the HTTP BIO provided in the B argument, whereby it may make use of a custom defined argument B, which may for instance refer to an I structure. -During connection establishment, just after calling BIO_connect_retry(), +During connection establishment, just after calling BIO_do_connect_retry(), the function is invoked with the B argument being 1 and the B argument being 1 if HTTPS is requested, i.e., SSL/TLS should be enabled. On disconnect B is 0 and B is 1 if no error occurred, else 0. diff --git a/include/openssl/bio.h b/include/openssl/bio.h index 19f9311c68..88e57e70a8 100644 --- a/include/openssl/bio.h +++ b/include/openssl/bio.h @@ -664,8 +664,8 @@ int BIO_sock_should_retry(int i); int BIO_sock_non_fatal_error(int error); int BIO_socket_wait(int fd, int for_read, time_t max_time); # endif -int BIO_wait(BIO *bio, time_t max_time, unsigned int milliseconds); -int BIO_connect_retry(BIO *bio, int timeout); +int BIO_wait(BIO *bio, time_t max_time, unsigned int nap_milliseconds); +int BIO_do_connect_retry(BIO *bio, int timeout, int nap_milliseconds); int BIO_fd_should_retry(int i); int BIO_fd_non_fatal_error(int error); diff --git a/util/libcrypto.num b/util/libcrypto.num index 317481388c..230126ff55 100644 --- a/util/libcrypto.num +++ b/util/libcrypto.num @@ -4926,7 +4926,7 @@ RAND_DRBG_set_callback_data ? 3_0_0 EXIST::FUNCTION: RAND_DRBG_get_callback_data ? 3_0_0 EXIST::FUNCTION: BIO_socket_wait ? 3_0_0 EXIST::FUNCTION:SOCK BIO_wait ? 3_0_0 EXIST::FUNCTION: -BIO_connect_retry ? 3_0_0 EXIST::FUNCTION: +BIO_do_connect_retry ? 3_0_0 EXIST::FUNCTION: ERR_load_HTTP_strings ? 3_0_0 EXIST::FUNCTION: OSSL_HTTP_get ? 3_0_0 EXIST::FUNCTION: OSSL_HTTP_get_asn1 ? 3_0_0 EXIST::FUNCTION: -- 2.34.1