From 73dcd474b5bd2dbf21b3cf84dac7c10826e175e7 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 15 Jun 2023 15:03:42 -0400 Subject: [PATCH] Turn SocketCloser in bssl_shim into a proper owning type It's a bit more verbose to set up, but makes the error paths in Connect() tidier. While I'm here, stick to Windows' actual SOCKET type until we have to cross into BIO. It doesn't really matter (Windows cannot use the upper half of that type without badly breaking backwards compatibility), but it silences some 64/32 truncation warnings. Change-Id: I7be7c2b543373a7a9fc50711131e5345d84ebb8b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60886 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- ssl/test/bssl_shim.cc | 126 ++++++++++++++++++++++++++---------------- 1 file changed, 79 insertions(+), 47 deletions(-) diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index c14d7c060..6afbcc050 100644 --- a/ssl/test/bssl_shim.cc +++ b/ssl/test/bssl_shim.cc @@ -68,14 +68,14 @@ OPENSSL_MSVC_PRAGMA(comment(lib, "Ws2_32.lib")) #if !defined(OPENSSL_WINDOWS) -static int closesocket(int sock) { - return close(sock); -} +using Socket = int; +#define INVALID_SOCKET (-1) -static void PrintSocketError(const char *func) { - perror(func); -} +static int closesocket(int sock) { return close(sock); } +static void PrintSocketError(const char *func) { perror(func); } #else +using Socket = SOCKET; + static void PrintSocketError(const char *func) { int error = WSAGetLastError(); char *buffer; @@ -94,6 +94,57 @@ static void PrintSocketError(const char *func) { } #endif +class OwnedSocket { + public: + OwnedSocket() = default; + explicit OwnedSocket(Socket sock) : sock_(sock) {} + OwnedSocket(OwnedSocket &&other) { *this = std::move(other); } + ~OwnedSocket() { reset(); } + OwnedSocket &operator=(OwnedSocket &&other) { + drain_on_close_ = other.drain_on_close_; + reset(other.release()); + return *this; + } + + bool is_valid() const { return sock_ != INVALID_SOCKET; } + void set_drain_on_close(bool drain) { drain_on_close_ = drain; } + + void reset(Socket sock = INVALID_SOCKET) { + if (is_valid()) { + if (drain_on_close_) { +#if defined(OPENSSL_WINDOWS) + shutdown(sock_, SD_SEND); +#else + shutdown(sock_, SHUT_WR); +#endif + while (true) { + char buf[1024]; + if (recv(sock_, buf, sizeof(buf), 0) <= 0) { + break; + } + } + closesocket(sock_); + } + } + + drain_on_close_ = false; + sock_ = sock; + } + + Socket get() const { return sock_; } + + Socket release() { + Socket sock = sock_; + sock_ = INVALID_SOCKET; + drain_on_close_ = false; + return sock; + } + + private: + Socket sock_ = INVALID_SOCKET; + bool drain_on_close_ = false; +}; + static int Usage(const char *program) { fprintf(stderr, "Usage: %s [flags...]\n", program); return 1; @@ -107,7 +158,7 @@ struct Free { }; // Connect returns a new socket connected to the runner, or -1 on error. -static int Connect(const TestConfig *config) { +static OwnedSocket Connect(const TestConfig *config) { sockaddr_storage addr; socklen_t addr_len = 0; if (config->ipv6) { @@ -117,7 +168,7 @@ static int Connect(const TestConfig *config) { sin6.sin6_port = htons(config->port); if (!inet_pton(AF_INET6, "::1", &sin6.sin6_addr)) { PrintSocketError("inet_pton"); - return -1; + return OwnedSocket(); } addr_len = sizeof(sin6); memcpy(&addr, &sin6, addr_len); @@ -128,60 +179,34 @@ static int Connect(const TestConfig *config) { sin.sin_port = htons(config->port); if (!inet_pton(AF_INET, "127.0.0.1", &sin.sin_addr)) { PrintSocketError("inet_pton"); - return -1; + return OwnedSocket(); } addr_len = sizeof(sin); memcpy(&addr, &sin, addr_len); } - int sock = socket(addr.ss_family, SOCK_STREAM, 0); - if (sock == -1) { + OwnedSocket sock(socket(addr.ss_family, SOCK_STREAM, 0)); + if (!sock.is_valid()) { PrintSocketError("socket"); - return -1; + return OwnedSocket(); } int nodelay = 1; - if (setsockopt(sock, IPPROTO_TCP, TCP_NODELAY, + if (setsockopt(sock.get(), IPPROTO_TCP, TCP_NODELAY, reinterpret_cast(&nodelay), sizeof(nodelay)) != 0) { PrintSocketError("setsockopt"); - closesocket(sock); - return -1; + return OwnedSocket(); } - if (connect(sock, reinterpret_cast(&addr), addr_len) != 0) { + if (connect(sock.get(), reinterpret_cast(&addr), + addr_len) != 0) { PrintSocketError("connect"); - closesocket(sock); - return -1; + return OwnedSocket(); } return sock; } -class SocketCloser { - public: - explicit SocketCloser(int sock) : sock_(sock) {} - ~SocketCloser() { - // Half-close and drain the socket before releasing it. This seems to be - // necessary for graceful shutdown on Windows. It will also avoid write - // failures in the test runner. -#if defined(OPENSSL_WINDOWS) - shutdown(sock_, SD_SEND); -#else - shutdown(sock_, SHUT_WR); -#endif - while (true) { - char buf[1024]; - if (recv(sock_, buf, sizeof(buf), 0) <= 0) { - break; - } - } - closesocket(sock_); - } - - private: - const int sock_; -}; - // DoRead reads from |ssl|, resolving any asynchronous operations. It returns // the result value of the final |SSL_read| call. static int DoRead(SSL *ssl, uint8_t *out, size_t max_out) { @@ -787,13 +812,20 @@ static bool DoConnection(bssl::UniquePtr *out_session, #endif } - int sock = Connect(config); - if (sock == -1) { + OwnedSocket sock = Connect(config); + if (!sock.is_valid()) { return false; } - SocketCloser closer(sock); - bssl::UniquePtr bio(BIO_new_socket(sock, BIO_NOCLOSE)); + // Half-close and drain the socket before releasing it. This seems to be + // necessary for graceful shutdown on Windows. It will also avoid write + // failures in the test runner. + sock.set_drain_on_close(true); + + // Windows uses |SOCKET| for socket types, but OpenSSL's API requires casting + // them to |int|. + bssl::UniquePtr bio( + BIO_new_socket(static_cast(sock.get()), BIO_NOCLOSE)); if (!bio) { return false; }