From b7fe39871b97bc6802e9548f08e381de753dd96b Mon Sep 17 00:00:00 2001 From: catbot Date: Tue, 28 Jul 2026 20:18:51 +0000 Subject: [PATCH] fix(tls): stop a peer that closed first from killing the process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenSSL's socket BIO writes with write(2) rather than send(2), so unlike PlainStream it cannot pass MSG_NOSIGNAL. Writing to a peer that is gone therefore raised SIGPIPE, and with the default disposition that takes the whole process down. This is not an edge case. It fires on any teardown where the far side closed first, because SSL_shutdown still tries to put a close_notify on the wire — which is exactly what ShouldSendRecieveHTTPS1 does when it drops a client whose certificate check failed. The test died on SIGPIPE with every assertion passing. Installing a process-wide SIG_IGN would fix it by changing how the caller's own writes report failure, which a library has no business doing. SIGPIPE from write(2) is delivered to the writing thread, so block it for that thread across each OpenSSL call instead and drain any pending instance before unblocking. A caller who already blocks SIGPIPE is left untouched — a pending signal there may be theirs to consume. Co-Authored-By: Claude Opus 4.8 --- implementations/Crafter.Network-TLS.cpp | 52 +++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/implementations/Crafter.Network-TLS.cpp b/implementations/Crafter.Network-TLS.cpp index deb4c9c..4977021 100644 --- a/implementations/Crafter.Network-TLS.cpp +++ b/implementations/Crafter.Network-TLS.cpp @@ -4,6 +4,8 @@ module; #include #include +#include +#include #include #include @@ -23,6 +25,48 @@ import std; using namespace Crafter; namespace { + // ── SIGPIPE ────────────────────────────────────────────────────────── + // OpenSSL's socket BIO writes with write(2), not send(2), so it cannot + // pass MSG_NOSIGNAL the way PlainStream does. Writing to a peer that has + // gone therefore raises SIGPIPE and — with the default disposition — kills + // the process. That is not an edge case: it happens on every teardown + // where the far side closed first, because SSL_shutdown still tries to put + // a close_notify on the wire. + // + // A library must not install a process-wide SIG_IGN on its caller's + // behalf; that would silently change how the caller's own writes behave. + // SIGPIPE from write(2) is delivered to the thread that wrote, so block it + // for this thread across the call instead, and drain any instance that + // went pending while it was blocked so it cannot fire on unblock. + class SigPipeGuard { + public: + SigPipeGuard() { + sigset_t pipeOnly; + sigemptyset(&pipeOnly); + sigaddset(&pipeOnly, SIGPIPE); + sigset_t previous; + // If the caller already blocks SIGPIPE, leave everything alone — + // any pending instance may be theirs to consume, not ours. + blocked = pthread_sigmask(SIG_BLOCK, &pipeOnly, &previous) == 0 + && sigismember(&previous, SIGPIPE) == 0; + restore = previous; + } + ~SigPipeGuard() { + if (!blocked) return; + sigset_t pipeOnly; + sigemptyset(&pipeOnly); + sigaddset(&pipeOnly, SIGPIPE); + const timespec immediately{ .tv_sec = 0, .tv_nsec = 0 }; + while (sigtimedwait(&pipeOnly, nullptr, &immediately) >= 0) {} + pthread_sigmask(SIG_SETMASK, &restore, nullptr); + } + SigPipeGuard(const SigPipeGuard&) = delete; + + private: + sigset_t restore{}; + bool blocked = false; + }; + // ── OpenSSL plumbing ───────────────────────────────────────────────── template struct Releaser { @@ -424,6 +468,7 @@ struct TLSStream::Impl { // handshake, not each poll, so a peer that dribbles records cannot // stretch it indefinitely. void Handshake(bool client, std::chrono::milliseconds timeout) { + const SigPipeGuard noSigPipe; const auto deadline = std::chrono::steady_clock::now() + timeout; for (;;) { ERR_clear_error(); @@ -546,6 +591,9 @@ StreamStatus TLSStream::ReadSome(char* buffer, std::size_t size, std::size_t& read) { read = 0; if (size == 0) return StreamStatus::Data; + // A read can put bytes on the wire too — a TLS 1.3 key update, or an alert + // in response to something we refuse. + const SigPipeGuard noSigPipe; const auto deadline = std::chrono::steady_clock::now() + timeout; const int wanted = static_cast(std::min(size, INT_MAX)); for (;;) { @@ -586,6 +634,7 @@ StreamStatus TLSStream::ReadSome(char* buffer, std::size_t size, void TLSStream::Write(const void* buffer, std::size_t size, std::chrono::milliseconds timeout) { + const SigPipeGuard noSigPipe; const auto deadline = std::chrono::steady_clock::now() + timeout; const char* data = reinterpret_cast(buffer); std::size_t sent = 0; @@ -625,6 +674,9 @@ void TLSStream::Write(const void* buffer, std::size_t size, void TLSStream::Shutdown() noexcept { if (!impl || impl->ssl == nullptr || impl->shutdownSent) return; impl->shutdownSent = true; + // The likeliest SIGPIPE of the lot: by the time a connection is being torn + // down the peer has often gone already. + const SigPipeGuard noSigPipe; // One attempt only: close_notify goes out, and we deliberately do not // wait for the peer's. Waiting means blocking a teardown path on a peer // that may never answer, and every framing decision has already been made