[BUG] Stop embedded HTTP server writes from raising SIGPIPE#4294
Draft
thc1006 wants to merge 3 commits into
Draft
[BUG] Stop embedded HTTP server writes from raising SIGPIPE#4294thc1006 wants to merge 3 commits into
thc1006 wants to merge 3 commits into
Conversation
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>
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.31% 81.24% -0.06%
==========================================
Files 445 446 +1
Lines 18859 18875 +16
==========================================
Hits 15333 15333
- Misses 3526 3542 +16
🚀 New features to boost your workflow:
|
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>
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
Opened as a draft while CI runs. Ready for review from my side.
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, which also means fixing the fatal send error item in #4287 on its own would not have been enough.Checked through
SocketTools::Socket::send()itself, on asocketpairwhose peer end is closed, forking so the signal is observable:The fix
send()passesMSG_NOSIGNALon platforms that define it, which covers Linux.SO_NOSIGPIPEinstead, which is macOS and the BSDs, get the socket option set once per socket: in the(af, type, proto)constructor and on the socket handed back byaccept().The suppression is per socket on purpose. Installing a process-wide
SIGPIPEhandler or setting it to ignored would work too, but that is a decision belonging to the program embedding this server, not to a header it includes.On testing
The check above forks and inspects the wait status, which does not translate cleanly into a normal gtest assertion. The gtest form would be a death test, and those are restricted under some sanitizers and behave differently on Windows.
There is also no home for it yet: #4292 introduces
ext/test/http/socket_tools_test.cc, and adding the same file here would guarantee a conflict between the two. I would rather add this as a follow-up on that target once #4292 lands, and I am happy to do that. If you would prefer the test in this PR instead, say so and I will rebase this on top of #4292.Verification
.clang-tidyonsocket_tools.h: 2 warnings before and 2 after, both pre-existing.warning_limitis untouched.MSG_NOSIGNALandSO_NOSIGPIPEare both used behind#ifdef, so no platform sees an undefined identifier.