diff --git a/docs/spec/security.md b/docs/spec/security.md index 42913547..66842a86 100644 --- a/docs/spec/security.md +++ b/docs/spec/security.md @@ -616,6 +616,27 @@ transport above is not a matter of degree: (without a Close status code — see [backend.md](core/backend.md#limitations)). This is the one area where the two transports are comparable; it is also the only one. +- **Socket errors are rendered with a thread-safe formatter.** Every throw + site in `net/detail/tcp_socket.hpp` builds its message on whichever thread + hit the error, and this transport owns several of them: `SocketServer` runs + an accept loop thread plus one `clientLoop` thread per accepted connection, + and `SocketBackend` runs an I/O thread and a handler thread. A peer that + provokes socket errors on several connections at once therefore has several + threads rendering an `errno` at the same moment. They go through + `std::system_category().message()`, which returns an owned `std::string` and + carries the library's ordinary "shall not introduce a data race" guarantee, + rather than `std::strerror`, which is permitted to hand every caller a + pointer to one shared static buffer (morph#625). Stated precisely, because + the distinction matters: what was repaired is the data race the + specification of `std::strerror` permits, inferred from the code. No + interleaved or corrupted message was ever observed, and on the glibc/Linux + configuration this project tests, the two spellings render an `errno` to + identical bytes. The property gained is that the guarantee now holds by + specification rather than by the implementation happening to be safe. + `TcpSocket::connect`'s `::gai_strerror` is deliberately untouched: it renders + `EAI_*` resolver codes, which are not `errno` values, so + `std::system_category()` cannot describe them and no drop-in substitution + exists. ## Residual limitations & hardening checklist diff --git a/include/morph/net/detail/tcp_socket.hpp b/include/morph/net/detail/tcp_socket.hpp index 2ee1abb0..80c85594 100644 --- a/include/morph/net/detail/tcp_socket.hpp +++ b/include/morph/net/detail/tcp_socket.hpp @@ -11,12 +11,13 @@ #include #include #include +#include #include -#include #include #include #include #include +#include #if defined(__APPLE__) #include @@ -192,7 +193,7 @@ class TcpSocket { static TcpSocket listen(std::uint16_t port, int backlog = 64) { int fd = ::socket(AF_INET, SOCK_STREAM, 0); if (fd < 0) { - throw std::runtime_error(std::string{"TcpSocket::listen: socket() failed: "} + std::strerror(errno)); + throw std::runtime_error("TcpSocket::listen: socket() failed: " + errnoMessage(errno)); } int const reuse = 1; ::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &reuse, sizeof(reuse)); @@ -203,12 +204,12 @@ class TcpSocket { if (::bind(fd, reinterpret_cast(&addr), sizeof(addr)) != 0) { int const err = errno; ::close(fd); - throw std::runtime_error(std::string{"TcpSocket::listen: bind() failed: "} + std::strerror(err)); + throw std::runtime_error("TcpSocket::listen: bind() failed: " + errnoMessage(err)); } if (::listen(fd, backlog) != 0) { int const err = errno; ::close(fd); - throw std::runtime_error(std::string{"TcpSocket::listen: listen() failed: "} + std::strerror(err)); + throw std::runtime_error("TcpSocket::listen: listen() failed: " + errnoMessage(err)); } return TcpSocket{fd}; } @@ -273,7 +274,7 @@ class TcpSocket { if (errno == EINTR) { continue; } - throw std::runtime_error(std::string{"TcpSocket::accept: "} + std::strerror(errno)); + throw std::runtime_error("TcpSocket::accept: " + errnoMessage(errno)); } } @@ -328,8 +329,7 @@ class TcpSocket { if (wouldBlock(err)) { return std::nullopt; } - // NOLINTNEXTLINE(concurrency-mt-unsafe) — std::strerror, as at every other throw site here - throw std::runtime_error(std::string{"TcpSocket::tryAccept: "} + std::strerror(err)); + throw std::runtime_error("TcpSocket::tryAccept: " + errnoMessage(err)); } } @@ -350,7 +350,7 @@ class TcpSocket { if (errno == ECONNRESET) { return 0; } - throw std::runtime_error(std::string{"TcpSocket::recvSome: "} + std::strerror(errno)); + throw std::runtime_error("TcpSocket::recvSome: " + errnoMessage(errno)); } return static_cast(n); } @@ -368,7 +368,7 @@ class TcpSocket { if (errno == EINTR) { continue; } - throw std::runtime_error(std::string{"TcpSocket::sendAll: "} + std::strerror(errno)); + throw std::runtime_error("TcpSocket::sendAll: " + errnoMessage(errno)); } sent += static_cast(n); } @@ -445,6 +445,29 @@ class TcpSocket { [[nodiscard]] bool valid() const noexcept { return _fd >= 0; } private: + /// Describes `err` the way `std::strerror` would, without `std::strerror`'s + /// shared static buffer. + /// + /// Every throw site in this class formats its message on whichever thread + /// hit the error, and this subsystem spawns those threads itself: + /// `SocketServer` runs an accept loop thread plus one `clientLoop` thread + /// per accepted connection (each of which drives `recvSome`/`sendAll`), and + /// `SocketBackend` runs an I/O thread and a handler thread. Two of them can + /// therefore be inside a throw site at the same moment, and `std::strerror` + /// is permitted to return a pointer to one buffer shared by all callers -- + /// a data race on the message, not merely an interleaved string. + /// + /// `std::error_category::message` is specified with no such carve-out, so + /// it carries the library's ordinary "shall not introduce a data race" + /// guarantee, and it returns an owned `std::string`, leaving nothing for + /// two threads to share. `std::system_category()` is the category whose + /// values are `errno` values on POSIX, which is what every caller here + /// passes. Preferred over `strerror_r` because that function's XSI and GNU + /// variants differ in return type, so a portable call needs a build-time + /// discriminator and a caller-supplied buffer; this needs neither. + /// morph#625. + static std::string errnoMessage(int err) { return std::system_category().message(err); } + /// POSIX allows `EAGAIN` and `EWOULDBLOCK` to differ, and both name the /// same "nothing to take right now" answer. Written as two statements /// rather than `err == EAGAIN || err == EWOULDBLOCK` so GCC's diff --git a/tests/net/test_tcp_socket.cpp b/tests/net/test_tcp_socket.cpp index 08af5fb4..34f6a8f8 100644 --- a/tests/net/test_tcp_socket.cpp +++ b/tests/net/test_tcp_socket.cpp @@ -9,12 +9,15 @@ #include #include #include +#include +#include #include #include #include #include #include #include +#include #include #include @@ -396,6 +399,26 @@ TEST_CASE("TcpSocket::listen: fails with EADDRINUSE when the port is already bou REQUIRE_THROWS_AS(TcpSocket::listen(port), std::runtime_error); } +// Pins the *shape* of a socket error message, and pins it to the category that +// renders it rather than to a literal string. Every throw site in +// `tcp_socket.hpp` formats its message on whichever thread hit the error, and +// this subsystem spawns those threads itself, so the renderer has to be one +// that two threads may call at once -- `std::error_category::message`, not +// `std::strerror` (morph#625). +// +// What this case does not establish: that the previous `std::strerror` +// spelling was actually racing. glibc renders both spellings to the same +// bytes, so this assertion would have held before the change too. The evidence +// for the change is clang-tidy `concurrency-mt-unsafe` going from six findings +// in this header to none; this case is a standing guard on the message, not +// that measurement. +TEST_CASE("TcpSocket::listen renders a bind() failure through std::system_category", "[net][tcp]") { + auto first = TcpSocket::listen(0); + std::uint16_t const port = first.boundPort(); + REQUIRE_THROWS_WITH(TcpSocket::listen(port), Catch::Matchers::Equals("TcpSocket::listen: bind() failed: " + + std::system_category().message(EADDRINUSE))); +} + TEST_CASE("TcpSocket::boundPort: returns 0 on an empty socket", "[net][tcp]") { TcpSocket const empty; REQUIRE(empty.boundPort() == 0U);