feat(session)!: handle Auto-Detect Request PDUs from server#1178
Open
Greg Lamberson (glamberson) wants to merge 2 commits intoDevolutions:masterfrom
Open
feat(session)!: handle Auto-Detect Request PDUs from server#1178Greg Lamberson (glamberson) wants to merge 2 commits intoDevolutions:masterfrom
Greg Lamberson (glamberson) wants to merge 2 commits intoDevolutions:masterfrom
Conversation
The x224 processor now handles Auto-Detect Request PDUs on the IO channel instead of crashing with "unhandled PDU". Three match arms are added before the catch-all: - RttRequest: auto-responds with RttResponse echoing the sequence number, as required by [MS-RDPBCGR 2.2.14.1.1]. - NetworkCharacteristicsResult: surfaced to the application via new ProcessorOutput::AutoDetect and ActiveStageOutput::AutoDetect variants so consumers can use server-reported RTT/bandwidth metrics. - Other AutoDetectReq variants (BW_START, BW_PAYLOAD, BW_STOP): logged and returned as empty output until the bandwidth measurement state machine is implemented. All in-tree consumers of exhaustive ActiveStageOutput matches are updated: ironrdp-client, ironrdp-web, and the FFI layer. BREAKING CHANGE: ActiveStageOutput and ProcessorOutput have new variants. Exhaustive matches must add arms for AutoDetect.
There was a problem hiding this comment.
Pull request overview
Adds active-session handling for server-sent Auto-Detect Request PDUs (MS-RDPBCGR 2.2.14) in the session layer to prevent crashes and to respond to RTT probes as required by the protocol.
Changes:
- Implement Auto-Detect request handling in
x224::Processor, including automatic RTT responses and surfacing NetworkCharacteristicsResult as an output. - Thread the new
AutoDetectoutput throughActiveStageOutputand update in-tree consumers (client, web, FFI) to handle the new variant. - Add a new session testsuite module covering RTT response generation and non-crashing behavior for bandwidth PDUs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
crates/ironrdp-session/src/x224/mod.rs |
Handles Auto-Detect PDUs on the IO channel (RTT auto-response, surface netchar results, ignore BW PDUs). |
crates/ironrdp-session/src/active_stage.rs |
Introduces ActiveStageOutput::AutoDetect and maps it from x224::ProcessorOutput. |
crates/ironrdp-client/src/rdp.rs |
Adds an exhaustive match arm for the new ActiveStageOutput::AutoDetect. |
crates/ironrdp-web/src/session.rs |
Adds an exhaustive match arm for the new ActiveStageOutput::AutoDetect. |
ffi/src/session/mod.rs |
Extends FFI output enum typing to include AutoDetect. |
crates/ironrdp-testsuite-core/tests/session/mod.rs |
Registers the new autodetect test module. |
crates/ironrdp-testsuite-core/tests/session/autodetect.rs |
Adds tests for RTT auto-response, netchar surfacing, and BW request crash-prevention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Use self.user_channel_id instead of server's initiator_id for RTT response encoding. Remove unnecessary clone on NetworkCharacteristics match arm. Split test USER_CHANNEL_ID and IO_CHANNEL_ID to distinct values to catch ID mixup bugs. Add NetworkCharacteristics FFI struct and getter exposing base_rtt_ms, average_rtt_ms, and bandwidth_kbps for connection quality monitoring. These values will feed into FramePacingFeedback when the library-level health observer traits from Devolutions#1158 land.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a crash when the server sends Auto-Detect Request PDUs during an
active session. After #1176 added ShareDataPdu::AutoDetectReq routing,
these PDUs decode correctly but hit the catch-all error path in the x224
processor: "unhandled PDU: Auto-Detect Request PDU".
Per [MS-RDPBCGR 2.2.14], the client must respond to RTT Measure
Requests by echoing the sequence number. Without this, servers that
probe RTT (Windows Server, or IronRDP servers using #1177) get no
response, and the session terminates.
Changes:
Handle RttRequest by auto-responding with RttResponse (echoes the
sequence number per spec). Follows the ShutdownDenied pattern already
used in the processor for protocol-mandated auto-responses.
Surface NetworkCharacteristicsResult as a new
ActiveStageOutput::AutoDetect(AutoDetectRequest) variant. This
carries server-computed RTT and bandwidth metrics for applications
that want adaptive behavior.
Log and return empty for BW_START/BW_PAYLOAD/BW_STOP until the
bandwidth measurement state machine is implemented. This prevents
crashes without incorrect protocol behavior.
Update all in-tree exhaustive matches: ironrdp-client, ironrdp-web,
FFI.
6 tests in ironrdp-testsuite-core: RTT response generation, sequence
number preservation, NetworkCharacteristicsResult surfacing, and
crash-prevention for each BW variant.
Depends on: #1168 (merged), #1176 (merged)
Related: #1177 (server-side RTT), #1158 (multi-codec pipeline)
BREAKING CHANGE: ActiveStageOutput and ProcessorOutput have new
AutoDetect variants. Consumers with exhaustive matches must add an arm.