Skip to content

Sockets: add an optional deadline to ClientSocket::send() - #1825

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

Mysticial merged 4 commits into
mainfrom
audit/client-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 malicious (or merely faulty) peer that accepts a connection 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 backends the undrained write also pins the socket's event thread, which makes close()/connect() on that socket block indefinitely as well.

This PR adds an optional deadline to the client 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, so existing callers (e.g. SysbotBase::write_data) are unchanged.
  • 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 (~5 lines each; Win also keeps the existing partial-write return on expiry).
  • Qt (the non-trivial part): the deadline is carried into the queued internal_send call, so the event-thread write loop self-terminates at the deadline — this is what un-pins the event thread so close() (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() destroying the socket before the queued call runs.

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 write loop on that backend. A stalled peer therefore shows up as "send completed" (data parked in RAM, delivered later if the peer ever reads), not as a partial count. That is also why the new Qt test only asserts timing and close behavior (see below). The bounded-caller guarantee still holds: send() always returns within its deadline.

Test

Test_Sockets_ClientSendTimeout (per backend): loopback pair where the server stalls reading on the first data for 4 s; the client sends 256 MB (far beyond any per-socket kernel autotuning cap) with a 100 ms deadline:

  • send() returns in under 1 s;
  • POSIX/Win: it returns a partial count (the stalled peer proves the deadline engaged, not a buffer-size coincidence);
  • close() finishes promptly and the socket is NOT_RUNNING.

Verification

  • pa-build clean; full pa-test command-line suite: 20,327 passed, 0 failed, 3 skipped (including the 2 new tests).
  • WinSocket backend compiles via CI (Windows); same 3-line loop change as POSIX.

Notes for reviewers

  • Sister PR for the server socket: Sockets: add an optional deadline to ServerSocket::send() #1826 (audit/server-send-timeout). Both add their test near the end of Sockets_Tests.cpp, so the one merged second may need a trivial conflict resolution there.
  • 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 client partial-write return vs POSIX/Qt loop-to-completion; the Qt userspace buffering above).

A peer that accepts the connection 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 backends the write
also never drains, pinning the socket's event thread, which blocks
close()/connect() 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 backends: 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).

Adds a loopback test per backend: the peer stalls reading, and a
256 MB send with a 100 ms deadline must give up on time (with a
partial count on POSIX/Win) and the socket must close afterwards.
On the Qt backend QTcpSocket::write() buffers the whole payload in
userspace and reports it as written, so only the timing and close
behavior are asserted there.

This addresses the "send pins a thread forever against a
non-reading peer" finding from the TCP security audit; it does not
cover the connect-phase lock or the listener-exception findings.
The identical comment was repeated on every backend override; the
interface contract now lives once, on AbstractClientSocket::send().
The default argument re-declaration stays on each override, since C++
default arguments are not inherited and callers use the concrete type.
The same 256 MB payload makes the test sensitive to memory and
scheduler pressure on shared CI runners: the send path's page
traffic (and the Qt backend's in-memory packet copy), or the calling
thread being descheduled mid-measurement, can take seconds. The
pre-fix behavior was an infinite hang, so the assertion only needs
to prove boundedness: 10 s for the send (100x the deadline) and 5 s
for the close check.
Windows CI buffered the entire 256 MB payload in its autotuned socket
buffers against the stalled peer, so the send legitimately completed
and the partial-count assertion failed. On that backend (POSIX/Win),
a completed send means the kernel buffers are now full, so a follow-up
send to the same stalled peer cannot complete and must stop at the
deadline with a partial count; the test retries until that happens
(up to 4 sends, i.e. unless the OS buffers over 1 GiB).

The peer's stall now lasts 12 s - longer than the 10 s send threshold
- so it is still engaged across the whole retry chain, and it is
interruptible: the test releases it before closing the peer, replacing
the blind 5 s wait-out.
@Mysticial
Mysticial merged commit 74ac7bb into main Nov 2, 2026
7 checks passed
@Mysticial
Mysticial deleted the audit/client-send-timeout branch November 2, 2026 06:28
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