Fix BIO socket error handling

crypto/bio/bio_socket_test.cc

@@ -43,13 +43,15 @@ 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() {
   char buf[DECIMAL_SIZE(int) + 1];
   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<const sockaddr *>(&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(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();

crypto/bio/bio_test.cc

@@ -1052,14 +1052,16 @@ 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;
 }
 
 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);

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;

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;
 }

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

crypto/bio/socket_helper.c

@@ -11,6 +11,9 @@
 
 #if !defined(OPENSSL_NO_SOCK)
 
+#include <errno.h>
+// Older glibc versions require <time.h> before <fcntl.h> so struct timespec is
+// complete when <fcntl.h> includes <sys/stat.h>.
 #include <time.h>
 #include <fcntl.h>
 #include <string.h>
@@ -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
 }