Repository navigation
fix(pdu): check remaining length when skipping padding and reading client core data - #2130
Conversation
`read_padding!` advanced the read cursor without checking the remaining length, so decoding truncated input could move the cursor past the end of the buffer and panic on a later read. - `read_padding!` now returns a "not enough bytes" error from the enclosing function, like `ensure_size!`, with the caller's name and cursor offset as context. Existing call sites are covered without modification. - Add `ironrdp_core::try_read_padding`, the fallible counterpart of `read_padding`, whose signature and behavior are unchanged. - The duplicate PER `read_padding` is removed; the conference create request decoder uses `read_padding!`. - `ReadCursor::len()` saturates at zero instead of overflowing when the cursor was advanced past the end, and `advance` gains a debug assertion that reports such an advance where it happens. The optional Padding of DR_DRIVE_QUERY_DIRECTORY_RSP ([MS-RDPEFS] 2.2.3.4.10) is now only skipped when present. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
desktopPhysicalHeight and deviceScaleFactor were read with an unchecked `read_u32` once their preceding field was present, so a Client Core Data block ending right after desktopPhysicalWidth or desktopScaleFactor was read past its end. Both fields are required by [MS-RDPBCGR] 2.2.1.3.2 when their predecessor is present, so such a block is now rejected with a "not enough bytes" error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
breaking-change
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #2130 fixes a real cursor-overrun panic class: read_padding! now checks remaining length via ensure_size! before advancing, ReadCursor::len saturates and advance gains a debug assertion, ClientCoreOptionalData bounds-checks paired mandatory optional fields, and the optional DR_DRIVE_QUERY_DIRECTORY_RSP padding byte is skipped only when present. The changes match MS-RDPBCGR 2.2.1.3.2 and MS-RDPEFS 2.2.3.4.10, and the bundled tests demonstrate the previously panicking truncation paths. No blocking defects found. Four low-severity maintainability candidates from specialists were verified against pr-head: the exported macro's new requirement on the enclosing function makes the 'no public API change' claim incomplete; the newly added try_read_padding is unused in-repo and overlaps read_padding! (two duplicate candidates, merged); and the free read_padding function is now an unchecked duplicate of ReadCursor::advance with no remaining callers, worth deprecating. All four are accepted as …
- [code-compressor] Free read_padding function is now an unchecked duplicate of ReadCursor::advance with no remaining in-repo callers — low 🟡 — crates/ironrdp-core/src/padding.rs
The macro no longer routes through the free read_padding (its body is just src.advance(n)), and repo searches find no remaining callers (cliprdr and egfx use read_padding!); the PR documents it as unchecked and steers users to the macro. Deprecating it ahead of removal would eliminate a redundant public entry point alongside advance (unchecked), read_padding! (checked, function-name context), and try_read_padding (checked, fixed context). Removal would be a breaking API change for a pre-existing export, so deprecation is the appropriate first step. Optional maintainability suggestion, not a defect.
Push a commit after addressing these findings. If no code change is needed, you may resolve inline threads and comment @github-actions review-ready to request human review.
| ($src:expr, $n:expr) => {{ | ||
| let n: usize = $n; | ||
| let src: &mut $crate::ReadCursor<'_> = $src; | ||
| $crate::ensure_size!(in: src, size: n); | ||
| src.advance(n); | ||
| }}; |
There was a problem hiding this comment.
[skeptical] read_padding! now requires the enclosing function to return a decode Result, making the PR's 'no public API change' claim incomplete — low 🟡 — read_padding! is #[macro_export] and its new expansion performs ensure_size! followed by an early Err return, so enclosing functions not returning a Result compatible with ironrdp_core::Error no longer compile; previously the macro advanced the cursor in any context. cargo-semver-checks cannot detect macro semantic changes, so the PR body's compatibility claim overstates safety for downstream users. The change is intentional, documented, and mirrors ensure_size!'s existing requirement, so it is a documentation/communication gap rather than a code defect.
There was a problem hiding this comment.
Agreed, the compatibility note was overstated. 32e840e documents on read_padding! that, like ensure_size!, it must be used in a function returning Result<_, DecodeError>, and the PR description now states this contract change explicitly. All workspace uses already satisfy it.
| /// Moves read cursor, ignoring padding bytes, after checking that `n` bytes remain. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns a "not enough bytes" error, leaving the cursor untouched, when fewer than `n` bytes remain. | ||
| #[inline] | ||
| pub fn try_read_padding(src: &mut ReadCursor<'_>, n: usize) -> DecodeResult<()> { | ||
| ensure_size!(ctx: "padding", in: src, size: n); | ||
| src.advance(n); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
[skeptical + code-compressor] Newly exported try_read_padding has no callers and its logic is duplicated inline in read_padding! — low 🟡 — try_read_padding is added and re-exported from ironrdp-core, but nothing in the workspace calls it; only the new testsuite tests exercise it. read_padding! expands to the same ensure_size!-plus-advance sequence inline instead of delegating to try_read_padding(...)?, the only difference being error context (enclosing function name versus the fixed "padding" context). Since the function is newly added by this PR, no external callers can exist yet, so this is speculative public API surface: keeping one of the two entry points (or implementing the macro via try_read_padding with an accepted context change) would be a narrower change. Non-blocking design/maintainability note, not a defect.
There was a problem hiding this comment.
Removed in 32e840e; read_padding! is now the only checked entry point. The two remaining read_padding calls in ironrdp-rdpeusb use the macro too. I kept the read_padding function itself unchanged, documented as unchecked: deprecating it is reported by cargo-semver-checks as an API change, so I’d rather do that in a separate PR.
…y point - Remove `try_read_padding`: it had no callers and duplicated `read_padding!`. - Use `read_padding!` for the two remaining `read_padding` calls in ironrdp-rdpeusb, so no decoder in the workspace skips padding unchecked. - Document that `read_padding!`, like `ensure_size!`, must be used in a function returning a `Result` whose error type is `DecodeError`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
removed
scope/cross-cutting
|
Marc-André Moreau (@mamoreau-devolutions) I addressed the low findings. Could you please approve the outstanding CI runs again |
b69cda2
into
Devolutions:master
…ktop-scale-factor * upstream/master: (92 commits) fix(server): advertise horizontal mouse wheel input (Devolutions#2236) fix(pdu): reject RSA moduli below 512 bits (Devolutions#2225) ci(ffi): verify generated bindings (Devolutions#2166) build(ffi)!: move to Diplomat 0.16.1-devo.2 (Devolutions#2165) build: make the client's video, camera and decoder crates publishable (Devolutions#2239) ci(release): release the newly published crates (Devolutions#2218) perf(graphics): replace the RLGR decoder's bit reader (Devolutions#2150) chore(graphics): add RemoteFX decode benchmarks (Devolutions#2149) fix(server)!: pass a GfxContext to the GfxServerFactory build methods (Devolutions#2104) fix(pdu): check remaining length when skipping padding and reading client core data (Devolutions#2130) fix(session): avoid hang on empty QOIZ updates (Devolutions#2147) feat(session): expose EGFX compositing (Devolutions#2120) fix(server): send RDP as the Demand Active source descriptor (Devolutions#2119) feat(activex)!: broaden mstscax compatibility (Devolutions#2211) feat(viewer): run RemoteApp programs in native windows (Devolutions#2201) fix(graphics): reject RLGR3 values larger than their remainder (Devolutions#2132) feat(egfx)!: composite surfaces mapped to RemoteApp windows (Devolutions#2197) feat: redirect USB devices over URBDRC with nusb and RemoteFX USB backends (Devolutions#2181) build: mark exposed dependencies as public (Devolutions#2213) chore(release): prepare for publishing (Devolutions#2212) ... # Conflicts: # crates/ironrdp-testsuite-extra/tests/e2e.rs
Summary
read_padding!now checks the remaining length and returns a "not enough bytes" decode error from the enclosing function, likeensure_size!, instead of advancing the read cursor past the end of the buffer (which made a later read panic). All existing call sites are covered without modification; the error carries the caller's name and the cursor offset.ironrdp_core::read_paddingcalls (ironrdp-rdpeusb) useread_padding!too. The function itself keeps its signature and behavior, now documented as unchecked.read_padding;ConferenceCreateRequest::decodeusesread_padding!.ReadCursor::len()saturates at zero instead of overflowing when the cursor was advanced past the end, andReadCursor::advancegains a debug assertion so debug, test and fuzz builds report such an advance where it happens.ClientCoreOptionalData::decodechecks the remaining length before readingdesktopPhysicalHeightanddeviceScaleFactor. Both are required by [MS-RDPBCGR] 2.2.1.3.2 once their predecessor is present, so a block ending in between is rejected with a "not enough bytes" error.DR_DRIVE_QUERY_DIRECTORY_RSP([MS-RDPEFS] 2.2.3.4.10) is only skipped when present, so a length-0 response without it is accepted.Compatibility
cargo-semver-checkson theironrdpfacade, with the feature set used by PR automation, reports no semver update required.read_padding!changes its contract: it now returns early with aDecodeError, so it must be used in a function returningResult<_, DecodeError>, the same requirement asensure_size!.cargo-semver-checkscannot see macro bodies. All uses in the workspace already satisfy this, but a downstream crate usingread_padding!elsewhere would stop compiling, so this may warrant treating the change as breaking for the macro.Tests
pdu::mcs: decoding every prefix of a valid Connect Initial returns a result instead of panicking; snapshot of the error for the prefix ending before the PER padding byte.pdu::gcc: same for Client Core Data prefixes, plus blocks ending right afterdesktopPhysicalWidth/desktopScaleFactor.padding:read_padding!on truncated input; theadvancedebug assertion.rdpdr: empty query-directory response with and without the trailing padding byte.cargo xtask check fmt -v,cargo xtask check lints -vand the workspace test suite pass locally (the IPv6 loopback test is skipped: no IPv6 on the test machine).