Skip to content

Sockets: add an optional deadline to ServerSocket::send() - #1826

Merged
Mysticial merged 4 commits into
mainfrom
audit/server-send-timeout
Nov 2, 2026
Merged

Mysticial merged 4 commits into
mainfrom
audit/server-send-timeout

Conversation

@pa-devbot

@pa-devbot pa-devbot commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to the TCP security audit of Common/Cpp/Sockets/: a client that connects but never reads fills the kernel send buffer, and the send() loops on every backend spin on EAGAIN forever, pinning the calling thread. On the Qt backend the undrained write also pins the server's event thread, which makes close_connection()/close() on that server block indefinitely as well.

This PR adds an optional deadline to the server socket:

virtual size_t send(const void* data, size_t bytes,
    std::chrono::milliseconds timeout = std::chrono::milliseconds::max()) = 0;
  • Default is ms::max() — the previous unbounded behavior; the server socket has no app callers yet, so no caller changes are needed.
  • On expiry the loop stops writing and returns the number of bytes actually written, leaving the connection untouched (the caller can retry, close, or keep the connection for reads).
  • No backend ever waits longer than before.

Per backend

  • POSIX / WinSocket: deadline check at the top of the existing send loop.
  • Qt (the non-trivial part): the deadline is carried into the queued write call, so the event-thread write loop self-terminates at the deadline — this is what un-pins the event thread so close_connection() (which runs on it) can proceed. The caller-side CV wait becomes timeout + 1 s (queueing margin); the previous flat 5 s safety wait is kept for the infinite default, covering close()/close_connection() destroying the server before the queued call runs.
  • The interface docs live once on AbstractServerSocket::send(); the default-argument re-declaration stays on each override (C++ default arguments are not inherited, and callers use the concrete type).

Known Qt backend caveat (pre-existing, not changed by this PR)

QTcpSocket::write() accepts the whole payload into Qt's userspace buffer and reports it as written, so kernel backpressure never reaches the Qt write loop. A stalled peer therefore shows up as "send completed" (data parked in RAM), not as a partial count. The bounded-caller guarantee still holds: send() always returns within its deadline.

Test

Test_Sockets_ServerSendTimeout (per backend): loopback pair where the client stalls reading; the server sends 256 MB with a 100 ms deadline; close_connection() must finish afterwards.

  • Large-buffer OSes: Windows CI buffered the entire 256 MB payload in its autotuned socket buffers against the stalled peer, so a send that "completes" is retried: those kernel buffers are now full, so a follow-up send cannot complete and must stop at the deadline (up to 4 times, i.e. unless the OS buffers over 1 GiB).
  • Starvation tolerance: the peer's stall lasts 12 s (longer than the 10 s send threshold) and is interruptible, so shutting the peer down does not wait for it; see the second commit for the full mechanism.

Also in this PR (second commit)

The loopback tests are now tolerant of thread starvation on loaded CI machines — this fixes the recurring macOS flake in the client test from #1825 (close() did not finish after the timed-out send (6025 ms)), which this branch carries:

  • timing_unreliable(): a 32 MB copy + 100 ms sleep that should take well under a second on a healthy core; more than 2 s means the calling thread is being starved and the test's timing assertions cannot be trusted.
  • Every socket test skips instead of failing when the machine is starved (up front, and on connect/listen failures); the deadline tests additionally skip at their threshold crossings, and the close check first round-trips a zero-length send through the socket's event thread (on Qt, the thread close() waits on), so a slow close with a slow control is the machine's fault, not a bug.
  • The close threshold is 10 s instead of 5 s, and all connect/listen/data waits share a 30 s budget instead of 5 s. The pre-fix failure mode is an infinite hang, so the thresholds only have to prove boundedness while tolerating a loaded runner.

Verification

  • pa-build clean; full pa-test command-line suite: 20,329 passed, 0 failed, 3 skipped (including all 4 deadline tests).
  • WinSocket backend compiles via CI (Windows).

Notes for reviewers

  • Sister PR Sockets: add an optional deadline to ClientSocket::send() #1825 (client) is merged; this branch is rebased onto it (squashed to the 2 commits above).
  • Out of scope (separate audit findings, separate PRs): the connect-phase lock held across the connect loop, listener-exception isolation, and the cross-backend send() semantic differences (Win partial-write return vs POSIX/Qt loop-to-completion; the Qt userspace buffering above).

A client that connects but never reads fills the kernel send buffer,
and the send() loops on all backends spin on EAGAIN forever, pinning
the calling thread (on the Qt backend the undrained write also pins
the server's event thread, which blocks close_connection()/close() on
it).

send() now takes a std::chrono::milliseconds deadline (default:
std::chrono::milliseconds::max(), i.e. the previous unbounded
behavior). On expiry it stops writing and returns the number of
bytes actually written, leaving the connection untouched.

- POSIX/Win backends: check the deadline at the top of the send loop.
- Qt backend: carry the deadline into the queued write call so the
  event-thread write loop self-terminates at the deadline, and wait
  for it on the caller side for the deadline plus a start-up margin
  (the previous flat 5 s safety is kept for the infinite default).
