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
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>
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>
Two P3 hardening items from the review: a descendant that changes its
own process group escapes the deadline's group kill, and could then leak
or block the parent.
- The deadline path now also sends a pid-targeted SIGKILL to the direct
child, so child.wait() is bounded even if the child left the group and
the group kill missed it.
- The normal-exit path drains stdout and stderr non-blocking instead of
read_to_string: the child has exited so its output is already
buffered, but an escaped grandchild holding a write end would keep the
pipe from EOF and hang a blocking read. The drain is capped so a
descendant that keeps writing cannot spin it. first_buffered_line now
shares that drain.
Verified: a probe child whose grandchild setpgid-escapes and holds the
pipe returns in 25 ms instead of hanging, and a direct child that
escapes and blocks is bounded to the deadline instead of its full
sleep.
Addresses the maintainer's two requests on #580: linux.rs was crowded,
and the new fallback should not touch the base Wayland path.
All the socket-probe machinery moves to src/platform/linux/wayland_probe.rs
- the child entry point, the runtime-dir scan, the privilege drop, the
process-group probe and its buffered-line inspection - leaving
WaylandDisplayInfo and get_wayland_displays in linux.rs. The module and
the fallback call in get_wayland_displays are gated on a new off-by-default
feature 'wayland_probe'; without it get_wayland_displays returns the
connect error exactly as it did before this fallback existed, so a
consumer that does not build the DRM login-screen backend compiles none
of this. The DRM build turns the feature on through scrap/drm.
Addresses fufesou's #580 re-review:
- The child runs in its own process group and the deadline kills the
whole group, so a hung loginctl or a leaked grandchild cannot survive
the child or keep the pipes open past the reads. Verified: a
deliberately leaked sleep grandchild no longer outlives the probe.
- The probe parses compositor-controlled data, so before touching the
socket the child drops to the runtime directory's owner and refuses
to probe at all if the drop fails, rather than parsing untrusted
bytes as root. loginctl still runs as root first, since finding the
seat needs it.
- first_buffered_line now distinguishes an inspected-but-empty pipe
(WouldBlock) from an uninspectable one (fcntl or read error): only a
real absence of the handshake latches the binary unsupported, an
inspection failure does not.
- rustfmt-clean.
A binary that does not dispatch the probe argument runs its normal
startup; a long-running one outlives the deadline and was killed before
the handshake check could run, so PROBE_UNSUPPORTED never latched and
every enumeration cycle spawned a full consumer process again. The
timeout path now judges the child by what it already wrote: a real
probe prints the magic line first and flushes, so its absence after a
whole deadline means this is not a probe. Only the buffered bytes are
read, non-blocking, because an EOF-seeking read could hang on a
grandchild that inherited the write end of the pipe.
Also report the exit status when a failed child left stderr empty,
which panic=abort and signals do, and name the malformed-list case in
the deserialization error.
Two review findings:
- The release profile builds with panic=abort, so the in-thread isolation
was an illusion: an sctk panic on malformed bytes from a scanned socket
aborted the whole server before ProbeBusyGuard could run. The probe now
runs in a child process spawned from the current executable, which the
consumer binary dispatches to wayland_display_probe_child_main before
any other startup work. A panic there kills only the child, and the
deadline now kills the child instead of leaking a blocked thread.
- seat0_runtime_dir ran loginctl through Command::output with no bound,
before the worker and its timeout existed, while the caller held the
DISPLAYS lock. The lookup now runs inside the child, under the same
two second deadline as everything else.
A binary that does not dispatch the probe arg fails a magic line
handshake and the probe latches off for the process lifetime, so the
fallback degrades to the pre-fallback behavior instead of spawning a
full consumer process per enumeration cycle. The consumer wiring is one
early dispatch in core_main:
#[cfg(target_os = "linux")]
if std::env::args().nth(1).as_deref()
== Some(hbb_common::platform::linux::WAYLAND_DISPLAY_PROBE_ARG)
{
hbb_common::platform::linux::wayland_display_probe_child_main();
}
The fallback was silent on success, so a host where it engaged looked identical to one
where it was never reached. Debug level: it runs on a cache miss, not per frame.
Answers the review on this PR. The fallback kept three states that could not
self-heal, and two of them came from the same place: it trusted the environment and
it had no deadline.
B1, no deadline on any part of the new path while the caller holds a process-wide
lock: `connect(2)` parks on a full backlog and sctk's roundtrip polls with no
deadline, so a socket that accepts and never speaks wayland stalls the display
service, the six drm_capturer call sites and the flutter_ffi SyncReturn. The probe
now runs on its own thread with a 2 s deadline, under the 3 s the uinput caller
already budgets, and only one probe can be in flight. That also keeps sctk's
`panic!`/`todo!` on malformed output events off the thread holding the lock, so it
can no longer poison that mutex, and a `Builder` is used because `thread::spawn`
panics when a thread cannot be created.
B2, a newly reachable empty `Ok` latched into the caller's cache for the process
lifetime: an empty list is now an error, so the existing no-cache path still
applies and the next poll retries.
B3, the `WAYLAND_SOCKET` half of the guard could not fire, because
`connect_to_env` removes that variable on its success and bad-fd paths and this is
re-entered every ~1.5 s. Both variables are read at the call site before
connecting, and the answer is latched: a consumed variable cannot turn a process
that was pointed at a compositor into one free to look for another.
C4 and S1 have the same fix: the directory is `/run/user/<uid>` of the active seat0
session rather than `XDG_RUNTIME_DIR`, so no environment value reaches a connect in
a process that can be root, and it works in the default build, which is given no
such variable (C3). The candidates are scanned rather than guessed, since
`wl_display_add_socket_auto` takes the first FREE name up to `wayland-32` and a
greeter accumulates leftovers; that drops the `path.exists()` pre-stat and its
TOCTOU window with it (C7), and empty values are no longer read as names (C6).
C2, the first socket that accepted used to win unconditionally: the loop now
carries on to the next candidate when a socket connects but fails the protocol or
reports no outputs.
C8, `registry_handlers!()` was empty, so a `wl_output` advertised between the
registry snapshot and the roundtrip was dropped and never reached `outputs()`.
C5 is real and is not fixable here: `get_primary_monitor` spawning a timeout-less
xrandr inside the held lock is in the consumer, and I will send it there.
Review of the first version, all three correct.
The important one: `connect_to_env()` honours `WAYLAND_DISPLAY` and `WAYLAND_SOCKET`,
and the fallback ignored that choice. If an explicit endpoint was named and failed,
this could attach to a *different* compositor and read output positions from the wrong
display. It now runs only when neither variable is set, which is also the only case it
was ever justified by.
`XDG_RUNTIME_DIR` is read with `var_os` and rejected when relative, since `.` or
`../tmp` would probe against the working directory. And every failed probe is kept
instead of the last one overwriting the first, so an error naming both sockets says
what happened to both.
The comment is three lines now: the reasoning belongs here, in the commit.
`get_wayland_displays` gives up as soon as `Connection::connect_to_env()` fails,
which is any process that was not handed `WAYLAND_DISPLAY`. A login screen is
the case that matters: rustdesk starts the greeter's `--server` without the
compositor variables on purpose, and the desktop layout is then unavailable for
the whole session even though the compositor is running and its socket sits in
the runtime directory of the very user that process runs as.
Measured at an sddm Plasma Wayland greeter: `/run/user/112/wayland-0` exists,
`connect(2)` to it as that user succeeds, and the registry advertises
`wl_output` and `wl_seat`. With this fallback the greeter reads the same output
layout any other session does.
Strictly a fallback after `connect_to_env` has already failed, so it cannot
change a host where that succeeds; where there is no compositor at all the
socket is simply absent and it fails exactly as before.
rustdesk
RustDesk
fufesou
Mariano Abad