[AKolenda]

Every Windows host tested on a LAN answers the RDPUDP SYN with uUdpVer 0x0002: they speak MS-RDPEUDP (version 1/2), not MS-RDPEUDP2 (version 3). A client that only implements the version 3 framing therefore always times out on the UDP handshake and falls back to TCP, so the multitransport path never carries data against Windows.

This adds the version 1/2 reliable data path to the sans-I/O connection state machine, negotiated from the SYN+ACK and reusing the existing send/receive windows, RTT estimator, loss detection and timers:

• pdu: RDPUDP_SOURCE_PAYLOAD_HEADER (2.2.2.4) and the data payload on a version 1/2 Source Packet. DATA datagrams decode; FEC (lossy) datagrams are rejected.
• connection: NegotiatedParams records the wire format. Version 1/2 uses a single Source sequence space from ISN+1; snCoded numbers each transmission, snSourceAck drives an ACK vector walked down from the highest received sequence (3.1.1.4), ACK-of-ACKs resets the receiver's vector base (2.2.2.6), a Congestion Notify is answered with CWR at most once per RTT (3.1.1.8), and ACKDELAYED marks timer-driven ACKs (3.1.6.3). Timers take the version's floors.
• The 6-bit ACK-vector run length is encoded as count minus one on the wire, which MS-RDPEUDP 2.2.2.7.1 leaves ambiguous but Windows requires: element 0x00 acknowledges one Source Packet and 0x02 acknowledges three. Reading it literally leaves the first Source Packet unacknowledged forever, collapsing the congestion window.
• seq: 32-bit wrap-aware sequence reconstruction and narrowing.
• ConnectionConfig::offer_version lets a client offer version 2 or 1 outright (no cookieHash), which is what the measured hosts answer immediately.
• RdpeudpConnection::v1_stats exposes retransmit, ACK and datagram counters for observability.

Verified against the spec's 4.2.1 example capture and new state-machine tests; the existing tests are unchanged.

Note on MS-RDPEUDP 4.2.3. The spec's worked example annotates ACK vector element 0x04 as "4 datagrams received," a literal reading of the six-bit run length. Captures against Windows Server show the field is the run length minus one: after one Source Packet the server answers snSourceAck = 1 with element 0x00, after four it answers snSourceAck = 4 with 0x03. Read literally, the first is an empty run and the second never acknowledges seq 1, which stalls the sender. This implementation follows the wire, not the example. Evidence: https://gist.github.com/AKolenda/51931b094dd454ef0eb1430503629c8a

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Greg Lamberson (glamberson)]

I think there's an off-by-one in the ACK vector run-length encoding here. connection.rs:2131-2132 reads:

Windows encodes a run of n datagrams as n - 1 (an element of 0x00 acknowledges exactly one), which 2.2.2.7.1's prose leaves open.

MS-RDPEUDP 2.2.2.7.1 says the six least significant bits "specify the length of a continuous sequence of datagrams," and all three of the spec's own worked examples (4.2.1, 4.2.2, 4.2.3) gloss the same byte the same way: raw value 4 is "length of the vector, 4 datagrams received," a direct literal reading with no minus-one. FreeRDP's Wireshark dissector (tools/wireshark/rdp-udp.lua) reads it the same way: local rleLen = bit32.band(b, 0x3f), no adjustment.

This crate's own pre-existing enqueue_final_ack (unchanged by this PR) sends V1AckVectorElement { length: 1, .. } to acknowledge exactly one datagram, a literal encoding. That's the opposite of what this PR's new code assumes, so after this merges the same field means two different things depending on which code path built it.

The 4.2.1-capture test added here (SourcePayloadHeader round-trip) doesn't actually exercise the ACK vector byte from that same example, only the header bytes next to it. Round-tripping 0x04 from that capture through build_v1_ack_parts/process_v1_acknowledgement would show 4 datagrams going in and 5 coming out.

If I'm wrong about this and there's a real packet capture showing Windows requiring the minus-one reading, I'd like to see it, since it contradicts the spec text, all three of its own examples, and FreeRDP's independent implementation. As written, process_v1_acknowledgement's 0..=element.length will count one more datagram as acknowledged than the peer actually confirmed, which on a lossy link means retiring a send-window entry the peer never received.

[Greg Lamberson (glamberson)]

This counts one more datagram than the peer confirmed, for the reasons in the review body above.

[Greg Lamberson (glamberson)]

This is the encode side of the same issue.

[Greg Lamberson (glamberson)]

Test fixtures bake in the same convention, which is why the suite doesn't catch it against the spec's own example.

[AKolenda]

Thanks for the close read. The minus-one encoding is measured Windows behaviour, not a guess. Capture and byte-level decode here: https://gist.github.com/AKolenda/51931b094dd454ef0eb1430503629c8a

