morph::net formats every socket error with the non-thread-safe std::strerror, from threads it spawns itself

[Yaraslaut]

Summary

include/morph/net/detail/tcp_socket.hpp builds every one of its error messages
with std::strerror(errno). std::strerror is not required to be thread-safe —
glibc returns a pointer into a static buffer for unknown error numbers and is
explicitly documented as not MT-safe — and this header is used from a design
that is multi-threaded by construction:

• net/socket_server.hpp:95 spawns an accept thread, and :280 spawns one
  thread per client connection;
• net/socket_backend.hpp:106-107 spawns an I/O thread and a handler thread.

Two of those threads hitting a socket error at the same time can read a buffer
the other is writing. The damage is confined to the message text of a
std::runtime_error — no data is corrupted — so this is a wrong-diagnostic bug,
not a crash. It is also trivially fixable.

The seven sites

$ grep -rn "strerror" include src | grep -v strerror_r
include/morph/net/detail/tcp_socket.hpp:111:            throw std::runtime_error("TcpSocket::connect: getaddrinfo failed for " + host + ": " + ::gai_strerror(rc));
include/morph/net/detail/tcp_socket.hpp:195:            throw std::runtime_error(std::string{"TcpSocket::listen: socket() failed: "} + std::strerror(errno));
include/morph/net/detail/tcp_socket.hpp:206:            throw std::runtime_error(std::string{"TcpSocket::listen: bind() failed: "} + std::strerror(err));
include/morph/net/detail/tcp_socket.hpp:211:            throw std::runtime_error(std::string{"TcpSocket::listen: listen() failed: "} + std::strerror(err));
include/morph/net/detail/tcp_socket.hpp:276:            throw std::runtime_error(std::string{"TcpSocket::accept: "} + std::strerror(errno));
include/morph/net/detail/tcp_socket.hpp:331:            // NOLINTNEXTLINE(concurrency-mt-unsafe) — std::strerror, as at every other throw site here
include/morph/net/detail/tcp_socket.hpp:332:            throw std::runtime_error(std::string{"TcpSocket::tryAccept: "} + std::strerror(err));
include/morph/net/detail/tcp_socket.hpp:353:                throw std::runtime_error(std::string{"TcpSocket::recvSome: "} + std::strerror(errno));
include/morph/net/detail/tcp_socket.hpp:371:                throw std::runtime_error(std::string{"TcpSocket::sendAll: "} + std::strerror(errno));

:111 is ::gai_strerror, which is a different function and is thread-safe on
glibc; it is listed only for completeness. The other seven are std::strerror.

Of the seven, sendAll and recvSome (:371, :353) are the ones actually
reached concurrently in normal operation: socket_server.hpp's per-client
threads and socket_backend.hpp's I/O thread each drive their own socket, and a
transport dropping several connections at once is exactly when two of them throw
together.

The existing suppression is the pattern #580 warns about

Line 331 already carries:

// NOLINTNEXTLINE(concurrency-mt-unsafe) — std::strerror, as at every other throw site here

The stated reason is consistency with the other six sites, not safety. It
suppresses one instance of a check while leaving six identical ones live, and it
does not address the thing the check is reporting. #580 names this shape
directly — "the cheapest way out is a NOLINT, which is how a check set
decays" — and include/morph/render/locale_format.hpp:181 shows what the
alternative looks like in this tree (a paragraph explaining why the check is
wrong here).

Whatever is decided about the code, that comment should either become a real
argument or go away with the fix.

Verification status

Reproduced (the findings). On a020e69c, clang-tidy 22.1.8, .clang-tidy
unmodified, from a full-database sweep of all 695 translation units with
MORPH_BUILD_NET=ON:

include/morph/net/detail/tcp_socket.hpp:195:92: error: function is not thread safe [concurrency-mt-unsafe]
include/morph/net/detail/tcp_socket.hpp:206:90: error: function is not thread safe [concurrency-mt-unsafe]
include/morph/net/detail/tcp_socket.hpp:211:92: error: function is not thread safe [concurrency-mt-unsafe]
include/morph/net/detail/tcp_socket.hpp:276:75: error: function is not thread safe [concurrency-mt-unsafe]
include/morph/net/detail/tcp_socket.hpp:353:81: error: function is not thread safe [concurrency-mt-unsafe]
include/morph/net/detail/tcp_socket.hpp:371:80: error: function is not thread safe [concurrency-mt-unsafe]

