[BUG] Stop embedded HTTP server writes from raising SIGPIPE#4294
Open
thc1006 wants to merge 13 commits into
Open
[BUG] Stop embedded HTTP server writes from raising SIGPIPE#4294thc1006 wants to merge 13 commits into
thc1006 wants to merge 13 commits into
Conversation
This was referenced Jul 24, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4294 +/- ##
==========================================
- Coverage 81.25% 81.24% -0.00%
==========================================
Files 446 446
Lines 18872 18875 +3
==========================================
+ Hits 15332 15333 +1
- Misses 3540 3542 +2
🚀 New features to boost your workflow:
|
thc1006
added a commit
to thc1006/opentelemetry-cpp
that referenced
this pull request
Jul 25, 2026
Review of open-telemetry#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 open-telemetry#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>
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 open-telemetry#4293 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Review of open-telemetry#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 open-telemetry#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>
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>
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>
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>
thc1006
force-pushed
the
bugfix/suppress-sigpipe-on-send
branch
from
July 25, 2026 10:16
9007980 to
9ce1b34
Compare
Add <sys/types.h> and <string>; keep <pthread.h> 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).
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.
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.
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.
…pe-on-send # Conflicts: # CHANGELOG.md
6 tasks
- ScopedFd comment no longer calls the non-owning SocketTools::Socket the 'sole owner'; it now says the wrapper is non-owning and TearDown closes it (same wording applied to the two inline handoff notes). - SigPipeIsIgnored() comment no longer overstates that an ignored signal is 'discarded before it can pend'; it now says a broken-pipe write cannot be relied on to make SIGPIPE pending when the disposition is SIG_IGN. Comment-only; no behavior change.
The three signal-behavior cases skipped with 'a broken-pipe write cannot be observed', which overstates the guarantee. POSIX does not require an ignored blocked signal to remain pending, so the accurate reason is that suppression cannot be distinguished portably in that environment. Message-only change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4293
Refs #4287
The problem
Socket::send()passed a flags argument of0, and there was noMSG_NOSIGNALorSO_NOSIGPIPEanywhere underext/. POSIX raisesSIGPIPEon a write to a connected socket that can no longer transmit, in addition to failing withEPIPE, and its default disposition terminates the process. The embedded server could therefore be killed before any of the error handling insendMore()ran.The fix
send()passesMSG_NOSIGNALon every platform that defines it, which is Linux, modern macOS and the BSDs. This repo's POSIX CI runs Linux and macOS.MSG_NOSIGNALis the guaranteed per-send protection wherever the platform defines it. Where the platform also offersSO_NOSIGPIPE(macOS, the BSDs), the option is set once per socket, in the(af, type, proto)constructor and on the socket handed back byaccept(), as a best-effort second layer: asetsockoptfailure is logged rather than failing the socket. This PR therefore does not make a hard guarantee for a platform whereSO_NOSIGPIPEwould be the only mechanism, since a failedsetsockoptdoes not invalidate the socket.SIGPIPEand compiles both branches out.This PR prevents process termination only. The fatal send-error connection cleanup (closing the socket, removing the reactor registration, changing state) is unchanged and remains tracked in #4287.
Testing
ext/test/http/socket_sigpipe_test.ccis a fixture-based test. Fixture-member sockets are closed inTearDown, while raw socketpair descriptors remain protected by a localScopedFdwhose destructor runs on every exit path, including a fatal assertion.TearDownthen drains any pendingSIGPIPEand restores the previous signal mask, so a leaked descriptor, signal mask, or queuedSIGPIPEcannot pollute other tests sharing the binary.gtest_add_testsregisters each case as its own CTest process, so no case can rely on another to set up the signal environment. Each signal-behavior case therefore guards itself and skips ifSIGPIPEisSIG_IGNin its own process, because POSIX does not require an ignored blocked signal to remain pending. In that environment the absence of a pendingSIGPIPEcannot portably prove that the socket-level suppression mechanism was responsible.RawSendToClosedPeerRaisesSigPipe) asserts that a rawsend(..., 0)to a closed peer does raiseSIGPIPE. This is an independent case, not a precondition for the others.WrapperSendToClosedPeerUsesMsgNoSignalcalls the wrappersend()on a raw-fd socket withoutsuppressSigPipe()(onlyMSG_NOSIGNALin play), andSuppressSigPipeGuardsRawSendToClosedPeercallssuppressSigPipe()then a rawsend(..., 0)that bypassesMSG_NOSIGNAL(onlySO_NOSIGPIPEin play). Each assertsEPIPEwith no pendingSIGPIPE.SO_NOSIGPIPEis defined, two tests read it back withgetsockopt(): one on a(af, type, proto)-constructed socket, and one on anaccept()ed socket whose listener has itsSO_NOSIGPIPEexplicitly cleared to 0 first, so a value of 1 provesaccept()'s ownsuppressSigPipe()rather than inheritance.SO_NOSIGPIPEcases skip at runtime where the option is absent (Linux), and Windows gets an explicit skipped test.The CMake target is POSIX-only and registered with
gtest_add_tests, the convention used elsewhere in this tree. EveryTEST_Fdeclaration is present in the POSIX binary (platform-dependent cases skip at runtime rather than being compiled out), so the source-parsinggtest_add_testsregisters only tests the binary actually contains. The Bazel target builds on all platforms; on Windows it compiles the single skipped test rather than an empty binary, and linksws2_32forsocket_tools.h'sWsaInitializerglobal.On #4292 and merge order
This PR uses a separate
socket_sigpipe_test.cc, so it does not depend on #4292's test source. A rebase may still be needed after either PR lands, since both touchCHANGELOG.md,ext/test/http/BUILDandext/test/http/CMakeLists.txt.Verification
MSG_NOSIGNALand negative-control cases run, theSO_NOSIGPIPEcases skip); theSO_NOSIGPIPEbranch compiles under-Wall -Wextra -Werrorin both C++14 and C++17..cmake-format.py) and buildifier are clean; GCC maintainer mode (-Wlogical-op -Werror) is clean.MSG_NOSIGNALandSO_NOSIGPIPEare both used behind#ifdef, so no platform sees an undefined identifier.