Skip to content

[BUG] Stop embedded HTTP server writes from raising SIGPIPE#4294

Draft
thc1006 wants to merge 3 commits into
open-telemetry:mainfrom
thc1006:bugfix/suppress-sigpipe-on-send
Draft

[BUG] Stop embedded HTTP server writes from raising SIGPIPE#4294
thc1006 wants to merge 3 commits into
open-telemetry:mainfrom
thc1006:bugfix/suppress-sigpipe-on-send

Conversation

@thc1006

@thc1006 thc1006 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

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 of 0, and there was no MSG_NOSIGNAL or SO_NOSIGPIPE anywhere under ext/. POSIX raises SIGPIPE on a write to a connected socket that can no longer transmit, in addition to failing with EPIPE, and its default disposition terminates the process. The embedded server could therefore be killed before any of the error handling in sendMore() 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 a socketpair whose peer end is closed, forking so the signal is observable:

=== before ===
  Socket::send -> KILLED by signal 13 (Broken pipe)

=== after ===
  Socket::send -> process exited 0

The fix

  • send() passes MSG_NOSIGNAL on platforms that define it, which covers Linux.
  • Platforms that offer SO_NOSIGPIPE instead, 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 by accept().
  • Windows needs neither and compiles both branches out.

The suppression is per socket on purpose. Installing a process-wide SIGPIPE handler 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-format 18 clean.
  • clang-tidy 18 with the repo .clang-tidy on socket_tools.h: 2 warnings before and 2 after, both pre-existing. warning_limit is untouched.
  • MSG_NOSIGNAL and SO_NOSIGPIPE are both used behind #ifdef, so no platform sees an undefined identifier.

thc1006 added 2 commits July 25, 2026 00:40
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>
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.24%. Comparing base (d3c7a47) to head (39ef641).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
...clude/opentelemetry/ext/http/server/socket_tools.h 94.12% <100.00%> (+0.12%) ⬆️

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Embedded HTTP server writes can raise SIGPIPE and terminate the process

1 participant