Short version, from a session against Windows Server (MS-RDPEUDP protocol version 2, which uses the same FEC-header framing and 2.2.2.7.1 ACK vector). After our first Source Packet (seq 1, the TLS ClientHello) the ServerHello came back with snSourceAck = 1 and ACK vector 00 01 00 b5, one element 0x00. After four Source Packets (seq 1..4, TLS handshake complete on both sides) the ACK was 00 00 00 04 00 c6 00 04 00 01 03 b5: snSourceAck = 4, one element 0x03. Later snSourceAck = 5 with element 0x04. No ACK-of-ACKs flag in any of them. Read literally, 0x00 is an empty run and 0x03 leaves seq 1 unacknowledged forever, which is exactly the stall we hit before this change: seq 1 retransmitted with a doubling RTO until the window collapsed. Read as count-1, every ACK is consistent and the session runs.

On the sources: the 2.2.2.7.1 prose never gives the numeric mapping, and 4.2.1/4.2.2/4.2.3 show the 0x04 byte without the peer's sequence numbers, so they cannot distinguish 4 from 5. The rdp-udp.lua rleLen = bit32.band(b, 0x3f) code and the packet-rdpudp.c base_seq + rle_len code are both the RDPEUDP2 ACK vector (dissectV2 / RDPUDP2_ACKVEC), a different format. The Wireshark v1 path only renders the 0xc0 and 0x3f bit fields and does no sequence arithmetic, so it takes no position.

You are right that enqueue_final_ack was left on the old convention, sending a run of 1 where the new builder sends 0. Fixed in c5d3a71 with a test on the wire byte, plus a regression test that feeds the captured Windows ACKs through process_v1_acknowledgement and checks the send window drains. V1AckVectorElement::length is now documented as the raw wire value.

[Greg Lamberson (glamberson)]

Thanks for the detailed reply and the capture. That's genuinely convincing evidence.

I want to flag something for the record, though: MS-RDPEUDP's own worked example in section 4.2.3 explicitly reads a single ACK vector element with value 0x04 as "4 datagrams received," literally, with no minus-one adjustment. That's Microsoft's own annotation on their own example, and it directly conflicts with the count-minus-one convention your capture demonstrates and the fix now encodes.

I traced the actual walk in process_v1_acknowledgement against your two examples (snSourceAck=4/element=0x03, and the two-element gap test) and it's internally self-consistent under the count-minus-one model: It doesn't double-count across multiple elements, since current only ever anchors to snSourceAck once, at the start of the walk. So the implementation checks out given the premise.

But the premise is where the two sources disagree, and that conflict shouldn't pass silently. A live capture against a real Windows Server is stronger evidence than a documentation example (Microsoft's open specs have known errata, and the literal reading reproduces the exact stall this PR fixes), so I'd trust your fix over the worked example here. Might be worth a line in the PR body noting the discrepancy, for whoever reads section 4.2.3 next and wonders why the code doesn't match it.

[AKolenda]

Agreed on all counts, and thanks for tracing the walk. I've added a note to the PR body: 2.2.2.7.1's prose gives no numeric mapping, 4.2.3's annotation reads 0x04 as four datagrams literally, and Windows Server on the wire encodes a run of n as n-1 (0x00 acknowledges one). The code follows the wire, with the capture linked as the evidence, so readers of 4.2.3 know why the two differ.

[AKolenda]

I believe on all counts based on the back and forth that we have, I think we're ready to merge. Maintainers agree?

[Benoît Cortier (CBenoit)]

Agreed; I started the CI workflow and Copilot review. I’ll likely merge regardless, but I would appreciate follow up PRs if relevant. Thank you!

[Benoît Cortier (CBenoit)]

Looks like this is breaking the testsuite crates compilation.

[Copilot]

🟡 Changes recommended

ACK-state retirement, flow control, congestion signaling, and malformed-packet handling contain protocol-breaking issues.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

crates/ironrdp-rdpeudp/src/connection.rs:2243

