From 09044b226cacc5f3800fc1a19b666daa0c9f6753 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:40:25 +0800 Subject: [PATCH 01/10] [BUG] Stop embedded HTTP server writes from raising SIGPIPE Socket::send() passed a flags argument of 0, and nothing under ext/ used MSG_NOSIGNAL or SO_NOSIGPIPE. Writing to a socket whose peer has gone away therefore raised SIGPIPE, whose default disposition terminates the host process, so the server could die before any of the error handling in sendMore() ran. send() now passes MSG_NOSIGNAL where the platform defines it. Platforms that offer SO_NOSIGPIPE instead get the option set per socket, at creation and on the socket returned by accept(). Windows needs neither. The suppression is deliberately per socket rather than a change to the process signal disposition, which belongs to the embedding program. Checked on Linux with a socketpair whose peer end is closed: before this change the process is killed by signal 13, after it returns -1 with EPIPE and keeps running. Fixes #4293 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/server/socket_tools.h | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 6a5b830f9..67691a7f5 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -280,7 +280,7 @@ struct Socket Socket(Type sock = Invalid) : m_sock(sock) {} - Socket(int af, int type, int proto) : m_sock(::socket(af, type, proto)) {} + Socket(int af, int type, int proto) : m_sock(::socket(af, type, proto)) { suppressSigPipe(); } operator Socket::Type() const { return m_sock; } @@ -345,6 +345,25 @@ struct Socket m_sock = Invalid; } + /** + * Stop writes to a socket whose peer has gone away from raising SIGPIPE, whose default + * disposition would terminate the host process. Platforms that offer MSG_NOSIGNAL do this + * per call in send() instead, and Windows has no SIGPIPE at all. Deliberately per socket + * rather than by changing the process signal disposition, which belongs to the embedding + * program rather than to this library. + */ + void suppressSigPipe() + { +#ifdef SO_NOSIGPIPE + if (m_sock != Invalid) + { + int value = 1; + ::setsockopt(m_sock, SOL_SOCKET, SO_NOSIGPIPE, reinterpret_cast(&value), + sizeof(value)); + } +#endif + } + int recv(void *buffer, unsigned size) { assert(m_sock != Invalid); @@ -355,7 +374,12 @@ struct Socket int send(void const *buffer, unsigned size) { assert(m_sock != Invalid); - return static_cast(::send(m_sock, reinterpret_cast(buffer), size, 0)); +#ifdef MSG_NOSIGNAL + int const flags = MSG_NOSIGNAL; +#else + int const flags = 0; +#endif + return static_cast(::send(m_sock, reinterpret_cast(buffer), size, flags)); } bool bind(SocketAddr const &addr) @@ -390,6 +414,7 @@ struct Socket socklen_t addrlen = sizeof(caddr); #endif csock = ::accept(m_sock, caddr, &addrlen); + csock.suppressSigPipe(); return !csock.invalid(); } From 6970a6eac816889e8137be1161655935b7e8e5e1 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 00:41:47 +0800 Subject: [PATCH 02/10] Add CHANGELOG entry for the SIGPIPE suppression fix Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8baba4503..04e2a12fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ Increment the: namespaces [#4303](https://github.com/open-telemetry/opentelemetry-cpp/pull/4303) +* [BUG] Stop embedded HTTP server writes from raising SIGPIPE + [#4294](https://github.com/open-telemetry/opentelemetry-cpp/pull/4294) + * [CODE HEALTH] Move metrics storage test fixtures into anonymous namespace [#4286](https://github.com/open-telemetry/opentelemetry-cpp/pull/4286) From 5f6baa34e9a522a47352de0d42d549a7b4cc4d7b Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 11:30:50 +0800 Subject: [PATCH 03/10] Do not swallow the SO_NOSIGPIPE setsockopt result Review of #4294 noted the setsockopt return value was discarded. On a platform whose only defence is SO_NOSIGPIPE (no MSG_NOSIGNAL) a silent failure would leave writes able to raise SIGPIPE, so it is logged now. Kept as a warning rather than closing the socket: the listening socket cannot refuse itself, and on every platform CI builds MSG_NOSIGNAL is present so send() is protected regardless, which makes the accept-time failure path untestable in CI. Also corrected the comment. Modern Linux, macOS and BSD all define MSG_NOSIGNAL, so SO_NOSIGPIPE is a second layer covering writes that do not go through send() rather than the only mechanism. The closed-peer regression test the review asks for depends on the socket_tools_test.cc target introduced by #4292 and will be added on top of it. A local harness confirmed the write must exceed the socket buffer to trigger the broken pipe deterministically; a small write is buffered and does not, which the test must account for. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../opentelemetry/ext/http/server/socket_tools.h | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 67691a7f5..80d761404 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -347,8 +347,11 @@ struct Socket /** * Stop writes to a socket whose peer has gone away from raising SIGPIPE, whose default - * disposition would terminate the host process. Platforms that offer MSG_NOSIGNAL do this - * per call in send() instead, and Windows has no SIGPIPE at all. Deliberately per socket + * disposition would terminate the host process. send() passes MSG_NOSIGNAL per call wherever + * the platform defines it, which covers modern Linux, macOS and the BSDs. Where the platform + * also offers SO_NOSIGPIPE this sets it at the socket level as well: that is a second layer + * covering any write to the descriptor that does not go through send(), and the only layer on + * an older platform that lacks MSG_NOSIGNAL. Windows has no SIGPIPE. Deliberately per socket * rather than by changing the process signal disposition, which belongs to the embedding * program rather than to this library. */ @@ -358,8 +361,13 @@ struct Socket if (m_sock != Invalid) { int value = 1; - ::setsockopt(m_sock, SOL_SOCKET, SO_NOSIGPIPE, reinterpret_cast(&value), - sizeof(value)); + if (::setsockopt(m_sock, SOL_SOCKET, SO_NOSIGPIPE, reinterpret_cast(&value), + sizeof(value)) != 0) + { + // Harmless where send() also passes MSG_NOSIGNAL, but on a platform whose only defence + // is SO_NOSIGPIPE this leaves writes able to raise SIGPIPE, so do not swallow it. + LOG_WARN("Socket: cannot set SO_NOSIGPIPE (errno %d)", errno); + } } #endif } From 216186f43dcac57bcc1f7cd778702754ede56084 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:09:07 +0800 Subject: [PATCH 04/10] Add a broken-pipe regression test and scope the SO_NOSIGPIPE claim Cover the fix with a test: block SIGPIPE in the test thread, close a socketpair peer, send one byte, and assert the write reports EPIPE with no SIGPIPE pending. A closed peer fails the write immediately, so one byte is enough and no fork or death test is needed. Where the platform defines SO_NOSIGPIPE a second test checks a created socket carries it. Scope the suppressSigPipe() comments to what the code guarantees: MSG_NOSIGNAL protects send() on every platform this repo builds on, and SO_NOSIGPIPE is a best-effort second layer. The setsockopt failure is logged where a logger is configured rather than claimed to be handled, since LOG_WARN is a no-op in a default build and the wrapper neither owns the descriptor nor promises to protect a platform whose only defence would be SO_NOSIGPIPE. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- .../ext/http/server/socket_tools.h | 20 +++-- ext/test/http/BUILD | 14 ++++ ext/test/http/CMakeLists.txt | 14 ++++ ext/test/http/socket_sigpipe_test.cc | 82 +++++++++++++++++++ 4 files changed, 121 insertions(+), 9 deletions(-) create mode 100644 ext/test/http/socket_sigpipe_test.cc diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 80d761404..d4aa2f990 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -347,13 +347,13 @@ struct Socket /** * Stop writes to a socket whose peer has gone away from raising SIGPIPE, whose default - * disposition would terminate the host process. send() passes MSG_NOSIGNAL per call wherever - * the platform defines it, which covers modern Linux, macOS and the BSDs. Where the platform - * also offers SO_NOSIGPIPE this sets it at the socket level as well: that is a second layer - * covering any write to the descriptor that does not go through send(), and the only layer on - * an older platform that lacks MSG_NOSIGNAL. Windows has no SIGPIPE. Deliberately per socket - * rather than by changing the process signal disposition, which belongs to the embedding - * program rather than to this library. + * disposition would terminate the host process. The guarantee is MSG_NOSIGNAL, which send() + * passes per call on every platform that defines it, which is modern Linux, macOS and the BSDs, + * and every platform this repo builds on. Where the platform also offers SO_NOSIGPIPE this + * additionally sets it at the socket level as a best-effort second layer, covering a write to + * the descriptor that does not go through send(). Windows has no SIGPIPE. Done per socket rather + * than by changing the process signal disposition, which belongs to the embedding program rather + * than to this library. */ void suppressSigPipe() { @@ -364,8 +364,10 @@ struct Socket if (::setsockopt(m_sock, SOL_SOCKET, SO_NOSIGPIPE, reinterpret_cast(&value), sizeof(value)) != 0) { - // Harmless where send() also passes MSG_NOSIGNAL, but on a platform whose only defence - // is SO_NOSIGPIPE this leaves writes able to raise SIGPIPE, so do not swallow it. + // Best effort: MSG_NOSIGNAL already guards send() on every platform this repo builds on, + // so a failure here is logged where a logger is configured rather than failing the socket. + // This wrapper does not own the descriptor's lifetime and does not promise to protect a + // platform whose only defence would be SO_NOSIGPIPE. LOG_WARN("Socket: cannot set SO_NOSIGPIPE (errno %d)", errno); } } diff --git a/ext/test/http/BUILD b/ext/test/http/BUILD index 2e6a07e68..b60f1637a 100644 --- a/ext/test/http/BUILD +++ b/ext/test/http/BUILD @@ -3,6 +3,20 @@ load("@rules_cc//cc:cc_test.bzl", "cc_test") +cc_test( + name = "socket_sigpipe_test", + srcs = [ + "socket_sigpipe_test.cc", + ], + # SIGPIPE is POSIX-only; the test body is compiled out on Windows, leaving no winsock calls, so + # no ws2_32 linkopt is needed here. + tags = ["test"], + deps = [ + "//ext:headers", + "@com_google_googletest//:gtest_main", + ], +) + cc_test( name = "curl_http_test", srcs = [ diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index b7707bd8c..b7b8618c7 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -14,6 +14,20 @@ if(WITH_HTTP_CLIENT_CURL) TEST_LIST ${FILENAME}) endif() +# The SIGPIPE suppression is POSIX-only (Windows has no such signal), and the test body is +# compiled out under _WIN32, so build the target only where it has tests. gtest_add_tests parses +# the source for TEST() names and would otherwise register tests the Windows binary does not have. +if(NOT WIN32) + set(SOCKET_SIGPIPE_FILENAME socket_sigpipe_test) + add_executable(${SOCKET_SIGPIPE_FILENAME} ${SOCKET_SIGPIPE_FILENAME}.cc) + target_link_libraries(${SOCKET_SIGPIPE_FILENAME} opentelemetry_ext ${GMOCK_LIB} + ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) + gtest_add_tests( + TARGET ${SOCKET_SIGPIPE_FILENAME} + TEST_PREFIX ext.http.socketsigpipe. + TEST_LIST ${SOCKET_SIGPIPE_FILENAME}) +endif() + set(URL_PARSER_FILENAME url_parser_test) add_executable(${URL_PARSER_FILENAME} ${URL_PARSER_FILENAME}.cc) target_link_libraries(${URL_PARSER_FILENAME} opentelemetry_ext ${GMOCK_LIB} diff --git a/ext/test/http/socket_sigpipe_test.cc b/ext/test/http/socket_sigpipe_test.cc new file mode 100644 index 000000000..f43df57d1 --- /dev/null +++ b/ext/test/http/socket_sigpipe_test.cc @@ -0,0 +1,82 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#include + +#include "opentelemetry/ext/http/server/socket_tools.h" + +// SIGPIPE only exists on POSIX. Windows has no such signal and the suppression code is compiled +// out there, so the whole fixture is POSIX-only. +#ifndef _WIN32 + +# include +# include +# include +# include +# include + +namespace +{ + +// Writing to a socket whose peer has closed must report the error through errno rather than +// raising SIGPIPE, whose default disposition would terminate the host process; send() passes +// MSG_NOSIGNAL for exactly this reason. The peer is closed rather than merely idle, so a single +// byte is enough: the write fails immediately with EPIPE instead of being buffered. +TEST(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) +{ + // Observe SIGPIPE through sigpending() rather than the default disposition, so the test neither + // depends on nor changes the process-wide handler and cannot be killed by a stray signal. + sigset_t blocked, previous; + sigemptyset(&blocked); + sigaddset(&blocked, SIGPIPE); + ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked, &previous), 0); + + int fds[2]; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); + SocketTools::Socket sender(fds[0]); + ::close(fds[1]); // the peer is gone, so any further write is a broken pipe + + const char byte = 'x'; + errno = 0; + const int sent = sender.send(&byte, 1); + const int saved = errno; + + EXPECT_EQ(sent, -1); + EXPECT_EQ(saved, EPIPE); + + sigset_t pending; + sigemptyset(&pending); + ASSERT_EQ(sigpending(&pending), 0); + EXPECT_FALSE(sigismember(&pending, SIGPIPE)); + + // If the fix regressed and a SIGPIPE was queued, drain it so it cannot escape the test. + if (sigismember(&pending, SIGPIPE)) + { + int signo = 0; + sigwait(&blocked, &signo); + } + + sender.close(); + pthread_sigmask(SIG_SETMASK, &previous, nullptr); +} + +# ifdef SO_NOSIGPIPE +// Where the platform offers SO_NOSIGPIPE (macOS and the BSDs), a socket created through the wrapper +// should carry it as the best-effort second layer described on suppressSigPipe(). +TEST(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) +{ + SocketTools::Socket sock(AF_INET, SOCK_STREAM, 0); + ASSERT_FALSE(sock.invalid()); + + int value = 0; + socklen_t length = sizeof(value); + ASSERT_EQ(::getsockopt(sock, SOL_SOCKET, SO_NOSIGPIPE, &value, &length), 0); + EXPECT_EQ(value, 1); + + sock.close(); +} +# endif // SO_NOSIGPIPE + +} // namespace + +#endif // _WIN32 From 19b4660949f73a9f37730d0a4f7576fd7e04baf7 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:36:31 +0800 Subject: [PATCH 05/10] Apply SO_NOSIGPIPE in the broken-pipe test as production does The test wrapped a raw socketpair fd, which uses the non-owning Socket constructor that does not set SO_NOSIGPIPE. On macOS and the BSDs, where MSG_NOSIGNAL is not defined, that left the write able to raise SIGPIPE, so the pending-signal assertion would fail on those platforms. Call suppressSigPipe() as accept() and the (af,type,proto) constructor do, so the test exercises each platform's real protection layer. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/socket_sigpipe_test.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ext/test/http/socket_sigpipe_test.cc b/ext/test/http/socket_sigpipe_test.cc index f43df57d1..e343e3d61 100644 --- a/ext/test/http/socket_sigpipe_test.cc +++ b/ext/test/http/socket_sigpipe_test.cc @@ -34,6 +34,10 @@ TEST(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) int fds[2]; ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); SocketTools::Socket sender(fds[0]); + // Production sockets get SO_NOSIGPIPE from accept()/the (af,type,proto) ctor. Apply it here too: + // on macOS and the BSDs, where MSG_NOSIGNAL is not defined, that option is the layer under test, + // and a raw-fd socket without it would raise SIGPIPE and fail the pending-signal check below. + sender.suppressSigPipe(); ::close(fds[1]); // the peer is gone, so any further write is a broken pipe const char byte = 'x'; From 9ce1b34c8a62e4412502d4bdd1b7a3c973821f17 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 18:15:07 +0800 Subject: [PATCH 06/10] Harden the SIGPIPE test: fixture cleanup, negative control, accept path Rework socket_sigpipe_test into a fixture whose TearDown restores the signal mask and drains a pending SIGPIPE even when a test aborts, so it cannot pollute other tests in the shared binary. A negative control sends on a raw socket first to prove this thread observes SIGPIPE, so the suppressed case is not vacuous. Add a test that the wrapper's accept() sets SO_NOSIGPIPE on the accepted socket (the listener is wrapped from a raw fd, so it proves accept()'s own call rather than inheritance), and gate the tests on MSG_NOSIGNAL/SO_NOSIGPIPE availability rather than on _WIN32 alone. Register the target with gtest_discover_tests, which enumerates the compiled binary, so the SO_NOSIGPIPE-only tests are never registered as phantom passes on a platform that compiled them out. Add an explicit skipped test on Windows so the Bazel target is not an empty binary, and link ws2_32 there for socket_tools.h's WsaInitializer global. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- ext/test/http/BUILD | 8 +- ext/test/http/CMakeLists.txt | 20 ++-- ext/test/http/socket_sigpipe_test.cc | 164 +++++++++++++++++++++------ 3 files changed, 147 insertions(+), 45 deletions(-) diff --git a/ext/test/http/BUILD b/ext/test/http/BUILD index b60f1637a..82ab0af6b 100644 --- a/ext/test/http/BUILD +++ b/ext/test/http/BUILD @@ -8,8 +8,12 @@ cc_test( srcs = [ "socket_sigpipe_test.cc", ], - # SIGPIPE is POSIX-only; the test body is compiled out on Windows, leaving no winsock calls, so - # no ws2_32 linkopt is needed here. + # socket_tools.h instantiates a WsaInitializer global on Windows (WSAStartup), so the Windows + # build links ws2_32 even though the SIGPIPE test body itself is compiled out there. + linkopts = select({ + "//bazel:windows": ["-DEFAULTLIB:Ws2_32.lib"], + "//conditions:default": [], + }), tags = ["test"], deps = [ "//ext:headers", diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index b7b8618c7..35afe4e1a 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -14,18 +14,20 @@ if(WITH_HTTP_CLIENT_CURL) TEST_LIST ${FILENAME}) endif() -# The SIGPIPE suppression is POSIX-only (Windows has no such signal), and the test body is -# compiled out under _WIN32, so build the target only where it has tests. gtest_add_tests parses -# the source for TEST() names and would otherwise register tests the Windows binary does not have. +# The SIGPIPE suppression is POSIX-only (Windows has no such signal), so the +# CMake target is built only on POSIX; the Bazel target covers Windows with an +# explicit skipped test. gtest_discover_tests enumerates the compiled binary +# rather than parsing the source, so the SO_NOSIGPIPE-only tests are registered +# only where they are actually compiled, never as phantom tests on a platform +# that compiled them out. if(NOT WIN32) set(SOCKET_SIGPIPE_FILENAME socket_sigpipe_test) add_executable(${SOCKET_SIGPIPE_FILENAME} ${SOCKET_SIGPIPE_FILENAME}.cc) - target_link_libraries(${SOCKET_SIGPIPE_FILENAME} opentelemetry_ext ${GMOCK_LIB} - ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) - gtest_add_tests( - TARGET ${SOCKET_SIGPIPE_FILENAME} - TEST_PREFIX ext.http.socketsigpipe. - TEST_LIST ${SOCKET_SIGPIPE_FILENAME}) + target_link_libraries( + ${SOCKET_SIGPIPE_FILENAME} opentelemetry_ext ${GMOCK_LIB} + ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) + gtest_discover_tests(${SOCKET_SIGPIPE_FILENAME} + TEST_PREFIX ext.http.socketsigpipe.) endif() set(URL_PARSER_FILENAME url_parser_test) diff --git a/ext/test/http/socket_sigpipe_test.cc b/ext/test/http/socket_sigpipe_test.cc index e343e3d61..26a903664 100644 --- a/ext/test/http/socket_sigpipe_test.cc +++ b/ext/test/http/socket_sigpipe_test.cc @@ -6,7 +6,8 @@ #include "opentelemetry/ext/http/server/socket_tools.h" // SIGPIPE only exists on POSIX. Windows has no such signal and the suppression code is compiled -// out there, so the whole fixture is POSIX-only. +// out there, so the POSIX tests live behind _WIN32 and Windows gets an explicit skipped test +// rather than an empty binary. #ifndef _WIN32 # include @@ -18,56 +19,108 @@ namespace { -// Writing to a socket whose peer has closed must report the error through errno rather than -// raising SIGPIPE, whose default disposition would terminate the host process; send() passes -// MSG_NOSIGNAL for exactly this reason. The peer is closed rather than merely idle, so a single -// byte is enough: the write fails immediately with EPIPE instead of being buffered. -TEST(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) +// The mask change and any pending SIGPIPE are cleaned up in TearDown, which runs even when a test +// aborts on a fatal assertion. That matters here: Socket is a non-owning wrapper with no +// destructor, and a leaked signal mask or a queued SIGPIPE would pollute later tests sharing this +// binary. +class SocketSigPipeTest : public ::testing::Test { - // Observe SIGPIPE through sigpending() rather than the default disposition, so the test neither - // depends on nor changes the process-wide handler and cannot be killed by a stray signal. - sigset_t blocked, previous; - sigemptyset(&blocked); - sigaddset(&blocked, SIGPIPE); - ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked, &previous), 0); +protected: + void SetUp() override + { + sigemptyset(&blocked_); + sigaddset(&blocked_, SIGPIPE); + // Observe SIGPIPE through sigpending() instead of the default disposition, so a stray signal + // cannot kill the runner and the test does not depend on the process-wide handler. + ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked_, &previous_), 0); + mask_changed_ = true; + EXPECT_FALSE(SigPipeIsPending()) << "a SIGPIPE was already pending before the test"; + } + + void TearDown() override + { + DrainPendingSigPipe(); + if (mask_changed_) + { + EXPECT_EQ(pthread_sigmask(SIG_SETMASK, &previous_, nullptr), 0); + } + } + + static bool SigPipeIsPending() + { + sigset_t pending; + sigemptyset(&pending); + EXPECT_EQ(sigpending(&pending), 0); + return sigismember(&pending, SIGPIPE) == 1; + } + + void DrainPendingSigPipe() + { + if (SigPipeIsPending()) + { + int signo = 0; + EXPECT_EQ(sigwait(&blocked_, &signo), 0); + EXPECT_EQ(signo, SIGPIPE); + } + } + + sigset_t blocked_{}; + sigset_t previous_{}; + bool mask_changed_{false}; +}; + +// Writing to a socket whose peer has closed must report EPIPE without raising SIGPIPE, whose +// default disposition would terminate the host process. A closed peer fails the write immediately, +// so one byte is enough. A negative control on a raw socket first proves this thread really does +// observe a SIGPIPE, so the suppressed result is meaningful rather than vacuous. +TEST_F(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) +{ +# if !defined(MSG_NOSIGNAL) && !defined(SO_NOSIGPIPE) + GTEST_SKIP() + << "no SIGPIPE-suppression mechanism (MSG_NOSIGNAL or SO_NOSIGPIPE) on this platform"; +# else + const char byte = 'x'; + // Negative control: a raw send() with no suppression to a closed peer raises SIGPIPE. + { + int raw[2]; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, raw), 0); + ::close(raw[1]); + errno = 0; + const ssize_t r = ::send(raw[0], &byte, 1, 0); + EXPECT_EQ(r, -1); + EXPECT_EQ(errno, EPIPE); + EXPECT_TRUE(SigPipeIsPending()) << "the environment does not deliver SIGPIPE, so the " + "suppressed case below would pass vacuously"; + DrainPendingSigPipe(); + ::close(raw[0]); + } + + // The wrapper suppresses it: EPIPE, and no SIGPIPE queued. int fds[2]; ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); SocketTools::Socket sender(fds[0]); - // Production sockets get SO_NOSIGPIPE from accept()/the (af,type,proto) ctor. Apply it here too: - // on macOS and the BSDs, where MSG_NOSIGNAL is not defined, that option is the layer under test, - // and a raw-fd socket without it would raise SIGPIPE and fail the pending-signal check below. + // Production sockets get SO_NOSIGPIPE from accept()/the (af,type,proto) ctor. Apply it here too, + // since on macOS and the BSDs, where MSG_NOSIGNAL is not defined, that option is the layer under + // test. sender.suppressSigPipe(); - ::close(fds[1]); // the peer is gone, so any further write is a broken pipe + ::close(fds[1]); - const char byte = 'x'; errno = 0; const int sent = sender.send(&byte, 1); const int saved = errno; - EXPECT_EQ(sent, -1); EXPECT_EQ(saved, EPIPE); - - sigset_t pending; - sigemptyset(&pending); - ASSERT_EQ(sigpending(&pending), 0); - EXPECT_FALSE(sigismember(&pending, SIGPIPE)); - - // If the fix regressed and a SIGPIPE was queued, drain it so it cannot escape the test. - if (sigismember(&pending, SIGPIPE)) - { - int signo = 0; - sigwait(&blocked, &signo); - } + EXPECT_FALSE(SigPipeIsPending()); sender.close(); - pthread_sigmask(SIG_SETMASK, &previous, nullptr); +# endif } # ifdef SO_NOSIGPIPE -// Where the platform offers SO_NOSIGPIPE (macOS and the BSDs), a socket created through the wrapper -// should carry it as the best-effort second layer described on suppressSigPipe(). -TEST(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) +// Where the platform offers SO_NOSIGPIPE (macOS, the BSDs), the (af,type,proto) constructor sets +// it. +TEST_F(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) { SocketTools::Socket sock(AF_INET, SOCK_STREAM, 0); ASSERT_FALSE(sock.invalid()); @@ -79,8 +132,51 @@ TEST(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) sock.close(); } + +// accept() must set SO_NOSIGPIPE on the socket it returns. The listener is wrapped from a raw fd +// (the Socket(Type) constructor does not call the helper) and the test drives the wrapper's own +// accept(), so a pass proves accept()'s suppressSigPipe() call rather than option inheritance. +TEST_F(SocketSigPipeTest, AcceptedSocketCarriesSoNoSigPipe) +{ + SocketTools::Socket listener(::socket(AF_INET, SOCK_STREAM, 0)); + ASSERT_FALSE(listener.invalid()); + listener.setReuseAddr(); + + SocketTools::SocketAddr bind_addr(SocketTools::SocketAddr::Loopback, 0); // 127.0.0.1, ephemeral + ASSERT_TRUE(listener.bind(bind_addr)); + ASSERT_TRUE(listener.listen(1)); + + SocketTools::SocketAddr bound; + ASSERT_TRUE(listener.getsockname(bound)); + + SocketTools::Socket client(AF_INET, SOCK_STREAM, 0); + ASSERT_FALSE(client.invalid()); + SocketTools::SocketAddr server_addr(SocketTools::SocketAddr::Loopback, bound.port()); + // Blocking connect on loopback completes into the listen backlog before accept() runs. + ASSERT_TRUE(client.connect(server_addr)); + + SocketTools::Socket accepted; + SocketTools::SocketAddr peer; + ASSERT_TRUE(listener.accept(accepted, peer)); // accept() calls suppressSigPipe() on the result + + int value = 0; + socklen_t length = sizeof(value); + ASSERT_EQ(::getsockopt(accepted, SOL_SOCKET, SO_NOSIGPIPE, &value, &length), 0); + EXPECT_EQ(value, 1); + + accepted.close(); + client.close(); + listener.close(); +} # endif // SO_NOSIGPIPE } // namespace +#else // _WIN32 + +TEST(SocketSigPipeTest, NotApplicableOnWindows) +{ + GTEST_SKIP() << "Windows does not generate POSIX SIGPIPE"; +} + #endif // _WIN32 From 68e90a5e237b0c162b1f07d76d3c64bd60834686 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 25 Jul 2026 23:43:26 +0800 Subject: [PATCH 07/10] Fix CI: IWYU includes, gtest_add_tests, always-compile platform tests Add and ; keep with an IWYU keep pragma because pthread_sigmask needs it on macOS/BSD. Switch from gtest_discover_tests to gtest_add_tests, the convention used elsewhere in this tree; gtest_discover_tests broke the CMake 3.16 minimum-version and FetchContent jobs. Compile the SO_NOSIGPIPE and Windows tests on every platform and skip at runtime, so gtest_add_tests registers only real tests rather than phantom entries. Test hygiene: fatal precondition assert, TearDown returns early when the mask was not changed, save errno in the negative control, drop the unnecessary setReuseAddr. Correct comments that claimed MSG_NOSIGNAL covers every platform (Windows has none). --- .../ext/http/server/socket_tools.h | 10 ++-- ext/test/http/CMakeLists.txt | 16 +++--- ext/test/http/socket_sigpipe_test.cc | 52 +++++++++++++------ 3 files changed, 50 insertions(+), 28 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index d4aa2f990..3ef63abb6 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -347,9 +347,9 @@ struct Socket /** * Stop writes to a socket whose peer has gone away from raising SIGPIPE, whose default - * disposition would terminate the host process. The guarantee is MSG_NOSIGNAL, which send() - * passes per call on every platform that defines it, which is modern Linux, macOS and the BSDs, - * and every platform this repo builds on. Where the platform also offers SO_NOSIGPIPE this + * disposition would terminate the host process. The primary guarantee is MSG_NOSIGNAL, which + * send() passes per call on every platform that defines it. Where the platform also offers + * SO_NOSIGPIPE (macOS, the BSDs) this * additionally sets it at the socket level as a best-effort second layer, covering a write to * the descriptor that does not go through send(). Windows has no SIGPIPE. Done per socket rather * than by changing the process signal disposition, which belongs to the embedding program rather @@ -364,8 +364,8 @@ struct Socket if (::setsockopt(m_sock, SOL_SOCKET, SO_NOSIGPIPE, reinterpret_cast(&value), sizeof(value)) != 0) { - // Best effort: MSG_NOSIGNAL already guards send() on every platform this repo builds on, - // so a failure here is logged where a logger is configured rather than failing the socket. + // Best effort: MSG_NOSIGNAL already guards send() wherever it is defined, so a failure here + // is logged where a logger is configured rather than failing the socket. // This wrapper does not own the descriptor's lifetime and does not promise to protect a // platform whose only defence would be SO_NOSIGPIPE. LOG_WARN("Socket: cannot set SO_NOSIGPIPE (errno %d)", errno); diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index 35afe4e1a..ac9143932 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -15,19 +15,21 @@ if(WITH_HTTP_CLIENT_CURL) endif() # The SIGPIPE suppression is POSIX-only (Windows has no such signal), so the -# CMake target is built only on POSIX; the Bazel target covers Windows with an -# explicit skipped test. gtest_discover_tests enumerates the compiled binary -# rather than parsing the source, so the SO_NOSIGPIPE-only tests are registered -# only where they are actually compiled, never as phantom tests on a platform -# that compiled them out. +# CMake target is built only on POSIX; the Bazel target covers Windows. Every +# test case compiles on every platform and skips at runtime where its mechanism +# is absent, so gtest_add_tests (which parses the source, like the rest of this +# tree) registers only tests that exist, and it works on the project's minimum +# CMake, where gtest_discover_tests does not. if(NOT WIN32) set(SOCKET_SIGPIPE_FILENAME socket_sigpipe_test) add_executable(${SOCKET_SIGPIPE_FILENAME} ${SOCKET_SIGPIPE_FILENAME}.cc) target_link_libraries( ${SOCKET_SIGPIPE_FILENAME} opentelemetry_ext ${GMOCK_LIB} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT}) - gtest_discover_tests(${SOCKET_SIGPIPE_FILENAME} - TEST_PREFIX ext.http.socketsigpipe.) + gtest_add_tests( + TARGET ${SOCKET_SIGPIPE_FILENAME} + TEST_PREFIX ext.http.socketsigpipe. + TEST_LIST ${SOCKET_SIGPIPE_FILENAME}) endif() set(URL_PARSER_FILENAME url_parser_test) diff --git a/ext/test/http/socket_sigpipe_test.cc b/ext/test/http/socket_sigpipe_test.cc index 26a903664..05ca326a9 100644 --- a/ext/test/http/socket_sigpipe_test.cc +++ b/ext/test/http/socket_sigpipe_test.cc @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 #include +#include #include "opentelemetry/ext/http/server/socket_tools.h" @@ -11,9 +12,10 @@ #ifndef _WIN32 # include -# include +# include // IWYU pragma: keep (pthread_sigmask lives here on macOS/BSD) # include # include +# include # include namespace @@ -34,16 +36,20 @@ class SocketSigPipeTest : public ::testing::Test // cannot kill the runner and the test does not depend on the process-wide handler. ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked_, &previous_), 0); mask_changed_ = true; - EXPECT_FALSE(SigPipeIsPending()) << "a SIGPIPE was already pending before the test"; + // A SIGPIPE pending before the test would make the negative control and suppressed case below + // impossible to attribute, so treat it as a fatal precondition rather than an expectation. + ASSERT_FALSE(SigPipeIsPending()) << "a SIGPIPE was already pending before the test"; } void TearDown() override { - DrainPendingSigPipe(); - if (mask_changed_) + if (!mask_changed_) { - EXPECT_EQ(pthread_sigmask(SIG_SETMASK, &previous_, nullptr), 0); + return; // SetUp never blocked SIGPIPE, so there is nothing to drain or restore, and + // sigwait() on an unblocked signal would be unsafe. } + DrainPendingSigPipe(); + EXPECT_EQ(pthread_sigmask(SIG_SETMASK, &previous_, nullptr), 0); } static bool SigPipeIsPending() @@ -88,8 +94,9 @@ TEST_F(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) ::close(raw[1]); errno = 0; const ssize_t r = ::send(raw[0], &byte, 1, 0); + const int saved = errno; // save before any assertion can perturb errno EXPECT_EQ(r, -1); - EXPECT_EQ(errno, EPIPE); + EXPECT_EQ(saved, EPIPE); EXPECT_TRUE(SigPipeIsPending()) << "the environment does not deliver SIGPIPE, so the " "suppressed case below would pass vacuously"; DrainPendingSigPipe(); @@ -100,8 +107,8 @@ TEST_F(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) int fds[2]; ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); SocketTools::Socket sender(fds[0]); - // Production sockets get SO_NOSIGPIPE from accept()/the (af,type,proto) ctor. Apply it here too, - // since on macOS and the BSDs, where MSG_NOSIGNAL is not defined, that option is the layer under + // Production sockets get SO_NOSIGPIPE from accept()/the (af,type,proto) ctor. Apply it here too: + // where MSG_NOSIGNAL is unavailable (some macOS and BSD builds) SO_NOSIGPIPE is the layer under // test. sender.suppressSigPipe(); ::close(fds[1]); @@ -117,11 +124,15 @@ TEST_F(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) # endif } -# ifdef SO_NOSIGPIPE // Where the platform offers SO_NOSIGPIPE (macOS, the BSDs), the (af,type,proto) constructor sets -// it. +// it. Where it does not (Linux, which suppresses via MSG_NOSIGNAL instead) the test skips at +// runtime, so it stays a real registered test on every POSIX platform rather than being compiled +// out. TEST_F(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) { +# ifndef SO_NOSIGPIPE + GTEST_SKIP() << "SO_NOSIGPIPE is not available on this platform"; +# else SocketTools::Socket sock(AF_INET, SOCK_STREAM, 0); ASSERT_FALSE(sock.invalid()); @@ -131,6 +142,7 @@ TEST_F(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) EXPECT_EQ(value, 1); sock.close(); +# endif } // accept() must set SO_NOSIGPIPE on the socket it returns. The listener is wrapped from a raw fd @@ -138,9 +150,11 @@ TEST_F(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) // accept(), so a pass proves accept()'s suppressSigPipe() call rather than option inheritance. TEST_F(SocketSigPipeTest, AcceptedSocketCarriesSoNoSigPipe) { +# ifndef SO_NOSIGPIPE + GTEST_SKIP() << "SO_NOSIGPIPE is not available on this platform"; +# else SocketTools::Socket listener(::socket(AF_INET, SOCK_STREAM, 0)); ASSERT_FALSE(listener.invalid()); - listener.setReuseAddr(); SocketTools::SocketAddr bind_addr(SocketTools::SocketAddr::Loopback, 0); // 127.0.0.1, ephemeral ASSERT_TRUE(listener.bind(bind_addr)); @@ -167,16 +181,22 @@ TEST_F(SocketSigPipeTest, AcceptedSocketCarriesSoNoSigPipe) accepted.close(); client.close(); listener.close(); +# endif } -# endif // SO_NOSIGPIPE } // namespace -#else // _WIN32 +#endif // _WIN32 -TEST(SocketSigPipeTest, NotApplicableOnWindows) +// Windows has no POSIX SIGPIPE, so the fixture and its tests above are POSIX-only. This placeholder +// keeps the Windows test target non-empty and records why; on POSIX it is a no-op alongside the +// real tests. Compiling it on every platform means gtest_add_tests registers only tests that +// exist, never a phantom for a branch that was compiled out. +TEST(SocketSigPipe, NotApplicableOnWindows) { +#ifdef _WIN32 GTEST_SKIP() << "Windows does not generate POSIX SIGPIPE"; +#else + GTEST_SKIP() << "POSIX SIGPIPE suppression is covered by the SocketSigPipeTest fixture"; +#endif } - -#endif // _WIN32 From 34ef516fa0896ed45e36d4fa69bd1b75a14ef9e1 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 00:58:46 +0800 Subject: [PATCH 08/10] Strengthen SIGPIPE tests; fix CMake comment and errno accuracy Split the closed-peer test into a negative control, an MSG_NOSIGNAL-only case (wrapper send, no suppressSigPipe), and an SO_NOSIGPIPE-only case (raw send bypassing MSG_NOSIGNAL), so each mechanism is regression-tested independently. Add a ScopedFd and fixture-member sockets closed in TearDown so a fatal assertion cannot leak descriptors; the accept test clears the listener SO_NOSIGPIPE to 0 first so a value of 1 on the accepted socket proves accept()'s own suppressSigPipe(); skip the negative control when SIGPIPE is SIG_IGN process-wide. Rewrite the CMakeLists comment to describe the current build only, save errno before LOG_WARN in suppressSigPipe(), and correct the pthread.h include comment. --- .../ext/http/server/socket_tools.h | 4 +- ext/test/http/CMakeLists.txt | 11 +- ext/test/http/socket_sigpipe_test.cc | 223 ++++++++++++------ 3 files changed, 159 insertions(+), 79 deletions(-) diff --git a/ext/include/opentelemetry/ext/http/server/socket_tools.h b/ext/include/opentelemetry/ext/http/server/socket_tools.h index 3ef63abb6..c3e469567 100644 --- a/ext/include/opentelemetry/ext/http/server/socket_tools.h +++ b/ext/include/opentelemetry/ext/http/server/socket_tools.h @@ -368,7 +368,9 @@ struct Socket // is logged where a logger is configured rather than failing the socket. // This wrapper does not own the descriptor's lifetime and does not promise to protect a // platform whose only defence would be SO_NOSIGPIPE. - LOG_WARN("Socket: cannot set SO_NOSIGPIPE (errno %d)", errno); + const int saved_errno = errno; // capture before LOG_WARN, which may clobber errno + LOG_WARN("Socket: cannot set SO_NOSIGPIPE (errno %d)", saved_errno); + static_cast(saved_errno); // LOG_WARN can compile to a no-op } } #endif diff --git a/ext/test/http/CMakeLists.txt b/ext/test/http/CMakeLists.txt index ac9143932..2294bcf74 100644 --- a/ext/test/http/CMakeLists.txt +++ b/ext/test/http/CMakeLists.txt @@ -14,12 +14,11 @@ if(WITH_HTTP_CLIENT_CURL) TEST_LIST ${FILENAME}) endif() -# The SIGPIPE suppression is POSIX-only (Windows has no such signal), so the -# CMake target is built only on POSIX; the Bazel target covers Windows. Every -# test case compiles on every platform and skips at runtime where its mechanism -# is absent, so gtest_add_tests (which parses the source, like the rest of this -# tree) registers only tests that exist, and it works on the project's minimum -# CMake, where gtest_discover_tests does not. +# The CMake target is POSIX-only (Windows has no SIGPIPE); the Bazel target +# covers Windows. The platform-dependent tests are declared unconditionally in +# the POSIX source and skip at runtime where their mechanism is unavailable, so +# gtest_add_tests, which scans the source, registers only tests the POSIX binary +# actually contains. if(NOT WIN32) set(SOCKET_SIGPIPE_FILENAME socket_sigpipe_test) add_executable(${SOCKET_SIGPIPE_FILENAME} ${SOCKET_SIGPIPE_FILENAME}.cc) diff --git a/ext/test/http/socket_sigpipe_test.cc b/ext/test/http/socket_sigpipe_test.cc index 05ca326a9..38c086555 100644 --- a/ext/test/http/socket_sigpipe_test.cc +++ b/ext/test/http/socket_sigpipe_test.cc @@ -12,7 +12,7 @@ #ifndef _WIN32 # include -# include // IWYU pragma: keep (pthread_sigmask lives here on macOS/BSD) +# include // IWYU pragma: keep (legacy BSD declarations may require it) # include # include # include @@ -21,10 +21,36 @@ namespace { -// The mask change and any pending SIGPIPE are cleaned up in TearDown, which runs even when a test -// aborts on a fatal assertion. That matters here: Socket is a non-owning wrapper with no -// destructor, and a leaked signal mask or a queued SIGPIPE would pollute later tests sharing this -// binary. +// Owns the two descriptors of a socketpair so they are closed on every path, including when a test +// aborts on a fatal assertion. A descriptor handed to a SocketTools::Socket is released from this +// owner by setting its slot to -1, which leaves the Socket as the sole owner. +struct ScopedFd +{ + int fds[2]{-1, -1}; + + ScopedFd() = default; + ScopedFd(const ScopedFd &) = delete; + ScopedFd(ScopedFd &&) = delete; + ScopedFd &operator=(const ScopedFd &) = delete; + ScopedFd &operator=(ScopedFd &&) = delete; + + ~ScopedFd() + { + if (fds[0] >= 0) + { + ::close(fds[0]); + } + if (fds[1] >= 0) + { + ::close(fds[1]); + } + } +}; + +// SocketTools::Socket is a non-owning wrapper with no destructor, so TearDown closes every socket a +// test may open and then restores the signal state. TearDown runs even when a test aborts on a +// fatal assertion, so a leaked descriptor, a leaked signal mask, or a queued SIGPIPE cannot pollute +// later tests sharing this binary. class SocketSigPipeTest : public ::testing::Test { protected: @@ -36,13 +62,20 @@ class SocketSigPipeTest : public ::testing::Test // cannot kill the runner and the test does not depend on the process-wide handler. ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked_, &previous_), 0); mask_changed_ = true; - // A SIGPIPE pending before the test would make the negative control and suppressed case below + // A SIGPIPE pending before the test would make the negative control and the suppressed cases // impossible to attribute, so treat it as a fatal precondition rather than an expectation. ASSERT_FALSE(SigPipeIsPending()) << "a SIGPIPE was already pending before the test"; } void TearDown() override { + // Sockets are always closed here because Socket keeps no ownership of the descriptor; the + // mask_changed_ guard below concerns only the signal state. + CloseIfValid(accepted_); + CloseIfValid(client_); + CloseIfValid(listener_); + CloseIfValid(socket_); + if (!mask_changed_) { return; // SetUp never blocked SIGPIPE, so there is nothing to drain or restore, and @@ -52,6 +85,14 @@ class SocketSigPipeTest : public ::testing::Test EXPECT_EQ(pthread_sigmask(SIG_SETMASK, &previous_, nullptr), 0); } + static void CloseIfValid(SocketTools::Socket &sock) + { + if (!sock.invalid()) + { + sock.close(); + } + } + static bool SigPipeIsPending() { sigset_t pending; @@ -73,114 +114,153 @@ class SocketSigPipeTest : public ::testing::Test sigset_t blocked_{}; sigset_t previous_{}; bool mask_changed_{false}; + SocketTools::Socket socket_; + SocketTools::Socket listener_; + SocketTools::Socket client_; + SocketTools::Socket accepted_; }; -// Writing to a socket whose peer has closed must report EPIPE without raising SIGPIPE, whose -// default disposition would terminate the host process. A closed peer fails the write immediately, -// so one byte is enough. A negative control on a raw socket first proves this thread really does -// observe a SIGPIPE, so the suppressed result is meaningful rather than vacuous. -TEST_F(SocketSigPipeTest, WriteToClosedPeerReportsEpipeWithoutRaisingSigPipe) +// Negative control: a raw send() with no suppression to a closed peer must raise SIGPIPE. This +// proves this thread really observes the signal, so a suppressed result in the tests below is +// meaningful rather than vacuous. +TEST_F(SocketSigPipeTest, RawSendToClosedPeerRaisesSigPipe) { -# if !defined(MSG_NOSIGNAL) && !defined(SO_NOSIGPIPE) - GTEST_SKIP() - << "no SIGPIPE-suppression mechanism (MSG_NOSIGNAL or SO_NOSIGPIPE) on this platform"; -# else - const char byte = 'x'; - - // Negative control: a raw send() with no suppression to a closed peer raises SIGPIPE. + // When SIGPIPE's disposition is SIG_IGN, a broken-pipe write cannot make the signal pending, so + // this control cannot demonstrate delivery; skip it rather than report a false failure. + struct sigaction current = {}; + ASSERT_EQ(::sigaction(SIGPIPE, nullptr, ¤t), 0); + if (current.sa_handler == SIG_IGN) { - int raw[2]; - ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, raw), 0); - ::close(raw[1]); - errno = 0; - const ssize_t r = ::send(raw[0], &byte, 1, 0); - const int saved = errno; // save before any assertion can perturb errno - EXPECT_EQ(r, -1); - EXPECT_EQ(saved, EPIPE); - EXPECT_TRUE(SigPipeIsPending()) << "the environment does not deliver SIGPIPE, so the " - "suppressed case below would pass vacuously"; - DrainPendingSigPipe(); - ::close(raw[0]); + GTEST_SKIP() << "SIGPIPE is ignored process-wide; a broken-pipe write cannot make it pending"; } - // The wrapper suppresses it: EPIPE, and no SIGPIPE queued. - int fds[2]; - ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, fds), 0); - SocketTools::Socket sender(fds[0]); - // Production sockets get SO_NOSIGPIPE from accept()/the (af,type,proto) ctor. Apply it here too: - // where MSG_NOSIGNAL is unavailable (some macOS and BSD builds) SO_NOSIGPIPE is the layer under - // test. - sender.suppressSigPipe(); - ::close(fds[1]); + ScopedFd pair; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair.fds), 0); + ::close(pair.fds[1]); + pair.fds[1] = -1; + + const char byte = 'x'; + errno = 0; + const ssize_t result = ::send(pair.fds[0], &byte, 1, 0); + const int saved = errno; // save before any assertion can perturb errno + EXPECT_EQ(result, -1); + EXPECT_EQ(saved, EPIPE); + EXPECT_TRUE(SigPipeIsPending()) << "the environment did not deliver SIGPIPE, so the suppression " + "tests would pass vacuously"; + DrainPendingSigPipe(); +} + +// The wrapper's send() passes MSG_NOSIGNAL wherever the platform defines it, so a write to a closed +// peer reports EPIPE without raising SIGPIPE. The socket is wrapped from a raw fd and +// suppressSigPipe() is not called, so MSG_NOSIGNAL is the only mechanism exercised. +TEST_F(SocketSigPipeTest, WrapperSendToClosedPeerUsesMsgNoSignal) +{ +# ifndef MSG_NOSIGNAL + GTEST_SKIP() << "MSG_NOSIGNAL is not available on this platform"; +# else + ScopedFd pair; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair.fds), 0); + socket_ = SocketTools::Socket(pair.fds[0]); + pair.fds[0] = -1; // ownership handed to socket_, which TearDown closes + ::close(pair.fds[1]); + pair.fds[1] = -1; + const char byte = 'x'; errno = 0; - const int sent = sender.send(&byte, 1); - const int saved = errno; + const int sent = socket_.send(&byte, 1); + const int saved = errno; // save before any assertion can perturb errno EXPECT_EQ(sent, -1); EXPECT_EQ(saved, EPIPE); EXPECT_FALSE(SigPipeIsPending()); +# endif +} - sender.close(); +// Where the platform offers SO_NOSIGPIPE (macOS, the BSDs), suppressSigPipe() sets it at the socket +// level, guarding even a write that bypasses the wrapper's per-call MSG_NOSIGNAL. A raw ::send() +// with flags 0 exercises exactly that path: EPIPE, and no SIGPIPE queued. +TEST_F(SocketSigPipeTest, SuppressSigPipeGuardsRawSendToClosedPeer) +{ +# ifndef SO_NOSIGPIPE + GTEST_SKIP() << "SO_NOSIGPIPE is not available on this platform"; +# else + ScopedFd pair; + ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair.fds), 0); + const int fd = pair.fds[0]; + socket_ = SocketTools::Socket(fd); + pair.fds[0] = -1; // ownership handed to socket_, which TearDown closes + socket_.suppressSigPipe(); + ::close(pair.fds[1]); + pair.fds[1] = -1; + + const char byte = 'x'; + errno = 0; + const ssize_t result = ::send(fd, &byte, 1, 0); // raw send bypasses the wrapper's MSG_NOSIGNAL + const int saved = errno; // save before any assertion can perturb errno + EXPECT_EQ(result, -1); + EXPECT_EQ(saved, EPIPE); + EXPECT_FALSE(SigPipeIsPending()); # endif } -// Where the platform offers SO_NOSIGPIPE (macOS, the BSDs), the (af,type,proto) constructor sets -// it. Where it does not (Linux, which suppresses via MSG_NOSIGNAL instead) the test skips at -// runtime, so it stays a real registered test on every POSIX platform rather than being compiled -// out. +// Where the platform offers SO_NOSIGPIPE (macOS, the BSDs), the (af, type, proto) constructor sets +// it through suppressSigPipe(). Where it does not (Linux, which suppresses via MSG_NOSIGNAL) the +// test skips at runtime, so it stays a registered test on every POSIX platform. TEST_F(SocketSigPipeTest, CreatedSocketCarriesSoNoSigPipe) { # ifndef SO_NOSIGPIPE GTEST_SKIP() << "SO_NOSIGPIPE is not available on this platform"; # else - SocketTools::Socket sock(AF_INET, SOCK_STREAM, 0); - ASSERT_FALSE(sock.invalid()); + socket_ = SocketTools::Socket(AF_INET, SOCK_STREAM, 0); + ASSERT_FALSE(socket_.invalid()); int value = 0; socklen_t length = sizeof(value); - ASSERT_EQ(::getsockopt(sock, SOL_SOCKET, SO_NOSIGPIPE, &value, &length), 0); + ASSERT_EQ(::getsockopt(socket_, SOL_SOCKET, SO_NOSIGPIPE, &value, &length), 0); EXPECT_EQ(value, 1); - - sock.close(); # endif } // accept() must set SO_NOSIGPIPE on the socket it returns. The listener is wrapped from a raw fd -// (the Socket(Type) constructor does not call the helper) and the test drives the wrapper's own -// accept(), so a pass proves accept()'s suppressSigPipe() call rather than option inheritance. +// (the Socket(Type) constructor does not call the helper) and its own SO_NOSIGPIPE is cleared to 0 +// first, so the accepted socket reading back 1 proves accept()'s own suppressSigPipe() call rather +// than inheritance from the listener. TEST_F(SocketSigPipeTest, AcceptedSocketCarriesSoNoSigPipe) { # ifndef SO_NOSIGPIPE GTEST_SKIP() << "SO_NOSIGPIPE is not available on this platform"; # else - SocketTools::Socket listener(::socket(AF_INET, SOCK_STREAM, 0)); - ASSERT_FALSE(listener.invalid()); + listener_ = SocketTools::Socket(::socket(AF_INET, SOCK_STREAM, 0)); + ASSERT_FALSE(listener_.invalid()); + + // Clear SO_NOSIGPIPE on the listener and confirm it reads back 0, so a value of 1 on the accepted + // socket can only come from accept()'s own suppressSigPipe() call. + int listener_flag = 0; + ASSERT_EQ( + ::setsockopt(listener_, SOL_SOCKET, SO_NOSIGPIPE, &listener_flag, sizeof(listener_flag)), 0); + socklen_t listener_length = sizeof(listener_flag); + ASSERT_EQ(::getsockopt(listener_, SOL_SOCKET, SO_NOSIGPIPE, &listener_flag, &listener_length), 0); + ASSERT_EQ(listener_flag, 0); SocketTools::SocketAddr bind_addr(SocketTools::SocketAddr::Loopback, 0); // 127.0.0.1, ephemeral - ASSERT_TRUE(listener.bind(bind_addr)); - ASSERT_TRUE(listener.listen(1)); + ASSERT_TRUE(listener_.bind(bind_addr)); + ASSERT_TRUE(listener_.listen(1)); SocketTools::SocketAddr bound; - ASSERT_TRUE(listener.getsockname(bound)); + ASSERT_TRUE(listener_.getsockname(bound)); - SocketTools::Socket client(AF_INET, SOCK_STREAM, 0); - ASSERT_FALSE(client.invalid()); + client_ = SocketTools::Socket(AF_INET, SOCK_STREAM, 0); + ASSERT_FALSE(client_.invalid()); SocketTools::SocketAddr server_addr(SocketTools::SocketAddr::Loopback, bound.port()); // Blocking connect on loopback completes into the listen backlog before accept() runs. - ASSERT_TRUE(client.connect(server_addr)); + ASSERT_TRUE(client_.connect(server_addr)); - SocketTools::Socket accepted; SocketTools::SocketAddr peer; - ASSERT_TRUE(listener.accept(accepted, peer)); // accept() calls suppressSigPipe() on the result + ASSERT_TRUE(listener_.accept(accepted_, peer)); // accept() calls suppressSigPipe() on the result int value = 0; socklen_t length = sizeof(value); - ASSERT_EQ(::getsockopt(accepted, SOL_SOCKET, SO_NOSIGPIPE, &value, &length), 0); + ASSERT_EQ(::getsockopt(accepted_, SOL_SOCKET, SO_NOSIGPIPE, &value, &length), 0); EXPECT_EQ(value, 1); - - accepted.close(); - client.close(); - listener.close(); # endif } @@ -188,10 +268,9 @@ TEST_F(SocketSigPipeTest, AcceptedSocketCarriesSoNoSigPipe) #endif // _WIN32 -// Windows has no POSIX SIGPIPE, so the fixture and its tests above are POSIX-only. This placeholder -// keeps the Windows test target non-empty and records why; on POSIX it is a no-op alongside the -// real tests. Compiling it on every platform means gtest_add_tests registers only tests that -// exist, never a phantom for a branch that was compiled out. +// Windows has no POSIX SIGPIPE, so the fixture and its tests above are POSIX-only. This test is +// declared on every platform to keep the Windows target non-empty and to give gtest_add_tests, +// which scans the source, a test the binary contains everywhere. TEST(SocketSigPipe, NotApplicableOnWindows) { #ifdef _WIN32 From a80da53e91d5524ba3617f9731307df279577f3f Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 02:21:32 +0800 Subject: [PATCH 09/10] Force SIGPIPE to SIG_DFL in the SigPipe test fixture SetUp gtest_add_tests registers each GoogleTest case as its own CTest process, so a suppression test cannot rely on a separate negative-control case to establish SIGPIPE's disposition in the same process. SetUp now forces SIG_DFL (with the signal blocked), so an unsuppressed broken-pipe write leaves SIGPIPE pending and a regression is observable in every process; an inherited SIG_IGN would otherwise discard it silently. TearDown restores the previous disposition, and the negative control no longer needs its own SIG_IGN skip guard. --- ext/test/http/socket_sigpipe_test.cc | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/ext/test/http/socket_sigpipe_test.cc b/ext/test/http/socket_sigpipe_test.cc index 38c086555..9c89e977f 100644 --- a/ext/test/http/socket_sigpipe_test.cc +++ b/ext/test/http/socket_sigpipe_test.cc @@ -62,6 +62,14 @@ class SocketSigPipeTest : public ::testing::Test // cannot kill the runner and the test does not depend on the process-wide handler. ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked_, &previous_), 0); mask_changed_ = true; + // gtest_add_tests runs each case in its own process, so a suppression test cannot rely on the + // negative control (a separate process) to establish the disposition. Force SIG_DFL here: with + // SIGPIPE blocked, an unsuppressed broken-pipe write then leaves it pending and a regression is + // observable, whereas an inherited SIG_IGN would discard the signal and mask the failure. + struct sigaction default_action = {}; + default_action.sa_handler = SIG_DFL; + ASSERT_EQ(::sigaction(SIGPIPE, &default_action, &previous_action_), 0); + action_changed_ = true; // A SIGPIPE pending before the test would make the negative control and the suppressed cases // impossible to attribute, so treat it as a fatal precondition rather than an expectation. ASSERT_FALSE(SigPipeIsPending()) << "a SIGPIPE was already pending before the test"; @@ -83,6 +91,10 @@ class SocketSigPipeTest : public ::testing::Test } DrainPendingSigPipe(); EXPECT_EQ(pthread_sigmask(SIG_SETMASK, &previous_, nullptr), 0); + if (action_changed_) + { + EXPECT_EQ(::sigaction(SIGPIPE, &previous_action_, nullptr), 0); + } } static void CloseIfValid(SocketTools::Socket &sock) @@ -114,6 +126,8 @@ class SocketSigPipeTest : public ::testing::Test sigset_t blocked_{}; sigset_t previous_{}; bool mask_changed_{false}; + struct sigaction previous_action_ = {}; + bool action_changed_{false}; SocketTools::Socket socket_; SocketTools::Socket listener_; SocketTools::Socket client_; @@ -125,15 +139,8 @@ class SocketSigPipeTest : public ::testing::Test // meaningful rather than vacuous. TEST_F(SocketSigPipeTest, RawSendToClosedPeerRaisesSigPipe) { - // When SIGPIPE's disposition is SIG_IGN, a broken-pipe write cannot make the signal pending, so - // this control cannot demonstrate delivery; skip it rather than report a false failure. - struct sigaction current = {}; - ASSERT_EQ(::sigaction(SIGPIPE, nullptr, ¤t), 0); - if (current.sa_handler == SIG_IGN) - { - GTEST_SKIP() << "SIGPIPE is ignored process-wide; a broken-pipe write cannot make it pending"; - } - + // SetUp forced SIGPIPE to SIG_DFL and blocked it, so a broken-pipe write makes the signal pending + // here regardless of the disposition the launcher inherited. ScopedFd pair; ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair.fds), 0); ::close(pair.fds[1]); From c08a13198bfd7e82ebedafae307449438f8b446e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sun, 26 Jul 2026 02:43:04 +0800 Subject: [PATCH 10/10] Guard each SIGPIPE signal-behavior test against an inherited SIG_IGN Under gtest_add_tests each case runs in its own CTest process, so no case can rely on another to establish SIGPIPE's disposition. Each signal-behavior case now checks SIGPIPE itself and skips when it is SIG_IGN: an ignored signal is discarded before it can pend (a linked library such as gRPC may set it process-wide), so a broken-pipe write would prove nothing there. The baseline case is likewise independent rather than a precondition for the suppression cases. --- ext/test/http/socket_sigpipe_test.cc | 52 +++++++++++++++++----------- 1 file changed, 31 insertions(+), 21 deletions(-) diff --git a/ext/test/http/socket_sigpipe_test.cc b/ext/test/http/socket_sigpipe_test.cc index 9c89e977f..34cf4c284 100644 --- a/ext/test/http/socket_sigpipe_test.cc +++ b/ext/test/http/socket_sigpipe_test.cc @@ -62,16 +62,8 @@ class SocketSigPipeTest : public ::testing::Test // cannot kill the runner and the test does not depend on the process-wide handler. ASSERT_EQ(pthread_sigmask(SIG_BLOCK, &blocked_, &previous_), 0); mask_changed_ = true; - // gtest_add_tests runs each case in its own process, so a suppression test cannot rely on the - // negative control (a separate process) to establish the disposition. Force SIG_DFL here: with - // SIGPIPE blocked, an unsuppressed broken-pipe write then leaves it pending and a regression is - // observable, whereas an inherited SIG_IGN would discard the signal and mask the failure. - struct sigaction default_action = {}; - default_action.sa_handler = SIG_DFL; - ASSERT_EQ(::sigaction(SIGPIPE, &default_action, &previous_action_), 0); - action_changed_ = true; - // A SIGPIPE pending before the test would make the negative control and the suppressed cases - // impossible to attribute, so treat it as a fatal precondition rather than an expectation. + // A SIGPIPE pending before the test would make the signal-behavior cases impossible to + // attribute, so treat it as a fatal precondition rather than an expectation. ASSERT_FALSE(SigPipeIsPending()) << "a SIGPIPE was already pending before the test"; } @@ -91,10 +83,6 @@ class SocketSigPipeTest : public ::testing::Test } DrainPendingSigPipe(); EXPECT_EQ(pthread_sigmask(SIG_SETMASK, &previous_, nullptr), 0); - if (action_changed_) - { - EXPECT_EQ(::sigaction(SIGPIPE, &previous_action_, nullptr), 0); - } } static void CloseIfValid(SocketTools::Socket &sock) @@ -113,6 +101,17 @@ class SocketSigPipeTest : public ::testing::Test return sigismember(&pending, SIGPIPE) == 1; } + // Each case runs in its own process under gtest_add_tests, so a signal-behavior test must + // establish for itself that SIGPIPE can be observed. A linked library such as gRPC may set + // SIG_IGN process-wide, and an ignored signal is discarded before it can pend, so a broken-pipe + // write would prove nothing; the caller skips instead of passing vacuously. + static bool SigPipeIsIgnored() + { + struct sigaction current = {}; + EXPECT_EQ(::sigaction(SIGPIPE, nullptr, ¤t), 0); + return current.sa_handler == SIG_IGN; + } + void DrainPendingSigPipe() { if (SigPipeIsPending()) @@ -126,21 +125,22 @@ class SocketSigPipeTest : public ::testing::Test sigset_t blocked_{}; sigset_t previous_{}; bool mask_changed_{false}; - struct sigaction previous_action_ = {}; - bool action_changed_{false}; SocketTools::Socket socket_; SocketTools::Socket listener_; SocketTools::Socket client_; SocketTools::Socket accepted_; }; -// Negative control: a raw send() with no suppression to a closed peer must raise SIGPIPE. This -// proves this thread really observes the signal, so a suppressed result in the tests below is -// meaningful rather than vacuous. +// Baseline: a raw send() with no suppression to a closed peer must raise SIGPIPE. It runs in its +// own process and guards itself with SigPipeIsIgnored(), so it is an independent case rather than a +// precondition the suppression tests below rely on. TEST_F(SocketSigPipeTest, RawSendToClosedPeerRaisesSigPipe) { - // SetUp forced SIGPIPE to SIG_DFL and blocked it, so a broken-pipe write makes the signal pending - // here regardless of the disposition the launcher inherited. + if (SigPipeIsIgnored()) + { + GTEST_SKIP() << "SIGPIPE is ignored in this process; a broken-pipe write cannot be observed"; + } + ScopedFd pair; ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair.fds), 0); ::close(pair.fds[1]); @@ -165,6 +165,11 @@ TEST_F(SocketSigPipeTest, WrapperSendToClosedPeerUsesMsgNoSignal) # ifndef MSG_NOSIGNAL GTEST_SKIP() << "MSG_NOSIGNAL is not available on this platform"; # else + if (SigPipeIsIgnored()) + { + GTEST_SKIP() << "SIGPIPE is ignored in this process; a broken-pipe write cannot be observed"; + } + ScopedFd pair; ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair.fds), 0); socket_ = SocketTools::Socket(pair.fds[0]); @@ -190,6 +195,11 @@ TEST_F(SocketSigPipeTest, SuppressSigPipeGuardsRawSendToClosedPeer) # ifndef SO_NOSIGPIPE GTEST_SKIP() << "SO_NOSIGPIPE is not available on this platform"; # else + if (SigPipeIsIgnored()) + { + GTEST_SKIP() << "SIGPIPE is ignored in this process; a broken-pipe write cannot be observed"; + } + ScopedFd pair; ASSERT_EQ(::socketpair(AF_UNIX, SOCK_STREAM, 0, pair.fds), 0); const int fd = pair.fds[0];