- 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
Same pattern as enable-udp-punch / enable-ipv6-punch: empty value reads
as on against the public server and off against a private one (the
injection lives in rustdesk's get_local_option), so self-hosted
deployments opt in once their server / TURN is ready.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Reverts the webrtc_all_ice proto field (64b54ab) in favor of an
`ice_policy: "all"` key inside the webrtc:// envelope JSON, next to the
RTCSessionDescription fields. Same information, better carrier:
- it is a property of the offer itself, so it rides with the offer;
- the rendezvous server never has to know: the envelope is an opaque,
length-bounded string to hbbs, so no forwarding code and no vendored
proto copies to keep in sync;
- serde ignores unknown JSON keys when parsing RTCSessionDescription,
so every skew combination degrades exactly like the proto field did:
absence - not an error - is the old Relay-only reading.
endpoint_declares_all_ice() is the receiving side: parse failure,
foreign scheme or missing key all read as "not declared".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
use_ws() folds into force_relay because a ws tunnel kills classic TCP/UDP
punching — but ICE opens its own sockets and does not care how signaling
reaches the server. Without a signal, the controlled side must treat every
force_relay offer as Relay-only ICE (answer gated on TURN), which locks
WebSocket deployments out of direct WebRTC entirely.
webrtc_all_ice marks an offer that gathered every candidate type: the
controller's force_relay covers only classic punching, not ICE policy.
Absent/false keeps today's semantics on every skew combination (old
controller, old server dropping the field, old controlled side).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
`requester_id = 11` was added and removed in the same rebase batch, never
reached main, and never reached hbbs — whose vendored copy of this file still
stops at field 9. So nothing has ever written or read tag 11, and reserving it
guards a wire format that does not exist.
It was also inconsistent with what this branch already does: `IceCandidate`
retyped tag 2 from `string to_id` to `bytes socket_addr` in place, which is only
sound because none of this proto has shipped. Same premise, so tag 11 is free.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`next()` read every non-FRAG_END header as "more fragments", so a peer whose
framing had diverged was only caught by the MAX_FRAME_LENGTH cap — and a
FRAG_MORE carrying no payload was never caught at all: it adds nothing to the
accumulator, so the cap never trips and the loop spins for as long as the peer
keeps writing, with no error and no teardown. Decide the header's meaning in one
match, so a future header kind cannot be handled in one place and missed in the
other. Neither case is reachable from send_bytes_inner, which emits FRAG_MORE
only for a full MAX_FRAGMENT_PAYLOAD chunk.
Release the accumulator on the error paths rather than truncating it: at the cap
that is ~1 GiB still referenced through the SESSIONS clone.
Doc corrections, all of them overclaims in the previous pass:
- the cancel-safety entry held only for the successful read path.
read_data_channel does await after dequeuing on its ErrShortBuffer and DCEP
branches, and next() awaits pc.close() on its error paths — where
RTCPeerConnection::close latches is_closed before its first await, so a
cancelled close silently turns every later close into a no-op and leaves the
pc in SESSIONS.
- recv_state: cancellation drops the guard mid-message, so it is the
single-reader assumption, not the mutex, that ultimately keeps two readers
from splicing into one accumulator.
- is_relayed: stream.rs promised None before pair selection while webrtc.rs
documented Some(true) under Relay policy; align both.
- get_local_endpoint: examples/webrtc.rs calls it too, not only the tests.
- PunchHole.reserved 11: named the wrong writer — PunchHole is written by the
rendezvous server, not by peers. Reserve the name as well as the tag.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rotating on age alone let a single day's file grow without limit, so whoever
can drive a hot log site decided how much disk this uses and no amount of
per-site throttling could bound it. Add a size criterion, which covers every
call site at once — including ones no throttle was added to.
LogThrottle: recover the guard on a poisoned lock rather than returning None.
Poisoning only means another thread panicked while holding it; the guarded
data is two counters that are still usable, and going silent for the rest of
the process is worse than a stale count. AGENTS.md permits handling lock
poisoning directly, and it forbids swallowing the error.
Keep map_or over clippy's is_none_or: that was stabilized in Rust 1.82 and CI
pins 1.75.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
Debug output is written to the log file, so a log site that fires per received
message lets whoever is sending decide how much a machine writes to disk.
Dropping the line instead would hide real faults, so collapse it: one line per
interval carrying the count of everything suppressed since the last one, with
the first occurrence after a quiet period always reported so an isolated fault
is not delayed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ExUfAkYbq8UC9pQCiLy8TQ
- Cargo.toml: record why webrtc is pinned to 0.13 — >=0.14 pulls sdp 0.10 /
webrtc-util 0.12 using usize::is_multiple_of (needs rustc >=1.87), while
rustdesk CI builds with Rust 1.75 (sciter i128 ABI pin)
- module-level upgrade checklist in src/webrtc.rs listing the version-coupled
webrtc-rs internals this transport relies on (SCTP write backpressure,
64KB message cap, detach() semantics, handler-capture leak cycle,
Disconnected transience, stats-based is_relayed), all verified against
webrtc 0.13 / webrtc-data 0.11 / webrtc-sctp 0.12
- send_bytes: document the bounded-backpressure mechanism (128 KiB PendingQueue
semaphore + cwnd/rwnd cap) and that it is NOT cancel-safe
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- 1-byte-header fragmentation past the 64KB SCTP cap; empty-message and clean-EOF handling
- is_relayed() via selected candidate-pair stats for the direct/relayed flag
- IdPk.dtls_fingerprint + rendezvous webrtc SDP/IceCandidate proto fields
- fix pc leaks: Weak capture breaks the state-handler Arc self-cycle; close pc on new() error paths
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every key rustdesk reads through `LocalConfig` was checked against the
settings lists; these were in no list, so a custom client's
`default-settings` / `override-settings` could not set them:
- allow-sync-clipboard-between-sessions (rustdesk/rustdesk#15934): the
constant lived in rustdesk's `src/clipboard.rs`; it is defined here
now, with the other option keys.
- printer-incomming-job-action, allow-printer-auto-print,
printer-selected-name: the incoming-print-job policy, read in
`client/io_loop.rs`.
- enable-check-update: listed for completeness; `check_software_update`
returns early for custom clients, so presetting it changes nothing
there today.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EZ49AbZJYfm8NTp5yDPMab
Written by the texture-render watchdog / startup probe in the main repo;
a failed record forces texture rendering off until it is cleared (probe
pass or an explicit toggle of use-texture-render).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
rustdeskandClaude Opus 5