From 16fa62a78d5e6048375dec320b4317395b32f71b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 22:53:53 +0200 Subject: [PATCH] net: format socket errors with a thread-safe renderer (fixes #625) Every throw site in `TcpSocket` built its message with `std::strerror`, which is permitted to return a pointer to one static buffer shared by all callers, and every one of them runs on whichever thread hit the error. This subsystem spawns those threads itself: `SocketServer` runs an accept loop thread plus one `clientLoop` thread per accepted connection (each driving `recvSome`/`sendAll`), and `SocketBackend` runs an I/O thread and a handler thread. Two of them can be inside a throw site at the same moment. Replaced with a private `errnoMessage()` over `std::system_category().message()`, which returns an owned `std::string` and carries the library's ordinary "shall not introduce a data race" guarantee. Chosen over `strerror_r`, whose XSI and GNU variants differ in return type and so need a build-time discriminator and a caller-supplied buffer. `TcpSocket::connect`'s `::gai_strerror` is deliberately left alone. It is a different function rendering `EAI_*` resolver codes, which are not `errno` values, so `std::system_category()` cannot describe them -- there is no drop-in substitution, and clang-tidy's `concurrency-mt-unsafe` does not classify it as unsafe (measured: it reports no finding on that line). Filed separately rather than swept in. The hand-written `NOLINTNEXTLINE(concurrency-mt-unsafe)` at the `tryAccept()` site goes with it. Its reason ("as at every other throw site here") generalised a suppression to six sites that never carried one, which is the shape #627 was about. Measured on this branch, clang-tidy 22.1.8, same invocation each time (`clang-tidy -p --checks='-*,concurrency-mt-unsafe' --header-filter='include/morph/.*' tests/net/test_tcp_socket.cpp`): a8511aa6 as-is 6 findings, exit 1 (the 7th suppressed) a8511aa6 with the NOLINT gone 7 findings, exit 1 this commit 0 findings, exit 0 Not measured: the race itself. No interleaved or corrupted message was observed; the defect is what the specification of `std::strerror` permits, inferred from the code. The added test pins the message shape and the category that renders it -- it was run against the pre-change header and passed there too, so it is a standing guard, not evidence for this change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- docs/spec/security.md | 21 +++++++++++++ include/morph/net/detail/tcp_socket.hpp | 41 +++++++++++++++++++------ tests/net/test_tcp_socket.cpp | 23 ++++++++++++++ 3 files changed, 76 insertions(+), 9 deletions(-) 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);