net: format socket errors with a thread-safe renderer (fixes #625)

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
 

include/morph/net/detail/tcp_socket.hpp

@@ -11,12 +11,13 @@
 #include <algorithm>
 #include <cerrno>
 #include <chrono>
+#include <cstddef>
 #include <cstdint>
-#include <cstring>
 #include <limits>
 #include <optional>
 #include <stdexcept>
 #include <string>
+#include <system_error>
 
 #if defined(__APPLE__)
 #include <signal.h>
@@ -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<sockaddr*>(&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<std::size_t>(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<std::size_t>(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

tests/net/test_tcp_socket.cpp

@@ -9,12 +9,15 @@
 #include <array>
 #include <atomic>
 #include <catch2/catch_test_macros.hpp>
+#include <catch2/matchers/catch_matchers_string.hpp>
+#include <cerrno>
 #include <chrono>
 #include <cstring>
 #include <morph/net/detail/tcp_socket.hpp>
 #include <optional>
 #include <stdexcept>
 #include <string>
+#include <system_error>
 #include <thread>
 #include <vector>
 
@@ -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);