Repository navigation
Test: replay review publication for upstream PR #2005 - #1
meanaverage wants to merge 2 commits into
Conversation
A server can send data on a dynamic channel before it sees the client decline that channel in its Create Response. GNOME Remote Desktop does this for AUDIO_PLAYBACK_DVC right after the Create Request, so a client without an audio channel failed `process` with "access to non existing DVC channel" and the session ended about a second after connecting. Such data has nowhere to go: log it and drop it.
As with a declined channel on the server side, this is not a fault, and a server may keep sending such data, so a warning per PDU would flood the log.
meanaverage
left a comment
meanaverage
left a comment
There was a problem hiding this comment.
PR 2005 replaces a session-ending error in DrdynvcClient::process_data with a debug-logged drop when data arrives for a channel that is not open (e.g. declined AUDIO_PLAYBACK_DVC before the server sees the NO_LISTENER Create Response), consistent with MS-RDPEDYC's ignore-stale-references model, existing process_close/process_soft_sync_request tolerance, and verified by a new reproducing test. Both specialist candidates are valid: the identical pre-decline race remains fatal on the multitransport tunnel path (process_tunnel still errors for channels absent from tunnel_channels, and declined channels are never inserted there), published as a question since tunnel-routing strictness may be intentional and no tunnel reproduction exists; and the new test's local closure duplicates a data-PDU encoding pattern that actually appears four times across the test file, refined to recommend a file-level shared helper.
- [skeptical] Same pre-decline race remains fatal on the multitransport tunnel path — low 🟡 ❓ — crates/ironrdp-dvc/src/client.rs
The PR makes the TCP path drop data for a never-opened channel, but process_tunnel still returns a hard session-ending error when a Data PDU's channel_id is absent from tunnel_channels (client.rs:380-382). Declined channels are never inserted there: process_soft_sync_request only maps channels present in dynamic_channels and skips declined ones with a debug log (lines 510-519), while its own comments state the server starts sending data for listed channels (including declined ones) on the tunnel before seeing the Soft-Sync response (lines 498-509). A server reproducing the AUDIO_PLAYBACK_DVC race over a Soft-Sync tunnel would therefore still terminate the session. It is unclear whether this is intentional tunnel-routing validation (MS-RDPEDYC 3.1.5.4.3/3.2.5.3.2) that the PR deliberately scopes out, and no tunnel-path reproduction was provided, so maintainer input is needed before treating it as a defect.
| let data = |channel_id: u32, payload: &[u8]| { | ||
| encode_vec(&DrdynvcServerPdu::Data(DrdynvcDataPdu::Data(DataPdu::new( | ||
| channel_id, | ||
| payload.to_vec(), | ||
| )))) | ||
| .expect("DVC data should encode") | ||
| }; |
There was a problem hiding this comment.
[code-compressor] New test's data closure duplicates a data-PDU encoding pattern repeated across the file — low 🟡 — The new test introduces a local data closure (lines 99-105) wrapping the encode_vec(&DrdynvcServerPdu::Data(DrdynvcDataPdu::Data(DataPdu::new(...)))) construction, which is already written inline in established_dynamic_channel_routes_recorded_data_without_negotiation (lines 133-137) and appears again in two further tests (lines 243 and 295). Hoisting one file-level helper function in the style of the existing exchange_capabilities helper and using it at all four sites would remove the duplication and shrink each test body. The closure as written is correct; this is a behavior-neutral, optional cleanup.
1 participant
Disposable diagnostic PR in this public fork. Recreates the exact base and head of Devolutions#2005 to replay the review API payload that failed during the October 7 GitHub outage. No merge intended; this PR will be closed after the test.