- The interface docs live once, on AbstractServerSocket::send(); the
  default argument re-declaration stays on each override, since C++
  default arguments are not inherited and callers use the concrete
  type.

Known Qt backend caveat (pre-existing): QTcpSocket::write() accepts
the whole payload into Qt's userspace buffer and reports it as
written, so kernel backpressure never reaches the Qt write loop and a
stalled peer shows up as "send completed" (data parked in RAM)
rather than a partial count. The bounded-caller guarantee still
holds: send() always returns within its deadline.

Test: per backend, the client stalls reading, and the server sends
256 MB with a 100 ms deadline. The deadline must engage (partial
count on POSIX/Win; on Qt only timing is asserted, per the caveat
above). Because an OS with unusually large autotuned socket buffers
(observed on Windows CI) can buffer the entire payload in the kernel
against a non-reading peer, a send that "completes" is retried:
those kernel buffers are now full, so a follow-up send cannot
complete and must stop at the deadline (up to 4 times, i.e. unless
the OS buffers over 1 GiB). The peer's 12 s stall (longer than the
10 s send threshold, which tolerates loaded CI runners - the pre-fix
behavior was an infinite hang) is interruptible, so shutting the
peer down does not wait for it. close_connection() must finish
promptly afterwards.
@pa-devbot
pa-devbot force-pushed the audit/server-send-timeout branch from 387300a to 66a176c Compare November 2, 2026 07:18
The parallel test runner can starve threads for long stretches on
loaded CI machines (observed: a 5 s close() threshold flaking at 6 s
on the macOS runner), where fixed wall-clock assertions then fail
tests that would pass on a healthy machine.

- timing_unreliable(): a small fixed workload (32 MB copy + 100 ms
  sleep) that should take well under a second on a healthy core; if
  it takes more than 2 s the calling thread is being starved and the
  caller's timing assertions cannot be trusted.
- Every socket test skips (instead of failing) when the machine is
  starved: up front, and on connect/listen-phase failures.
- The send/close deadline tests additionally skip at their threshold
  crossings, and the close check first round-trips a zero-length send
  through the socket's event thread: on the Qt backend that is the
  thread close() has to wait on, so a slow close() with a slow control
  is the machine's fault, not a bug.
- The close() threshold is 10 s instead of 5 s, and all
  connect/listen/data waits share a 30 s budget instead of 5 s: the
  pre-fix failure mode is an infinite hang, so the thresholds only
  have to prove boundedness while tolerating a loaded runner.

Also restores Sockets_Tests.cpp's CRLF line endings (matching the
rest of the Sockets/ directory); an intermediate tool pass had
silently rewritten the file to LF, which the "* -text"
.gitattributes surfaced as a whole-file diff.
@pa-devbot
pa-devbot force-pushed the audit/server-send-timeout branch from 66a176c to 029f693 Compare November 2, 2026 07:34
CI starvation is extreme, so the tests that expect to finish quickly
now wait 60 s before giving up:

- The shared connect/listen/data budget is 60 s instead of 30 s.
- The deadline tests' send and close thresholds are 60 s instead of
  10 s; the peer's stall (which must outlast the whole retry chain)
  is 70 s. It stays interruptible, and the stalling listeners now
  also set "stop" in their destructors, so an early return (skip)
  does not make the peer's teardown close() wait out the stall.
- The one test that deliberately waits a stall out (Qt completion
  lifetime) keeps the 5 s waits it was written around.

Also fixes the close-check's control round trip: it used a 100 ms
send timeout, but the Qt caller-side wait is capped at "timeout + 1
s", so the control could never take more than ~1.1 s and could never
expose a starved event thread. It now uses the infinite default
(canary: ~0 ms on a healthy event thread, the full 5 s internal
safety wait if it is starved) and flags >2 s.

Finally, the Qt backends' comment on the 5 s infinite-default wait
now states what it is: a limitation of this wrapper, not of Qt - if
close() destroys the event loop while a send is still queued, Qt
discards the pending event and the "done" flag is never set, so an
unbounded wait would hang the caller.
…ests

close_connection() on the Qt backend queues the close onto the
server's event thread and stops waiting after its internal 5 s
bound, so on a starved event thread (Mac CI) it returns with the
close still pending and the state still not LISTENING; the queued
close does run once the event thread gets to it. The tests that
assert LISTENING right after close_connection() now poll for it
within the shared 60 s budget first, and skip instead of failing
if it still has not applied and the machine is starved.

Two tests assert that state: the server send-timeout test (which
failed on Mac CI) and the cut-connection-during-send test, which
had the identical latent race.

Test-only change; the backends are untouched.
@Mysticial
Mysticial merged commit 404127f into main Nov 2, 2026
6 of 7 checks passed
@Mysticial
Mysticial deleted the audit/server-send-timeout branch November 2, 2026 09:03
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.

2 participants