fix(rdpsnd): echo the Training PDU's wPackSize in the Training Confirm

[meanaverage (meanaverage)]

Problem

MS-RDPEA 2.2.3.2 says the Training Confirm PDU's wPackSize "MUST be the value of the wPackSize field of the Training PDU": the size of the whole Training PDU, header included, or 0 when it carries no data. Rdpsnd::training_confirm sent pdu.data.len() instead, so any Training PDU with data got a confirm that is 8 bytes short.

Servers that check it ignore the confirm. GNOME Remote Desktop 46 logs [RDP.AUDIO_PLAYBACK] Received invalid Training Confirm PDU. Ignoring... and never starts audio playback. Training PDUs without data were unaffected, since both sides are 0 then.

Change

• training_confirm echoes the Training PDU's size: 0 without data, otherwise the encoded size of the whole ServerAudioOutputPdu::Training, which is what the server put in wPackSize.
• No public API change.

Testing

• New training_confirm_echoes_the_training_pdu_size in ironrdp-testsuite-core (rdpsnd client tests): a Training PDU with 32 bytes of data must be confirmed with its full wPackSize (40). It fails without the fix (left: 32, right: 40).
• cargo test -p ironrdp-testsuite-core --test integration_tests_core rdpsnd (73 passing), cargo fmt --all -- --check, cargo clippy -p ironrdp-rdpsnd -p ironrdp-testsuite-core --all-targets -- -D warnings.
• In use: with this change, audio plays from GNOME Remote Desktop 46 (Ubuntu 24.04) through an RDPSND client built on ironrdp-rdpsnd; without it, the server never starts playback.

Prepared with AI assistance; I reviewed the change and ran the tests above.

[github-actions]

Automated review will not run because this contributor is not yet eligible under the automation policy.

Contributors become eligible after one qualifying IronRDP pull request is merged into master. Maintainer review is required.

[Copilot]

Copilot review overview

🟢 Approval recommended

The protocol fix is correct and regression-tested; the remaining allocation concern is optional.

Review effort: Balanced
Findings: 1

Open (1)

• Avoid cloning training payload when calculating encoded size · New

What changed in this PR

Corrects RDPSND Training Confirm sizing to comply with MS-RDPEA.

Changes:

• Echoes the complete Training PDU size.
• Adds regression coverage for non-empty training data.

File	Description
crates/​ironrdp-rdpsnd/​src/​client.rs	Corrects wPackSize generation.
crates/​ironrdp-testsuite-core/​tests/​rdpsnd/​client.rs	Tests the corrected size.

[meanaverage (meanaverage)]

Fixed in 2d99f67, no clone anymore (see the reply below).

[github-actions]

The PR correctly fixes RDPSND Training Confirm wPackSize to echo the Training PDU's whole-PDU size per MS-RDPEA 2.2.3.2. The recomputed value (4-byte header + 4-byte fixed part + data length, or 0 when empty) is byte-identical to what TrainingPdu::encode writes, and because decode derives data length as wPackSize minus 8, it reproduces the received wPackSize exactly for wire-sourced PDUs; the empty-data branch correctly emits 0. The new regression test asserts the exact encoder math. No correctness, protocol, or safety defect found; the only published finding is a low-severity maintainability note about duplicated size-computation logic and an unnecessary full PDU clone, best addressed with a shared helper on TrainingPdu.

[github-actions]

[code-compressor] Duplicated wPackSize computation plus a full PDU clone; share one helper with TrainingPdu::encode — low 🟡 — The added if/else re-implements the size rule already encoded in TrainingPdu::encode (pdu/mod.rs:637-641: 0 when data is empty, otherwise size() + ServerAudioOutputPdu::FIXED_PART_SIZE), creating two copies of the same invariant that can drift. The ServerAudioOutputPdu::Training(pdu.clone()) exists only because size() is called through the enum variant; the value equals TrainingPdu::FIXED_PART_SIZE + data.len() + 4 regardless, so the clone needlessly allocates and copies the data buffer. Extracting a private TrainingPdu helper (e.g. fn w_pack_size(&self) -> usize) used by both encode and training_confirm removes the branch, the clone, and the duplicated arithmetic with byte-identical output and no API change; the existing cast_length!/map_err error handling stays as-is.

[meanaverage (meanaverage)]

Done in 2d99f67: TrainingPdu::pack_size() now holds the rule, and both TrainingPdu::encode and training_confirm use it, so the confirm no longer clones the PDU. Output is byte-identical; the rdpsnd tests pass.

[meanaverage (meanaverage)]

Note: 2d99f67 from the replies above is now 45c76bd, the old SHA no longer exists after the rebase.

The failing "Check public API compatibility" is the sspi / picky-krb 0.12.5 break, not this change. Once #2074 lands I'll rebase so the automation re-runs and the PR picks up needs-review.

[Benoît Cortier (CBenoit)]

PR automation is failing because of a picky-krb 0.12.5 incompatibility, fixed on master by #2074. Please rebase on master to fix it.

Update: no rebase needed anymore. picky-krb 0.12.5 was yanked from crates.io (re-released as 0.13.0), so the API check builds again without changes to this branch. PR automation has been re-run here and passes.

[github-actions]

The PR correctly fixes the RDPSND client's Training Confirm to echo the Training PDU's wPackSize per MS-RDPEA 2.2.3.1/2.2.3.2 instead of the data length alone. The new TrainingPdu::pack_size() helper (0 for empty data, else body size plus the 4-byte SNDPROLOG header) mirrors the decode path's reverse computation, is shared by encode and training_confirm, and involves no public API change. A new integration test covers the full process() path. The sole candidate is a valid, low-severity optional style cleanup in training_confirm. Nothing blocks merge.

1. [code-compressor] Collapse two-step cast_length into a single expression — low 🟡 — crates/ironrdp-rdpsnd/src/client.rs
   training_confirm binds cast_length! to an intermediate EncodeResult<_> variable, then maps the error on a second line. Merge into `let pack_size = cast_length!("wPackSize", pdu.pack_size()).map_err(|e| encode_err!(e))?;` to remove the binding and its explicit annotation with identical behavior; u16 is inferred from TrainingConfirmPdu::pack_size. Optional maintainability cleanup, not a correctness defect.