diff --git a/crypto/bio/bio_socket_test.cc b/crypto/bio/bio_socket_test.cc index fbdfc7b1f4..e502e0f0b8 100644 --- a/crypto/bio/bio_socket_test.cc +++ b/crypto/bio/bio_socket_test.cc @@ -43,6 +43,7 @@ using Socket = int; #define INVALID_SOCKET (-1) static int closesocket(const int sock) { return close(sock); } static std::string LastSocketError() { return strerror(errno); } +static void SetLastSocketError(int error) { errno = error; } #else using Socket = SOCKET; static std::string LastSocketError() { @@ -50,6 +51,7 @@ static std::string LastSocketError() { snprintf(buf, sizeof(buf), "%d", WSAGetLastError()); return buf; } +static void SetLastSocketError(int error) { WSASetLastError(error); } #endif struct SockaddrStorage { @@ -388,28 +390,40 @@ static bool SocketSetNonBlocking(Socket sock) { #endif } -enum class WaitType { kRead, kWrite }; +enum class WaitType { kRead, kWrite, kConnect }; static bool WaitForSocket(Socket sock, WaitType wait_type) { - // Use an arbitrary 5-second timeout, so the test doesn't hang indefinitely if - // there's an issue. - static constexpr int kTimeoutSeconds = 5; + // Arbitrary timeouts, so the test cannot hang. |kConnect| waits on a full + // connection attempt, which Windows can take a couple of seconds to refuse. + const int timeout_seconds = wait_type == WaitType::kConnect ? 30 : 5; + // A failed connection attempt can surface as an error, not writability. + const bool watch_errors = wait_type == WaitType::kConnect; #if defined(OPENSSL_WINDOWS) - fd_set read_set, write_set; + fd_set read_set, write_set, error_set; FD_ZERO(&read_set); FD_ZERO(&write_set); + FD_ZERO(&error_set); fd_set *wait_set = wait_type == WaitType::kRead ? &read_set : &write_set; FD_SET(sock, wait_set); - timeval timeout = {kTimeoutSeconds, 0}; - if (select(0 /* unused on Windows */, &read_set, &write_set, nullptr, - &timeout) <= 0) { + if (watch_errors) { + FD_SET(sock, &error_set); + } + timeval timeout = {timeout_seconds, 0}; + if (select(0 /* unused on Windows */, &read_set, &write_set, + watch_errors ? &error_set : nullptr, &timeout) <= 0) { return false; } - return FD_ISSET(sock, wait_set); + return FD_ISSET(sock, wait_set) || + (watch_errors && FD_ISSET(sock, &error_set)); #else const short events = wait_type == WaitType::kRead ? POLLIN : POLLOUT; pollfd fd = {.fd = sock, .events = events, .revents = 0}; - return poll(&fd, 1, kTimeoutSeconds * 1000) == 1 && (fd.revents & events); + if (poll(&fd, 1, timeout_seconds * 1000) != 1) { + return false; + } + // POLLERR and POLLHUP are reported regardless of |events|. + const int accepted = watch_errors ? (events | POLLERR | POLLHUP) : events; + return (fd.revents & accepted) != 0; #endif } @@ -454,6 +468,110 @@ TEST(BIOTest, SocketConnect) { ASSERT_EQ(Bytes(kTestMessage, sizeof(kTestMessage)), Bytes(buf, sizeof(buf))); } +static void TestSocketConnectFailure(bool non_blocking) { + sockaddr_in sin; + OPENSSL_cleanse(&sin, sizeof(sin)); + sin.sin_family = AF_INET; + ASSERT_EQ(1, inet_pton(AF_INET, "127.0.0.1", &sin.sin_addr)); + + // Find an unused loopback port for the connection attempt. + OwnedSocket bound_sock(Bind(AF_INET, SOCK_STREAM, + reinterpret_cast(&sin), + sizeof(sin))); + ASSERT_TRUE(bound_sock.is_valid()) << LastSocketError(); + + SockaddrStorage addr; + ASSERT_EQ(0, getsockname(bound_sock.get(), addr.addr_mut(), &addr.len)) + << LastSocketError(); + + char hostname[80]; + snprintf(hostname, sizeof(hostname), "127.0.0.1:%d", + ntohs(addr.ToIPv4().sin_port)); + bound_sock.reset(); + + const bssl::UniquePtr bio(BIO_new_connect(hostname)); + ASSERT_TRUE(bio); + ASSERT_TRUE(BIO_set_nbio(bio.get(), non_blocking)); + + ERR_clear_error(); + ASSERT_EQ(-1, BIO_do_connect(bio.get())); + const int fd = BIO_get_fd(bio.get(), nullptr); + ASSERT_NE(-1, fd); + + // Some platforms (e.g. BSDs) refuse a non-blocking loopback connect + // immediately. + const bool blocked = non_blocking && BIO_should_retry(bio.get()); + if (blocked) { + ASSERT_TRUE(WaitForSocket(fd, WaitType::kConnect)) << LastSocketError(); + + // A successful readiness call need not clear the retryable socket error left + // by connect. Ensure the connect BIO uses SO_ERROR, not this stale value. +#if defined(OPENSSL_WINDOWS) + SetLastSocketError(WSAEWOULDBLOCK); +#else + SetLastSocketError(EINPROGRESS); +#endif + ERR_clear_error(); + EXPECT_EQ(0, BIO_do_connect(bio.get())); + } + EXPECT_FALSE(BIO_should_retry(bio.get())); + + uint32_t error = ERR_get_error(); + EXPECT_EQ(ERR_LIB_SYS, ERR_GET_LIB(error)); + // |ERR_GET_REASON| truncates to 12 bits, so Winsock codes (10000 and up) + // cannot round-trip. Only assert the reason where it is representable. +#if !defined(OPENSSL_WINDOWS) + EXPECT_EQ(ECONNREFUSED, ERR_GET_REASON(error)); +#endif + // The reason depends on how the connect failed, not the NBIO setting. + const int reason = blocked ? BIO_R_NBIO_CONNECT_ERROR : BIO_R_CONNECT_ERROR; + error = ERR_get_error(); + EXPECT_EQ(ERR_LIB_BIO, ERR_GET_LIB(error)); + EXPECT_EQ(reason, ERR_GET_REASON(error)); + EXPECT_EQ(0u, ERR_get_error()); + + // Even if the peer starts listening, the failed BIO must not silently create + // a new socket or interpret a cleared SO_ERROR as a successful connection. + bound_sock = Bind(AF_INET, SOCK_STREAM, addr.addr(), addr.len); + ASSERT_TRUE(bound_sock.is_valid()) << LastSocketError(); + ASSERT_EQ(0, listen(bound_sock.get(), 1)) << LastSocketError(); + ASSERT_EQ(0, BIO_do_connect(bio.get())); + EXPECT_FALSE(BIO_should_retry(bio.get())); + EXPECT_EQ(fd, BIO_get_fd(bio.get(), nullptr)); + error = ERR_get_error(); + EXPECT_EQ(ERR_LIB_BIO, ERR_GET_LIB(error)); + EXPECT_EQ(reason, ERR_GET_REASON(error)); + EXPECT_EQ(0u, ERR_get_error()); + + // Reading and writing must also fail without reporting a stale system error. + char buf[4]; + EXPECT_EQ(0, BIO_read(bio.get(), buf, sizeof(buf))); + EXPECT_FALSE(BIO_should_retry(bio.get())); + error = ERR_get_error(); + EXPECT_EQ(ERR_LIB_BIO, ERR_GET_LIB(error)); + EXPECT_EQ(reason, ERR_GET_REASON(error)); + EXPECT_EQ(0u, ERR_get_error()); + + EXPECT_EQ(0, BIO_write(bio.get(), "test", 4)); + EXPECT_FALSE(BIO_should_retry(bio.get())); + error = ERR_get_error(); + EXPECT_EQ(ERR_LIB_BIO, ERR_GET_LIB(error)); + EXPECT_EQ(reason, ERR_GET_REASON(error)); + EXPECT_EQ(0u, ERR_get_error()); + + // An explicit reset closes the failed socket and permits a fresh connection. + ASSERT_EQ(0, BIO_reset(bio.get())); + EXPECT_EQ(-1, BIO_get_fd(bio.get(), nullptr)); + ASSERT_TRUE(BIO_set_nbio(bio.get(), 0)); + ASSERT_EQ(1, BIO_do_connect(bio.get())) << LastSocketError(); + EXPECT_FALSE(BIO_should_retry(bio.get())); + EXPECT_EQ(0u, ERR_get_error()); +} + +TEST(BIOTest, SocketConnectFailure) { TestSocketConnectFailure(false); } + +TEST(BIOTest, SocketNonBlockingConnectFailure) { TestSocketConnectFailure(true); } + TEST(BIOTest, SocketNonBlocking) { OwnedSocket listening_sock = ListenLoopback(SOCK_STREAM); ASSERT_TRUE(listening_sock.is_valid()) << LastSocketError(); diff --git a/crypto/bio/bio_test.cc b/crypto/bio/bio_test.cc index 44e67f0fa4..1a6f44ce77 100644 --- a/crypto/bio/bio_test.cc +++ b/crypto/bio/bio_test.cc @@ -1052,7 +1052,7 @@ static int callback_invoked = 0; static long callback(BIO *b, int state, int res) { callback_invoked = 1; - EXPECT_EQ(state, 0); + EXPECT_EQ(state, 3 /* BIO_CONN_S_ERROR */); EXPECT_EQ(res, -1); return 0; } @@ -1060,6 +1060,8 @@ static long callback(BIO *b, int state, int res) { TEST(BIOTest, InvokeConnectCallback) { #if defined(TARGET_OS_IPHONE) && TARGET_OS_IPHONE GTEST_SKIP() << "InvokeConnectCallback does not run on iOS"; +#elif defined(OPENSSL_WASM) + GTEST_SKIP() << "InvokeConnectCallback requires socket support"; #endif ASSERT_EQ(callback_invoked, 0); diff --git a/crypto/bio/connect.c b/crypto/bio/connect.c index b7f6c546bb..c18175235e 100644 --- a/crypto/bio/connect.c +++ b/crypto/bio/connect.c @@ -32,6 +32,9 @@ enum { BIO_CONN_S_BEFORE, BIO_CONN_S_BLOCKED_CONNECT, BIO_CONN_S_OK, + // BIO_CONN_S_ERROR is terminal: the connect attempt failed and its error + // was already reported. Keep last; |bio_info_cb| sees these values. + BIO_CONN_S_ERROR, }; typedef struct bio_connect_st { @@ -41,6 +44,9 @@ typedef struct bio_connect_st { char *param_port; int nbio; + // error_reason is the |BIO_R_*| reason reported in |BIO_CONN_S_ERROR|. + int error_reason; + unsigned short port; struct sockaddr_storage them; @@ -189,6 +195,8 @@ static int conn_state(BIO *bio, BIO_CONNECT *c) { OPENSSL_PUT_ERROR(BIO, BIO_R_CONNECT_ERROR); ERR_add_error_data(4, "host=", c->param_hostname, ":", c->param_port); + c->state = BIO_CONN_S_ERROR; + c->error_reason = BIO_R_CONNECT_ERROR; } goto exit_loop; } else { @@ -198,17 +206,29 @@ static int conn_state(BIO *bio, BIO_CONNECT *c) { case BIO_CONN_S_BLOCKED_CONNECT: i = bio_sock_error_get_and_clear(bio->num); + if (i < 0) { + BIO_clear_retry_flags(bio); + OPENSSL_PUT_SYSTEM_ERROR(); + OPENSSL_PUT_ERROR(BIO, BIO_R_NBIO_CONNECT_ERROR); + ERR_add_error_data(4, "host=", c->param_hostname, ":", c->param_port); + c->state = BIO_CONN_S_ERROR; + c->error_reason = BIO_R_NBIO_CONNECT_ERROR; + ret = 0; + goto exit_loop; + } if (i) { - if (bio_socket_should_retry(ret)) { + if (bio_socket_error_is_retryable(i)) { BIO_set_flags(bio, (BIO_FLAGS_IO_SPECIAL | BIO_FLAGS_SHOULD_RETRY)); c->state = BIO_CONN_S_BLOCKED_CONNECT; bio->retry_reason = BIO_RR_CONNECT; ret = -1; } else { BIO_clear_retry_flags(bio); - OPENSSL_PUT_SYSTEM_ERROR(); + OPENSSL_PUT_ERROR(SYS, i); OPENSSL_PUT_ERROR(BIO, BIO_R_NBIO_CONNECT_ERROR); ERR_add_error_data(4, "host=", c->param_hostname, ":", c->param_port); + c->state = BIO_CONN_S_ERROR; + c->error_reason = BIO_R_NBIO_CONNECT_ERROR; ret = 0; } goto exit_loop; @@ -217,6 +237,15 @@ static int conn_state(BIO *bio, BIO_CONNECT *c) { } break; + case BIO_CONN_S_ERROR: + // The original error was already reported. Do not start a new connection + // or re-read |SO_ERROR|, which may have been cleared. |errno| is stale too. + BIO_clear_retry_flags(bio); + OPENSSL_PUT_ERROR(BIO, c->error_reason); + ERR_add_error_data(4, "host=", c->param_hostname, ":", c->param_port); + ret = 0; + goto exit_loop; + case BIO_CONN_S_OK: ret = 1; goto exit_loop; diff --git a/crypto/bio/errno.c b/crypto/bio/errno.c index 8a07b36933..8ec81dc2d3 100644 --- a/crypto/bio/errno.c +++ b/crypto/bio/errno.c @@ -9,31 +9,31 @@ int bio_errno_should_retry(int return_value) { - if (return_value != -1) { - return 0; - } + return return_value == -1 && bio_errno_is_retryable(errno); +} +int bio_errno_is_retryable(int error) { return #ifdef EWOULDBLOCK - errno == EWOULDBLOCK || + error == EWOULDBLOCK || #endif #ifdef ENOTCONN - errno == ENOTCONN || + error == ENOTCONN || #endif #ifdef EINTR - errno == EINTR || + error == EINTR || #endif #ifdef EAGAIN - errno == EAGAIN || + error == EAGAIN || #endif #ifdef EPROTO - errno == EPROTO || + error == EPROTO || #endif #ifdef EINPROGRESS - errno == EINPROGRESS || + error == EINPROGRESS || #endif #ifdef EALREADY - errno == EALREADY || + error == EALREADY || #endif 0; } diff --git a/crypto/bio/internal.h b/crypto/bio/internal.h index 4f25a44be2..92736e32bf 100644 --- a/crypto/bio/internal.h +++ b/crypto/bio/internal.h @@ -52,9 +52,14 @@ int bio_socket_nbio(int sock, int on); // bio_clear_socket_error clears the last socket error on |sock|. void bio_clear_socket_error(int sock); -// bio_sock_error_get_and_clear clears and returns the last socket error on |sock|. +// bio_sock_error_get_and_clear clears and returns the last socket error on +// |sock|, or -1 if querying the socket error failed. int bio_sock_error_get_and_clear(int sock); +// bio_socket_error_is_retryable returns non-zero if |error| is a non-fatal socket +// error. |error| is a Winsock error on Windows and an errno value elsewhere. +int bio_socket_error_is_retryable(int error); + // bio_socket_should_retry returns non-zero if |return_value| indicates an error // and the last socket error indicates that it's non-fatal. int bio_socket_should_retry(int return_value); @@ -84,6 +89,9 @@ union bio_addr_st { // and |errno| indicates that it's non-fatal. int bio_errno_should_retry(int return_value); +// bio_errno_is_retryable returns non-zero if |error| is a non-fatal errno value. +int bio_errno_is_retryable(int error); + #if defined(__cplusplus) } // extern C #endif diff --git a/crypto/bio/socket_helper.c b/crypto/bio/socket_helper.c index a7ff9d3505..db25b04f29 100644 --- a/crypto/bio/socket_helper.c +++ b/crypto/bio/socket_helper.c @@ -11,6 +11,9 @@ #if !defined(OPENSSL_NO_SOCK) +#include +// Older glibc versions require before so struct timespec is +// complete when includes . #include #include #include @@ -108,17 +111,28 @@ int bio_sock_error_get_and_clear(int sock) { socklen_t error_size = sizeof(error); // Get and clear the pending socket error. The SO_ERROR option is read-only. if (getsockopt(sock, SOL_SOCKET, SO_ERROR, (char *)&error, &error_size) < 0) { - return 1; + return -1; } return error; } -int bio_socket_should_retry(int return_value) { +int bio_socket_error_is_retryable(int error) { #if defined(OPENSSL_WINDOWS) - return return_value == -1 && (WSAGetLastError() == WSAEWOULDBLOCK); + return error == WSAEWOULDBLOCK; #else // On POSIX platforms, sockets and fds are the same. - return bio_errno_should_retry(return_value); + return bio_errno_is_retryable(error); +#endif +} + +int bio_socket_should_retry(int return_value) { + if (return_value != -1) { + return 0; + } +#if defined(OPENSSL_WINDOWS) + return bio_socket_error_is_retryable(WSAGetLastError()); +#else + return bio_socket_error_is_retryable(errno); #endif }