feat: add ironrdp-str crate with typed wire-aware RDP string primitives#1159
feat: add ironrdp-str crate with typed wire-aware RDP string primitives#1159Benoît Cortier (CBenoit) merged 20 commits intomasterfrom
Conversation
Introduces `ironrdp-str`, a new crate providing zero-copy, lazily-validated string types covering all RDP UTF-16LE field shapes: - `FixedSizeUnicodeString<N>`: fixed-size fields (e.g. clientName, fileName) - `UnicodeStringField<P, N>`: length-prefixed fields with configurable cch/cb prefix and null-terminator policy (NullCounted, NullUncounted, NoNull) - `ExternallyLengthedString`: externally-lengthed fields (length from sibling) - `MultiSzString`: MULTI_SZ string lists (e.g. HardwareIds in MS-RDPEUSB) Key design invariant: wire bytes are stored as `Vec<u16>` and never eagerly converted to Rust strings, enabling single-allocation decode and zero-cost decode-encode passthrough for proxy use cases. Conversion is deferred to explicit `to_native()` / `to_native_lossy()` calls. Uses `bytemuck::try_cast_slice` for zero-copy LE→u16 decode on little-endian targets, with per-element fallback for misaligned buffers and big-endian.
|
Related to #130 |
Coverage Report 🤖 ⚙️Past: New: Diff: -0.10% [this comment will be updated automatically] |
There was a problem hiding this comment.
Pull request overview
Adds a new ironrdp-str crate providing typed, wire-aware UTF-16LE string primitives for multiple RDP field shapes, plus a dedicated testsuite module validating encoding/decoding behavior and edge cases.
Changes:
- Introduce
ironrdp-strcrate with fixed-size, length-prefixed, externally-lengthed, andMULTI_SZstring types using lazy native conversion. - Add comprehensive tests in
ironrdp-testsuite-corecovering round-trips, odd-length rejection, null handling, non-BMP, and surrogate behavior. - Wire the new crate into the workspace/tests by updating
Cargo.toml, test module registration, andCargo.lock.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/ironrdp-str/Cargo.toml | New crate manifest and feature/dependency setup |
| crates/ironrdp-str/src/lib.rs | Public API surface + UTF-16 helper functions and error type |
| crates/ironrdp-str/src/repr.rs | Shared internal dual representation (Wire(Vec<u16>) / Native(String)) and conversion helpers |
| crates/ironrdp-str/src/fixed.rs | Fixed-size UTF-16LE field type implementation |
| crates/ironrdp-str/src/prefixed.rs | Length-prefixed UTF-16LE field type implementation with prefix/policy markers |
| crates/ironrdp-str/src/unframed.rs | Externally-lengthed UTF-16LE field type implementation |
| crates/ironrdp-str/src/multi_sz.rs | MULTI_SZ list type implementation |
| crates/ironrdp-str/README.md | Crate documentation and design invariants |
| crates/ironrdp-testsuite-core/Cargo.toml | Add ironrdp-str dependency to testsuite |
| crates/ironrdp-testsuite-core/tests/main.rs | Register new str_types test module |
| crates/ironrdp-testsuite-core/tests/str_types/mod.rs | New test module root |
| crates/ironrdp-testsuite-core/tests/str_types/convert.rs | Tests for UTF-16 byte/unit conversion helpers |
| crates/ironrdp-testsuite-core/tests/str_types/fixed.rs | Tests for fixed-size string behavior |
| crates/ironrdp-testsuite-core/tests/str_types/prefixed.rs | Tests for prefixed field variants and edge cases |
| crates/ironrdp-testsuite-core/tests/str_types/unframed.rs | Tests for externally-lengthed strings |
| crates/ironrdp-testsuite-core/tests/str_types/multi_sz.rs | Tests for MULTI_SZ encoding/decoding and builders |
| Cargo.lock | Lockfile updates for new crate |
💡 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.
|
Copilot open a new pull request to apply changes based on the comments in this thread. Also fix all the clippy warnings. |
|
Benoît Cortier (@CBenoit) I've opened a new pull request, #1160, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ts_flat panic, clippy (#1160) Addresses all review comments on the `ironrdp-str` crate: arithmetic overflow on 32-bit targets in decode/encode paths, a panic on empty input in `from_wire_units_flat`, unnecessary allocation in `TryFrom` impls, unsafe `as usize` casts, and several clippy lints. ## Correctness fixes - **`from_wire_units_flat` empty-input panic** — `if let Some(&u) = last() && u != 0` silently passed for empty `Vec`, then hit `len() - 1` underflow. Replaced with `if last() != Some(&0)` which rejects empty input naturally. - **`checked_mul(2)` in decode** (`prefixed`, `multi_sz`, `unframed`) — a large wire-supplied length on 32-bit/wasm32 wraps to a small value, making `ensure_size!` pass and causing under-read + cursor desync. Each decode path now returns `InvalidField` on overflow. - **`checked_mul(2)` in encode** (`prefixed`) — byte-count prefix `counted_cch * 2` can overflow; now returns `InvalidField`. - **`saturating_mul`/`saturating_add` in `size()`** (`multi_sz`) — overflow wrapped to a small value would let `ensure_size!` succeed with an undersized buffer; saturation to `usize::MAX` forces the check to fail instead. ## Allocation / API fixes - **`TryFrom` impls use `into_native()`** (`fixed`, `prefixed`, `unframed`) — `to_native().map(Cow::into_owned)` always allocates; `into_native()` is zero-cost for the `Native` variant. ```rust // Before — always allocates even for String-constructed values fn try_from(s: FixedSizeUnicodeString<N>) -> Result<Self, Self::Error> { s.0.to_native().map(Cow::into_owned) } // After — zero-cost for native-constructed values fn try_from(s: FixedSizeUnicodeString<N>) -> Result<Self, Self::Error> { s.0.into_native() } ``` ## Clippy - `as usize` on `read_u32()` → `cast_length!()` (`prefixed` `CchU32`/`CbU32`, `multi_sz`) - `std::error::Error` → `core::error::Error` (`lib.rs`, `fixed.rs`) - `actual <= WCHAR_COUNT - 1` → `actual < WCHAR_COUNT` (`fixed.rs`) - `#[allow(...)]` → `#[expect(...)]` (`multi_sz.rs`) <!-- START COPILOT CODING AGENT TIPS --> --- 💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey). --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: CBenoit <3809077+CBenoit@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
💡 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
💡 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.
Vladyslav Nikonov (vnikonov-devolutions)
left a comment
Vladyslav Nikonov (vnikonov-devolutions)
left a comment
There was a problem hiding this comment.
Impressive work, looking good! Copilot made some good comments, but other than that, I have nothing to add 🥇
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.
💡 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
💡 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.
|
Vladyslav Nikonov (@vnikonov-devolutions) Thanks! I made a few extra Copilot rounds to be safe! (Big diff + subtle logic) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.
💡 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 3 comments.
💡 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
💡 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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
💡 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.
Introduces
ironrdp-str, a new crate providing lazily-validated stringtypes covering all RDP UTF-16LE field shapes:
FixedString<N>: fixed-size fields (e.g.clientName,fileName),zero-padded on encode, trailing-null stripped on decode.
PrefixedString<P, N>: length-prefixed fields with configurableLengthPrefix(CchU16,CchU32,CbU16) andNullTerminatorPolicy(
NullCounted,NullUncounted,NoNull) type parameters.UnframedString: externally-lengthed fields whose length comes from asibling field in the containing message.
MultiSzString:MULTI_SZstring lists (e.g.HardwareIdsinMS-RDPEUSB §2.2.4.2), with a
u32 cchprefix counting all nullterminators including the final sentinel.
Key design invariant: wire bytes are stored as
Vec<u16>and nevereagerly converted to Rust strings, enabling single-allocation decode and
zero-cost decode→encode passthrough for proxy use cases. Conversion is
deferred to explicit
to_native()/to_native_lossy()calls.Uses
bytemuckfor zero-copy LE→u16 reinterpretation on little-endiantargets; falls back to per-element byte-swap on big-endian.