• Truncating the vector from the highest-sequence end drops the oldest runs while snSourceAck remains the overall highest sequence. With more than 32 alternating runs, an old received packet is never represented, so the sender keeps it pending and cannot send an ACK-of-ACKs past it. Encode a complete representable range or constrain the V1 receive window so its full worst-case vector fits; do not silently omit the low end.

                if elements.len() >= V1_ACK_VECTOR_MAX_ELEMENTS {
                    break 'runs;

crates/ironrdp-rdpeudp/src/connection.rs:2303

• commit_acknowledgement() releases the leading receive-window slots as soon as this ACK is encoded. In MS-RDPEUDP, the peer's ACK-of-ACKs is what resets the ACK-vector base; if this ACK is lost, the retransmitted Source Packet is now below base_seq, is discarded as a duplicate, and can never be acknowledged. Retain V1 reception state until process_v1_ack_of_acks advances it.

        self.commit_acknowledgement();

crates/ironrdp-rdpeudp/src/connection.rs:2128

• This ACK path uses sn_source_ack but ignores header.receive_window_size, and the send path gates only on the local SendWindow and congestion window. MS-RDPEUDP 3.1.5.3.2.1 permits a new Source Packet only when there is space in the receiver-advertised window, so a peer advertising a smaller or zero window will discard packets this connection continues sending. Track the remote window from SYN+ACK/ACK headers and gate new V1 Source Packets against it.

        let reference = send_window.next_channel_seq().saturating_sub(1);
        let highest_seen = seq::reconstruct_seq32(header.sn_source_ack, reference);

crates/ironrdp-rdpeudp/src/connection.rs:2142

• A decoded ACK vector may contain 2,048 elements, each representing 64 sequences, and every represented sequence performs a fresh linear scan of up to 32,768 pending entries. A small untrusted datagram can therefore trigger billions of comparisons and block the sans-I/O caller. Validate the represented span against the negotiated window and use a direct source-sequence lookup or a single ordered walk.

                    if element.state.is_received() {
                        if let Some(entry) = send_window.pending_entries().find(|e| e.channel_seq == source_seq) {
                            acked.push((entry.data_seq, entry.transmit_count == 1, entry.sent_at));

crates/ironrdp-rdpeudp/src/connection.rs:2202

• A reset reconstructed ahead of highest_seq makes advance_base discard all current state and set the base into the future, after which legitimate Source Packets are permanently below the window. snResetSeqNum is the sender's greatest cumulative ACK and cannot validly exceed the highest received sequence, so ignore or reject such a reset.

        let reference = recv_window.highest_seq();
        let reset = seq::reconstruct_seq32(ack_of_acks.reset_seq_num, reference);
        recv_window.advance_base(reset.saturating_add(1));

crates/ironrdp-rdpeudp/src/connection.rs:2270

• No V1 ACK construction path ever sets RDPUDP_FLAG_CN, even when receive-side loss is detected. MS-RDPEUDP requires the receiver to generate congestion notifications for detected losses and continue until the corresponding CWR arrives; without this feedback the peer does not reduce its send rate. Add receiver-side CN state/loss detection and include the flag until a matching CWR is observed.

        let mut flags = flags | V1Flags::ACK;
        if self.v1_ack_delayed {
            flags |= V1Flags::ACKDELAYED;
        }

crates/ironrdp-rdpeudp/src/connection.rs:2183

• The time-only suppression does not remember which Source Packet carried CWR. The protocol requires stale CNs whose snSourceAck precedes that CWR packet to be ignored; if the CWR is lost, the same old CN can currently reduce the congestion window again after one RTT. Pass the acknowledged source sequence into this path, record the sequence used for CWR, and filter stale CNs by that sequence.

        let rtt = self.rtt.srtt().unwrap_or_else(|| self.effective_rto());
        let recently = self
            .v1_last_cn_reaction
            .is_some_and(|last| now.duration_since(last) < rtt);

• Files reviewed: 9/9 changed files
• Comments generated: 5
• Review effort level: Balanced

[AKolenda]

Sorry about the testsuite break; 098d801 fixes it. ironrdp-testsuite-core builds V1Datagram and ConnectionConfig by hand and had not picked up the new data / offer_version fields. Locally cargo xtask check tests --no-run, cargo xtask check tests (rdpeudp, rdpeudp-tokio, testsuite-core: 195 + 53 + 1649 pass), check lints and check fmt are all green on 1.94.1. One pre-existing test, a_client_rejects_a_syn_ack_that_settles_below_version_3, asserted the behaviour this PR replaces; it now checks that the client follows the server down onto version 2 (MS-RDPEUDP 1.7) and that a version above the SYN's offer is refused.

Copilot review, same commit:

• connection.rs:1160 (version selection): agreed. handle_syn_ack now rejects a version other than 1/2/3 and any selection above ConnectionConfig::offer_version, with tests for both.
• connection.rs:2142 (per-sequence window scan): agreed. The pending entries are indexed once and the walk stops at the lowest pending sequence, so a 2048-element vector costs one pass over the window plus a lookup per named sequence. Test added with a 2048 x 64 vector.
• connection.rs:2202 (ACK-of-ACKs reset ahead of highest_seq): agreed, a reset above what we have received is dropped, with a test that a later Source Packet is still delivered.
• pdu/mod.rs:131, pdu/mod.rs:272, v1_data.rs:22, seq.rs:306: all applied (docs, lowercase error message, named addends, functions moved above the test module and the seq.rs module doc corrected).

Deferred to follow-up PRs, as suggested above, because each changes on-the-wire behaviour I want to validate against Windows before landing:

• connection.rs:2128 gating new Source Packets on the peer's advertised receive_window_size;
• connection.rs:2243 / 2303 how ACK-vector state is retired: the Windows captures never carried an ACK-of-ACKs, so holding reception state until one arrives interacts with the 32-element cap and needs measuring rather than reasoning about;
• connection.rs:2270 / 2183 receiver-side CN generation and sequence-tracked CWR suppression.

Benoît Cortier (@CBenoit) the workflow will need re-approving for the new commit.