Six, not seven, because :332 is suppressed. These are the only
concurrency-mt-unsafe findings anywhere under include/morph/.

Inferred from reading the code, not observed. That two of these sites can
execute concurrently: established by reading socket_server.hpp:95/280 and
socket_backend.hpp:106-107, which create the threads, and by the fact that
each thread owns its own TcpSocket. I did not construct a test that makes
two threads throw at the same moment.

Not verified.

• No ThreadSanitizer report. I did not run the TSan preset against
  morph::net, and I would not expect it to fire on its own: the race needs two
  concurrent failing syscalls, which the existing tests do not arrange. This
  is latent, which is why static analysis found it and the dynamic gates have
  not.
• No demonstration of a torn message. I have not produced a wrong error
  string; the claim rests on strerror's documented contract, not on
  observation.
• Not checked on macOS/Windows. morph::net is POSIX-only
  (MORPH_BUILD_NET is ignored on Windows), so glibc and Apple libc are the
  relevant implementations; I checked neither's source, only the documented
  guarantee.

Suggested fix (not applied)

std::generic_category().message(errno) is thread-safe and returns a
std::string, so it drops straight into every one of these expressions with no
buffer management:

throw std::runtime_error(std::string{"TcpSocket::sendAll: "} +
                         std::generic_category().message(errno));

Seven sites, one #include <system_error>, and the NOLINTNEXTLINE at :331
is deleted rather than replicated. strerror_r is the C alternative but has two
incompatible signatures across libc's and would need a wrapper.

I deliberately did not apply this: it is outside the remit of the measurement
lane that found it, and AGENTS.md says to file rather than fold.

What would change the verdict

Close this when the seven sites no longer call std::strerror and the
suppression at :331 is gone, so clang-tidy reports zero
concurrency-mt-unsafe under include/morph/ with the check enabled and no
NOLINT holding it back.

Downgrade or close as won't-fix if someone establishes that std::strerror is
in fact MT-safe on every libc morph supports — in which case the six findings
want one reasoned suppression saying so, not seven copies of the current
comment.

Related

Found while running the full-tree clang-tidy census for #580; it is one of that
census's 596 findings. Filed separately because it is a claim about shipped
runtime behaviour, not about the gate's scope, and because its fix does not wait
on #580's policy decision.

[Yaraslaut]

Triage: valid — premise reproduced, with the numbers

Measured on a8511aa6.

The non-thread-safe calls exist, and there are eight of them, all in one file:

$ grep -rc "std::strerror" include/morph/net/
include/morph/net/detail/tcp_socket.hpp:8

at listen() (three), accept(), tryAccept(), recvSome(), sendAll(), and one more. (connect() uses ::gai_strerror, which is a different function with its own thread-safety rules — worth stating separately in the fix rather than sweeping into the same change.)

The threads are real and are spawned by morph::net itself:

include/morph/net/socket_backend.hpp:106:        _ioThread      = std::thread{[this] { ioThreadMain(); }};
include/morph/net/socket_backend.hpp:107:        _handlerThread = std::thread{[this] { handlerThreadMain(); }};

So two threads owned by the subsystem can be inside a TcpSocket error path at the same time, each calling a function that is permitted to return a pointer to a shared static buffer. That is a data race on the message, not merely an ugly string.

The tree already knows. tcp_socket.hpp:331 carries

// NOLINTNEXTLINE(concurrency-mt-unsafe) — std::strerror, as at every other throw site here

which is the check firing and being suppressed by hand, with a reason that generalises the suppression to seven sites that do not carry it. Whatever the fix is, that NOLINT should go with it — a suppression outliving its subject is the shape #627 was about.

triage: valid, bug, area: core — there is no area: net label; net work has been filed under area: core so far (#586, #587). Worth adding one, but I have not, since a label nobody agreed on is its own small mess.

Not verified: I did not observe an interleaved or corrupted message. The claim rests on the specification of std::strerror plus the presence of two concurrent threads, which is inference from the code, not a reproduction. Reproducing it deliberately is awkward and probably not worth a ticket's time — but that means the fix should not claim a measured before/after either.

For whoever takes it: std::system_category().message(err) is thread-safe, portable, and needs no buffer management, which makes it a better fit here than strerror_r, whose XSI and GNU variants differ in return type. Say which you chose and why.

What would change the verdict

Close as invalid if every one of the eight sites turns out to be reachable only from a single thread — in which case the finding is that the NOLINT's "as at every other throw site here" is the wrong reason, not that the calls are unsafe.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW