The check was spent on the first frame that decrypted, whatever it held.
A server tags the messages it chooses to, and a frame carrying no echo
reads as `echoed == 0`, which passes; on the way out it also disarmed the
check for good, so an attacker who rewrote the advertised version only had
to let one untagged frame through to be rid of the only thing that would
have caught it.
A zero now says nothing either way and leaves the check standing. Only a
non-zero echo settles the question, matching or refusing the stream.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab
Version 1 of the key exchange splits the exchanged key into a subkey per
direction, the way Noise splits its cipher states after the handshake, and
leaves the nonce layout and the handshake messages as they are. Each subkey
is a keyed BLAKE2b over the handshake transcript: both ephemeral public keys,
the version the responder advertised and the one the initiator picked, so a
change to any of it in transit leaves the two sides with different keys. The
original scheme is version 0, which is also what an absent field means.
Negotiated through new fields old peers do not see: the side that speaks
first advertises the newest version it has (`KeyExchange.version` from the
server, `IdPk.kx_version` inside the signed identity from the controlled
side), the other side answers with the highest both have, and the pair runs
that. Any old peer on either end keeps today's stream byte for byte. The
controlled side's advertisement sits inside the signature; the server's is
repeated in `RendezvousMessage.kx_advertised` on its first message after the
exchange, inside the encrypted channel, and a client that was shown a
different value in the clear refuses the stream.
`Encrypt::new` is unchanged; `new_split`, `set_key_split` and
`check_kx_advertised` are added beside it, and `box_pk_of` for a responder
that kept only the secret half of its ephemeral pair. The tests pin the
version 0 output as well as the split and the echo.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
Upstream reserves a frame's whole declared length the moment its header
is in, bounded only by max_frame_size, so a header alone buys the
allocation it names: 128 KiB before a session is authorized and 16 MiB
after. The rustdesk-org tungstenite-rs fork on rustdesk-patches-0.29,
upstream v0.29.0, grows the read buffer one read_buffer_size chunk at a
time as the bytes actually arrive instead, and tokio-tungstenite moves to
its rustdesk-patches-0.29 branch to match, still one commit for
set_config. 0.27's read chunking comes with the move, so a large frame no
longer clears its whole grown buffer on every read.
No source change: Bytes payloads and the non-exhaustive config arrived in
0.26 and 0.25 and were already in use here, and 0.28's boxing of
Error::Http derefs at its one use. The read buffer keeps tungstenite's
128 KiB default, this side reading video frames and file blocks over a
handful of connections rather than small messages over thousands. The
workspace root patches tungstenite to the fork, where cargo reads
[patch], so this crate's copy and tokio-tungstenite's resolve to one.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab
`new_direct_udp_for` is back on `test_target`. The punch reply the
controlled side sends with it goes to the address it picks, and that reply
lost is a device online and unreachable - so it keeps the handshake that
proves the address, as the registration does. The controller's NAT test,
once per connection, is what could not afford the proof, a round trip
right before the connection it then opens to the same host; it alone takes
the resolver's preferred address, as `new_direct_udp_for_unverified`, and
a wrong one there costs that connection its UDP punch and nothing more.
That lookup is the first on the controller's connection path and the one
not under CONNECT_TIMEOUT, so it gets a second: a resolver that hangs
would otherwise hold the connection for its whole retry schedule before
`connect_tcp` even starts, and what a slower lookup loses is, again, only
the NAT test.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
f688d41 replaced the TCP handshake in `test_target` with the resolver's
first answer for every caller. The two are the same when an IPv6 route is
missing, or leads to nothing: a v6-only host gets the synthesized address
first either way, and the handshake sat out its second on a black hole and
fell back to that same first answer. They differ when the server refuses
IPv6 outright - an AAAA published for a daemon bound to 0.0.0.0, a firewall
that rejects - where the handshake failed fast and moved on to the A
record, and the resolver's order stays on the AAAA.
That matters on one path. `new_udp_for` and `rebind_udp_for` choose the
address the rendezvous registration lives on, and `start_udp` never leaves
UDP for TCP nor asks for a second candidate: a rebind after `MAX_FAILS2`
calls the same function and gets the same address. A self-hosted server
with such an AAAA would never see the controlled side register. Those two
keep the handshake, one round trip per server connection, under the old
name and its old behaviour. `new_direct_udp_for`, per connection and
against servers whose records are ours, takes the resolver's answer as
`resolve_target`: the round trip it was spared stays spared.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
`test_target` opened a TCP connection to the rendezvous server to learn
which address the kernel would use, then dropped it: a full handshake, one
round trip, on every connection the controller starts, before the one
`_start_inner` opens to the same host a moment later - and a second's wait
first wherever an IPv6 route led to nothing, before it fell back to the
resolver anyway.
The handshake stood in for a choice that had been made by hand: before
2022's NAT64 fix the code preferred an A record outright, which a v6-only
host cannot reach, and asking the server itself was the way to let the OS
decide. The resolver decides the same way. getaddrinfo orders what it
returns as RFC 6724 says, looking up a route and a source address for each
candidate as it goes, so the synthesized IPv6 comes first on a v6-only host
and IPv6 on a dual-stack one, the order the handshake would have tried
them in. Its first answer is the address, and the fallback branch is all
that is left.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
The two WebSocket tests both stopped at max_frame_size: a header over the cap
and a single large frame after the cap is lifted. Dropping max_message_size
from set_max_packet_length left both passing, and that line is what refuses
two frames each under the cap that reassemble to a message over it - the other
way a peer buys the allocation. A test now sends exactly that, 12 KiB and a
12 KiB continuation against a 16 KiB cap, and fails when the line is dropped.
The doc on Stream::set_max_packet_length claimed that lowering the bound never
discards a message already being reassembled. That is the TCP codec, which
checks only on the header; WebRTC and a fragmented WebSocket message re-check
the bound on every fragment and end the message once it passes. The doc now
promises only what holds everywhere: set it before the first read, and do not
lean on lowering it to constrain a message in progress.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab
The cap's header-only refusal was tested on the TCP codec and its reassembly
bound on WebRTC, but not on WebSocket, which is the transport where a frame
header buys an allocation, and neither transport had a test that lifting the
cap lets a large message through again - the half of the lifecycle a server
runs at authorization, and the one a forgotten restore would break silently.
Two WebSocket tests build a loopback from a raw TcpStream with no handshake,
write server-to-client frames by hand, and check that a header declaring one
byte over the cap is refused before any payload exists, and that a message
refused under a low cap arrives once the cap is lifted. A WebRTC test does the
same lift. Each fails when the corresponding half of set_max_packet_length is
made a no-op.
The doc on Stream::set_max_packet_length now says the one thing the bound
does not do: lowering it applies from the next frame on and does not discard
one whose decoding or reassembly has already begun, so it has to be set before
the first read of untrusted data. That is how the server uses it; the note is
there so nothing later treats it as a switch that can be thrown at any time.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab
A frame header declares its own length, and until a connection authorizes nothing
bounded that length but the transport's ceiling: 1 GiB on TCP and on WebRTC, 64 MiB
on WebSocket. The decoder here already refuses to preallocate on a declared length,
so on TCP an attacker spends a byte sent per byte held; tungstenite does not, and
reserves a frame's whole declared payload the moment it passes max_frame_size, so a
fourteen-byte header buys 16 MiB. That is CVE-2021-32675's shape, and Redis answered
it the same way: a bound that holds only until the peer has authenticated.
`Stream::set_max_packet_length` carries such a bound to whichever transport is under
it. TCP has the knob already. WebRTC gains one: its reassembly ceiling moves from a
constant to a per-stream atomic, shared by every clone because the receive path runs
off one, and clamped to that constant so the send path's check still describes what a
peer will accept. WebSocket reaches tungstenite's own config, which needs a fork -
upstream exposes get_config and no set_config, at v0.26.2 and still at 0.30.0 - and
moves both bounds: max_frame_size is what stops a header from buying an allocation,
max_message_size is what bounds reassembly. Every transport clamps to its own
ceiling, so usize::MAX restores exactly what was there and no caller can raise one
above what it always had.
Nothing here calls it, so on its own this changes no behaviour.
Tests cover both new paths: the codec takes a header at the cap and dies on the one
a byte above it, before any payload is buffered; and a WebRTC loopback pair whose
receiver has lowered its bound refuses an oversize message, while the sender, whose
own bound is untouched, still puts it on the wire the way an unauthenticated peer
would.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab
The ICE crate marks a candidate pair failed after 7 unanswered binding
requests (~1.4s at the 200ms check interval) and stops sending on it.
Behind an address-restricted NAT those outgoing requests are also what
keep this side's mapping open to the remote's checks, so a browser peer
whose checks arrive late -- after the answerer has already given up --
never gets a response and the direct pair is lost while a relay carries
the session.
Set max_binding_requests to 74 (~14.8s, just inside libwebrtc's 15s
CONNECTION_WRITE_TIMEOUT) on the answerer only (remote_endpoint set), so
the controlled side keeps checking for about as long as a Chrome
controller does. The desktop offerer keeps the crate default as an
unchanged comparison point. Needs the SettingEngine knob added in
rustdesk-org/webrtc @ 80d5a205 (rustdesk-patches-0.13).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QcJZZeJ3Nqb2MHxXuUadkb
hbb_common is edition 2018, where a lone string literal is not treated as a
format string, so the capture printed itself and the loop over the two
terminal states could not say which one had failed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
`is_reusable_for` rejected a closed session but not one ICE currently reports
Disconnected. Nothing else does either - that state is deliberately not part of
the connection-state enum - so a caller could be given a session that already
carries the hint, and start off suspecting a peer it never used.
Also drops a race from the receive-progress test: it raced `next()` against a
sleeping sibling in a select, so a transfer that landed inside one poll left the
assertion looking false, and a slow enough one exhausted the loop and failed
with a misleading message. It now reads the counter after a `next_timeout` that
returned nothing, which is a point where the message is known to be incomplete.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
A peer that is killed, logged out or rebooted sends no reset over UDP, so a
session only learns of it by timing out. Nothing here acts on that; these are
the two observations a caller needs to.
`rx_progress` counts arriving fragments. A message reaches the caller only once
it is whole, so a large one - a clipboard image is the case that occurs - leaves
the receive loop with nothing to observe for as long as the transfer takes, and
a peer sending steadily looks exactly like one that died. A counter, not a
timestamp: `Instant::now()` costs an order of magnitude more per read, and
callers only compare successive samples.
`webrtc_disconnected` exposes the ICE state webrtc-rs already raises ~5s after
it stops hearing from the peer. It is deliberately not a fourth
`WebRTCConnectionState`: that enum gates send and receive readiness and session
cache admissibility, and a transient state must change neither.
TCP and WebSocket answer for neither, on purpose: this change leaves their
existing close, error and timeout behaviour exactly as it was.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
hbb_common is shared with the server, but `fs`, `platform`, `keyboard`,
`message.proto` and most of `config::keys` are reached only from the client.
Keeping them here means every change to them costs a submodule round-trip.
They move to the new `base` crate in the rustdesk tree. What stays is
what the server actually names: `rendezvous.proto`, the socket/stream layer, the
`Config` core, and the 32 option keys this crate reads itself. Those 32 are
re-exported from `base::config::keys`, so callers still see one set.
`IdPk` moves from message.proto to rendezvous.proto. It is register-pk handshake
business, the server is its only other user, and the field numbers are unchanged
so the wire format is identical.
`config::patch()` resolves a home directory through the shell lookup that lived
in `platform::linux`, so `find_cmd_path`, `CMD_*` and `run_cmds_trim_newline`
stay behind in a new `sh` module. `base::platform::linux` re-exports
them, leaving every existing `platform::linux::CMD_SH` path working.
backtrace, filetime, osascript, smithay-client-toolkit and winapi go with the
code that used them, so the server no longer builds a Wayland client toolkit and
a Windows API crate it never calls.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab
Racing WebRTC against UDP and TCP means one transport is cancelled on every
session that connects, and cancelling it walks agent_internal's candidate list
closing sockets that are already gone - one warn per candidate, plus the
peer_connection cascade behind it (ice cancelled by caller, SCTP without DTLS,
the SRTP sessions never opened). Trickle adds pingAllCandidates on every check
tick until the first pair arrives. None of it is a fault, and all of it lands
beside a connection that succeeded, where it reads as one: 90 of 386 lines in a
controlled-side log spent saying the design worked.
agent_gather stays at warn. A STUN server it could not reach is the one thing
upstream says that explains a session which never connected, and it is a
different module from the noise, so the two separate cleanly. Errors in the
quieted modules still print, and RUST_LOG overrides the whole spec for anyone
who wants the warns back.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M54JAqUK4RynudFou89hod
The local-candidate channel's only sender lived in the on_ice_candidate
handler until the pc reached a terminal state, so the forwarder loop a
caller writes on take_local_ice_rx sat parked on recv() for the whole
session. On the controlled side that loop holds its dedicated signaling
connection to hbbs, which hbbs closes after 90s idle, leaving a task and
a CLOSE_WAIT socket per session until the session ended. Gathering
complete is the agent's last word short of an ICE restart, which nothing
here does, so drop the sender there; the terminal-state handler's swap
stays for a pc that dies before it gets that far.
Reassembly handed the accumulated message over with mem::take, which
left the accumulator with no capacity and had every fragmented frame
regrow it from nothing, reallocating and recopying as it went. split()
hands over the same bytes and keeps the allocation, which reserve()
reclaims once the caller has dropped the message.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
Reads a `webrtc://` offer from stdin, answers it, bridges ICE candidates
as JSON lines over stdin/stdout and echoes every message. rustdesk-web-js
drives it from Chrome (js/e2e/webrtc) to check that the browser transport
and WebRTCStream still agree on the envelope, the signed fingerprint, the
candidate JSON and the fragment framing.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JXJJGEGdgu26wgCppvUXdZ
`Url` returns the username and password percent-encoded, and they went
into the ICE server configuration as-is, so a password containing `@`,
`/`, `?`, `#`, `%` or a space (written encoded, the only way the URL can
carry it) failed the long-term credential check with a 401 and TURN
never produced a relay candidate. Decode both, as the web client does.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JXJJGEGdgu26wgCppvUXdZ
The data channel's SCTP association runs RFC 4960's AIMD: a fast retransmit
halves cwnd, a T3 drops it to one MTU, and on a path whose loss is random
rather than congestion that pins the rate to MSS/(RTT*sqrt(p)) - about
1.3 Mbps at 70ms RTT and 1% loss - however idle the link is. KCP has always
shipped its turbo profile (nc=1) with no congestion window, which is why the
same link streams over KCP and crawls over WebRTC.
The sctp fork now carries a process-wide switch that bypasses the two places
gating sends on cwnd. Turn it on unless `allow-webrtc-congestion-control` is
set: the same opt-in `allow-kcp-congestion-control` gives KCP, for the reason
at `get_kcp_cc_enabled`. It is re-read on every new peer connection, so a
changed option reaches the next session without a restart.
Sender-side only: nothing is negotiated, and a browser or an older build on
the other end interoperates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
Taken from upstream #589. The eviction in the state handler re-derived the cache key
from `pc.local_description()` while holding SESSIONS - an await under the lock, on a
webrtc-rs internal mutex, in the one callback that runs inline from `pc.close()`. The
identity scan needs no description: it is synchronous, and it already was the fallback
for the key-derivation failure, so the two paths collapse into one. The duplicate-offer
guard survives unchanged, because a scan by `Arc::ptr_eq` cannot touch the winner
sharing the key.
The data-channel guard is released before `close()` rather than held across it, so a
concurrent `detach()` on the same stream cannot stall behind the peer's teardown.
Upstream's `cache_or_reuse_session` and its deadlock test are not carried: the insert
path here goes through `NewStreamHandoff` and never closes a pc under the lock, so the
function had no caller and the test exercised nothing this tree can do.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aokqJuhjvB3kijXtAg5Ns
Two of the three entries sat on one host, so they failed together - in the same millisecond,
on a peer whose route to that host was down. Spend the slots on separate networks instead:
two anycast, two unicast, and a :443 for the networks that pass no other UDP port. The note
about reading NAT type off two ports of one address goes with them - webrtc-ice queries each
URL from its own socket, so that comparison never held, and nothing consumes the result.
`stun_servers()` hands the STUN half out, so the IPv6 probe can stop keeping a second
hand-written copy and an operator's OPTION_ICE_SERVERS override reaches both paths.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
`option2bool` reads an `enable-` option as on unless it is literally "N", while this one's
accessor required a literal "Y" - the name promised default-on and the code shipped
default-off. `allow-` is the prefix whose rule matches the behaviour that already ships, so
the rename settles the contradiction without moving a single user's transport.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
The label a session is reported under names the transport that won the race, not the family
ICE ended up nominating, so a WebRTC session over IPv6 read the same as one over IPv4. Take
it from the remote side of the selected pair - the address the peer is actually reached at,
which the rendezvous-observed address the session is otherwise identified by cannot report.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
fe80::/10 can only be bound together with a scope id, which the gathered address list
drops before it reaches the bind, so every link-local address yields nothing but a failed
bind and a warning line - seven of them per session on a macOS host.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
An absent impl is invisible in the source, and this one keeps being
proposed. Closing on each clone's drop would end a live session the
moment a losing race future is dropped; closing on the last clone is
circular, since the cache entry is itself a clone and is removed by the
state handler a close fires. Ownership is carried by `OffererGuard` and
`Stream::WebRTC` instead, so say so where someone adding another holder
will look.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
`test_cancelled_new_does_not_leak_the_pc` cancelled `new()` with a zero
timeout and discarded whatever came back. The cancellation is not
guaranteed to win: the setup task runs on WEBRTC_RT, and it can finish
inside the single poll the timeout allows, in which case `new()` returns
a live stream. `WebRTCStream` has no `Drop`, so `let _ =` on that one
strands its pc in SESSIONS — and the test then reported the leak it had
just created, blaming the cancelled attempt.
Fewer test threads leave more CPU for that task, so it won the race
often enough that `--test-threads=2` failed every run while the default
count passed; the entry that survived carried `conn=New`,
`sig=HaveLocalOffer` and a `Pending` state watch, i.e. a pc nobody had
ever closed.
Close what a lost race hands back and retry for a real cancellation,
asserting that one happened rather than testing nothing. 24 tests now
pass at 1, 2, 4, 8 and default threads.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
`test_cancelled_new_does_not_leak_the_pc` demanded an instant at which
no key had appeared since its snapshot. That asks the whole suite to go
quiet, which `--test-threads=2` never grants: one lane is this test for
its entire 30s wait while the other keeps starting sessions, so the
difference is never empty and the test fails whatever the pc it is
actually watching did.
Intersect the difference across samples instead. A concurrent test's
session appears and is closed again, so it drops out; a leaked pc never
does. The cancelled attempt's own key cannot be named here — its
fingerprint is generated inside the task that was abandoned — so
outlasting the window is the property available to test, and it is the
one that means "leaked".
This sharpens what the failure says; it does not make `--test-threads=2`
pass. With the new assertion a single `offer:` key still survives all
30s there, and the test passes in 0.5s when run alone, so the entry
belongs to another test rather than to the cancelled `new()`. The two
that close only their answerer and leave the offerer to an indirect
path — `test_session_end_close_reaches_the_peer` (Stream::close_webrtc)
and `test_eof_close_then_drop_still_evicts` (Drop) — are where to look.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
`local_endpoint` replaces `get_local_endpoint_trickle`. The value is
taken at construction, so the getter could not fail and did not await —
but it kept an `async fn -> ResultType<String>` shape, leaving both call
sites maintaining error arms that can never run, one of them commented
as preventing a leak it can no longer prevent. Returning `&str` drops
the future, the Result, a copy of the envelope per read, and both arms.
`encode_endpoint` now takes the two fields a session description
actually serializes rather than the value. `trickle_endpoint` was
cloning a whole description — parsed SDP tree included — to overwrite
`.sdp`, which left `parsed` holding every candidate it had just
stripped: harmless only because `parsed` is `#[serde(skip)]` and the
value was serialized immediately. Building the envelope from
`(sdp_type, sdp)` removes both the clone and the trap. It is assembled
through an explicit `serde_json::Map` because `json!` expands its values
to `to_value(..).unwrap()`.
Tests: the `{keep}` in an `assert!` message was a literal, not a format
argument, under this crate's 2018 edition — it warned and would have
named no field; deserializing the stripped endpoint proved nothing
(`parsed` is skipped, so both fields are opaque strings) and now goes
through `get_key_for_sdp`, which unmarshals and requires the fingerprint
to have survived; the growth test compared the endpoint against itself,
which an immutable field makes unfalsifiable, and now compares it
against the live description that does grow; waiting out ICE gathering
is replaced by polling for the first candidate, and both tests close
before they assert so a failure here cannot strand a pc in SESSIONS and
be reported twice.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
`get_local_endpoint_trickle` read `pc.local_description()`, and webrtc-rs
runs `populate_local_candidates` there: it appends every candidate
gathered so far. The name promised a candidate-free endpoint, the value
grew with gathering, and callers read it a network round trip after
`new` — long enough on a multi-homed host for host and srflx candidates
to fill it in. The rendezvous server hands that blob to a UDP-registered
peer inside a single `PunchHole` datagram, so it fragmented and was
dropped without a trace on paths that discard fragments: no error, no
log, the punch simply never answered. A parallel offer-less TCP-punch
request had been covering for it, so what showed was "WebRTC never
wins", not "WebRTC is broken" — until that request went away with the
TCP punch switch and the connection failed outright.
Take the endpoint once, at construction, and store it. Encode it before
`set_local_description`, which is what starts gathering, so there is
nothing to strip; `trickle_endpoint` strips `a=candidate:` and
`a=end-of-candidates` anyway, making the bound a property of the value
instead of the call order. What is left is the session parameters the
peer needs to start ICE and DTLS — ufrag, pwd, fingerprint, setup, the
sctp m-line — a fixed 673 bytes, where the candidates that follow are
one small message each.
`get_local_endpoint` keeps its old contract: it still waits for
gathering and reads the live description, through the shared
`encode_endpoint`. `UDP_ENDPOINT_BUDGET` only warns — which leg carries
the endpoint is the server's to decide, and a TCP/WS route has no packet
ceiling, so refusing there would cost WebRTC for no reason.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019UzcMTdYTEv2QbMHcTSUy3
The client gains a switch for the TCP punch alongside the UDP, IPv6 and
WebRTC ones. Its "enable-" prefix gives it the usual default-on
semantics; unlike its siblings it is deliberately left out of the
self-hosted default-off list on the client side, since every hbbs has
always supported TCP punching.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016HV43uh1ztv6Wm5qi3Y1ne
A pc's UDP sockets register with the reactor, and its ICE/DTLS/SCTP
pumps spawn on the runtime, that is current while `new` builds it.
For a rustdesk controller that is io_loop's own
`#[tokio::main(flavor = "current_thread")]` runtime, dropped the
moment io_loop returns — so the pc outlived the only runtime able to
drive its I/O, and the session-end close, merely spawned there, was
never polled once. No DTLS close_notify left, and the peer waited out
ICE decay (~25-30s in its log) where TCP delivers a FIN at once.
Driving the close from somewhere else does not help: the sockets and
pumps are already gone, so close() returns having swallowed every
transport error without reaching the wire. Home the pc where it can
outlive its caller instead. `new` runs its body on WEBRTC_RT, a lazy
process-lifetime runtime, so every socket and background task belongs
to it; `close_detached_with` spawns each close there as its own task —
never cancelled, since a close cancelled after RTCPeerConnection::close
latches `is_closed` is unretryable and strands the pc in SESSIONS, and
never serialized, since close() has no deadline and one stalled pc
would otherwise block every later session's teardown. Data-plane
futures are still polled from caller runtimes; only the owning driver
has to stay alive.
A cancelled `new` needs the same care: rt.spawn keeps running when its
JoinHandle is dropped, so the setup task would finish, cache the
session, and hand its result to nobody — and an unanswered offerer
never reaches a terminal ICE state, so it sat in SESSIONS forever.
NewStreamHandoff closes an abandoned freshly-built stream, and hands a
cache hit back disarmed: that one is shared with a live caller whose
connection must survive this one's cancellation.
Tests cover the production shapes: a session-end close from a thread
with no runtime, an EOF-then-Drop double close, a close queued as the
creating runtime is destroyed, and the cancelled-new handoff both ways.
Two suite leaks surfaced on the way — streams dropped without close,
polluting SESSIONS for every later test — and are fixed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016HV43uh1ztv6Wm5qi3Y1ne
435 comment lines to 328, and the module header from 40 to 17. What went
is what the rules say does not belong in the source: past-bug narration,
rejected alternatives, measurements, and restatements of the adjacent
code. What stays is the why a reader cannot derive locally - the
single-reader invariant on the reassembly state, the send-permit handoff,
why the close must be detached and why that lock is held across an await,
and the webrtc-rs behaviours this module depends on.
Comments only; no code changed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
- The on_data_channel guards called dc.close() from inside the handler,
which webrtc-rs runs to completion BEFORE handle_open binds the SCTP
stream. At that point close() only flips the ready state and returns,
and handle_open sets it back to Open; with detached channels no read
loop is spawned either. So a refused channel stayed open and undrained,
and its queued bytes count against the association-wide receive window
- about a megabyte written into an ignored channel stalls the one
carrying the session, from an unauthenticated peer. Close from the
channel's own on_open instead, where the stream exists. This is also
what makes the ordered+reliable precondition an actual rejection
rather than a log line.
- A write that failed part-way through a fragment sequence called
close_detached() and let the send permit drop, while the timeout path
hands the permit to the teardown for exactly this reason: the close is
in flight, state_notify is still Open, and callers wrap sends in
allow_err! and keep going - so the next message could be written onto
the orphaned prefix the failure left on the peer. send_bytes_inner now
reports that it desynced and the caller, which holds the permit, does
the teardown.
- Drop the catch_unwind around Handle::spawn: release builds set
panic = "abort", so it can never catch anything, and the comment
claimed a mitigation that does not exist. The reachable case is having
no runtime handle at all, which is now logged at debug rather than
warn - Stream's Drop reaches this on every non-runtime thread.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
A message that arrives in one fragment was read into a scratch buffer and
then copied into a second, freshly allocated one - an allocation and a
full copy per message, on top of SCTP's own reassembly copy, for every
input event and most audio packets.
The read buffer is a BytesMut now, so such a message is split straight
out of it: the caller gets a slice sharing the allocation, with no copy
and no allocation. Splitting consumes the buffer from the front, so it is
re-initialized in chunks rather than per read, which spreads the one
remaining cost (zeroing) across every small message that fits in a chunk.
Slices keep their chunk alive, so this trades a bounded amount of
retention for the copy.
Multi-fragment messages still accumulate, unchanged - there is nothing to
hand out until the last fragment arrives.
Test alternates whole and fragmented messages either side of the fragment
boundary for long enough to cross several refills, which is where the
consumed-buffer scheme differs from the fixed one; an off-by-one in the
split reddens it.
The send side keeps its per-fragment copy: prepending the header byte is
what forces it, and removing that needs the fragment flag to move into
the SCTP PPID - another wire change, so it is deliberately left alone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
A WebRTC peer connection outlives its handle - the session cache holds a
clone, and the handler that evicts it only fires on a terminal ICE state
- so it has to be closed explicitly. Making that a per-exit-path
obligation meant every return, break and `?` had to remember it, and the
long-lived side never did: server::connection ends its ~15 exits by
dropping the stream, and the transport race drops the losing result
outright. Nothing warned; a missed close leaks a pc, its ICE agent and
its sockets silently, and only under WebRTC.
Stream is not Clone, so dropping it is the end of the transport and
there is no second owner to surprise - drop-means-close is simply what
the type already meant. The explicit close_webrtc() calls stay valid
(they close sooner than scope end), but they are an optimization now
rather than the thing correctness rests on.
Also delete get_webrtc_stream(): it had no callers and was the one API
handing out an owned clone that outlives its Stream, i.e. the only way
to defeat this.
Regression test included; it fails with the drop body emptied.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
- send_bytes computed one deadline before acquiring the send gate and
reused it for the write, so time queued behind another clone's message
was charged to this message. A caller that only just lost the gate
race got a few milliseconds to write in and then tore the whole peer
connection down for missing them - the narrower the miss, the more
certain the teardown. The write gets its own budget; the gate wait
keeps the one it had, and still never closes (it never held the
permit).
- Stream::close_webrtc awaited WebRTCStream::close, which is exactly the
form close_detached exists to avoid: most callers sit in a select! arm
or a future the UI can abandon, and a cancelled close is unretryable
(is_closed is latched before the first await, so later attempts
early-return and the handler that evicts the session never runs). It
is now a plain fn calling close_detached - with no await point there
is nothing to cancel. close_detached is public and carries the
runtime-teardown guard the parent repo had written separately for its
Drop path, so that copy goes away.
- Log retention goes back to 31 files. Raising it to 31*8 defended the
flood case badly (a file count cannot outrun a flood; only the rate
limits at the log sites can) while silently multiplying steady-state
retention and disk for every ordinary install, on every platform.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Two findings, both cases of a check that reads correct but never runs.
- The SESSIONS admissibility test was applied at the lookup only. The
insert-time duplicate check reads the SAME key and returned whatever
it found, so an entry the lookup had just rejected came straight back
- the freshly built peer connection was closed and the rejected one
returned in its place. A Relay-only request could therefore be served
by a cached All-policy pc (free to pick a direct pair, with
is_relayed() answering for a policy nobody asked for), and a caller
could be handed a pc already latched Closed. Both sites now share
`is_reusable_for`, which is the only way a test on a shared key holds.
- A TURN server with no credentials is not just useless: webrtc-rs
validates every configured server when the peer connection is built,
so one such entry fails EVERY connection, including plain non-relay
ones that never wanted TURN. The RFC 7065 spelling this branch taught
us to parse has nowhere to put credentials, so it produced exactly
that entry - and has_turn_server() then reported TURN as available,
making callers skip their "don't build a guaranteed-dead Relay-only
pc" guard. Drop such entries at the source (with a log naming the
spelling that does carry credentials); has_turn_server() goes back to
a plain scheme test, which is sound once the constructor guarantees a
host and credentials.
Malformed entries are now dropped rather than repaired: an unparsable
port used to be folded back into the host ("host:99999:3478") and an
unbracketed IPv6 literal was split at its last colon, both of which
webrtc-ice rejects - again taking every server down, not just the bad
one.
Also spell `&'static str` on the two associated consts (an elided
lifetime there is a future hard error on the pinned 1.75 toolchain) and
drop a test import left behind when the tests stopped touching config.
Regression test for the cache guard; mutation-checked, as are the
credential and malformed-entry paths.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Continues the review pass. Eight findings, each verified against the
vendored webrtc-rs (0.13 / -data 0.11 / -ice 0.13) rather than inferred:
- a mid-message dc.write() failure returned the error and left the pc
open, so the peer kept an unterminated FRAG_MORE prefix and appended
the next message to it - undetectable, since this framing carries no
length or sequence number, and callers wrap sends in allow_err!. Close
the stream when fragments are already on the wire.
- one deadline covered the connect-wait, the gate queue and every write.
A slow ICE/DTLS completion therefore ate the budget and the write
timed out into a pc.close() an RTT from working; and the gate arm
closed the pc from a task that never held the permit, aborting a
healthy sender's fragment sequence - the exact corruption the permit
exists to prevent. Connection setup gets its own budget, and only the
arm holding the permit tears down.
- a SESSIONS hit returned a pc built for the first caller, so a replayed
offer could get an All-policy connection where the mediator had just
computed Relay-only, with is_relayed() answering from the cached
handle. It could also hand back a stream the state handler had already
closed (it closes before it evicts). Reject both; peer_verified stays
shared, being a fact about the certificate the entry is keyed by.
- the ICE-candidate sender lives in the on_ice_candidate handler and
close() clears no handler, so the receiver never closed and the
forwarder loop this API asks callers to write parked on recv() holding
a stream clone - one leaked task plus one leaked pc per connection.
The terminal-state handler now drops that closure. Regression test
included (fails, by 20s timeout, with the drop removed).
- has_turn_server() accepted the RFC 7065 spelling `turn:host:port`,
which url makes cannot-be-a-base: host_str() is None, so the server
became a hostless "turn::3478" that still passed the scheme check.
force_relay then built a Relay-only pc that could only time out. Parse
host and port out of the path (IPv6 literals included), and make the
gate require a host.
- is_relayed() read the stats report's `nominated` flag, which
webrtc-ice sets per checklist entry and never clears, so after a pair
switch several entries carry it and HashMap order picked the answer.
Read the selected pair instead - which also drops a redundant stats
round trip.
- the ICE-server parsing tests rewrote the process-global, on-disk
`ice-servers` option while sibling loopback tests were building peer
connections from it. Split parsing out of get_ice_servers so they can
test it directly; the suite now passes in parallel.
- log retention is a file count, so pairing it with the new 16 MiB size
criterion let a flooder rotate away every file predating its own
activity. Keep enough files that ~31 days survives even at full size.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Four review findings on the receive path, all verified against the
vendored webrtc-rs rather than inferred:
- next() awaited pc.close() on every error/EOF path while every consumer
polls next() inside a select! against a 1s timer. close() latches
is_closed before its first await and fires the state handler last, so
losing that race left a pc no later close() could retry, a SESSIONS
entry only that handler evicts, and a state_notify that never reaches
Closed. close_detached() hands the teardown to the runtime; being a
non-async fn, its callers have no await point to be cancelled at. The
send path's timeout arm passes its logical-message permit along, so
the exclusion it relies on now outlives the caller too.
- the reassembly cap was checked after extend_from_slice, so the peak
was the cap plus a fragment, and BytesMut's reallocate-and-copy growth
held old and new buffers at once. Check before appending, and stop
borrowing bytes_codec's ~1 GiB MAX_FRAME_LENGTH: that bound is only
affordable for TCP because its length prefix rejects an oversize frame
before buffering any of it, while this framing can only discover the
overrun by accumulating it - and the answerer runs before any password
check. MAX_RECV_MESSAGE (64 MiB) bounds both directions.
- the EOF path claimed an empty message could never be confused with a
reset. It can: webrtc-data maps the StringEmpty/BinaryEmpty PPIDs to
n == 0 and dc.read() discards the flag that separates them. Both mean
the same thing to us, so the handling stands - the comment and the log
line now say what actually happened.
- on_data_channel bound whatever the remote opened, however it opened
it. Reassembly spans messages, so it is sound only on an ordered,
fully-reliable channel, and webrtc-rs derives those parameters
verbatim from the remote's DCEP OPEN; extra channels additionally
split teardown from the channel carrying traffic and re-arm Open over
a latched Closed. Refuse both.
Also release the accumulator on the EOF and read-error exits, which were
the only paths that left a partial message reachable through the
SESSIONS clone.
Regression tests for the detached teardown and the bind-once guard, both
mutation-checked; the test comments state what is and is not covered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
The type alone still needs a static plus an `if let` at every use, which
is why the codebase kept hand-rolling equivalents. The macro declares the
static for itself, so adding a bounded site is one line, and it appends
the multiplicity only when there is one to report - an isolated event
logs exactly as it would unthrottled.
Count semantics stay inclusive (the reported number is the total this
line stands for, first occurrence = 1), so a reader needs no arithmetic;
the type's docs now point at the macro and say when to reach past it.
Also rustfmt the module and webrtc.rs, which had drifted (no CI gate
enforces it on this branch).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
rustdeskandClaude Opus 5
RustDesk
fufesou
21pages