Skip to content

Commit 0e10d37

Browse files
Greg Lambersonlamco-office
authored andcommitted
feat(error): record byte offset on decode and encode error variants
Reshapes the position-aware error approach per #1266 review feedback. Moves position metadata out of the foundational `ironrdp-error` crate and onto the decode and encode `*ErrorKind` enum variants where it actually applies. Previously this PR added an optional `ErrorPosition { offset, field }` slot to `Error<Kind>`'s `ErrorMeta`, paying a metadata slot on every error in the workspace to serve only the decode and encode surfaces. The new shape places a non-optional `offset: usize` on each variant that can know one: - `DecodeErrorKind::{NotEnoughBytes, InvalidField, UnexpectedMessageType, UnsupportedVersion, UnsupportedValue}` gain `offset: usize` - `EncodeErrorKind::*` mirrors the same shape for encode-side errors - `*::Other` is unchanged; producers of `Other` typically lack stream-cursor access and the variant exists precisely for those cases The `in_field` dotted-path use case is dropped from this PR per the same review: `Error::set_context` already covers it. No parallel structured field-tracking channel is introduced. Trait and macro changes: - `NotEnoughBytesErr`, `InvalidFieldErr`, `UnexpectedMessageTypeErr`, `UnsupportedVersionErr`, `UnsupportedValueErr` all take an additional `offset: usize` parameter - The five corresponding `*_err!` macros gain `at:` and `in:` prefixes for explicit offset and cursor-extracted offset respectively - `OtherErr` and `other_err!` are unchanged - `cast_length!` and `cast_int!` macros pass `0` to the new offset slot (the macros do not have stream-cursor access) Call-site migration: - All 84 affected files updated to pass an explicit offset - Most existing call sites pass `0` (the documented "offset unknown" sentinel) since the cursor is not always in lexical scope at the call site - Cursor-aware sites can opt in via the new `in: cursor` macro syntax - A workspace sweep upgrading `at: 0` to `in: cursor` where applicable is a follow-up Display output includes the offset when known: `invalid \`field\` at offset 47: reason` Refs #1257, #1120. Supersedes the cross-cutting `ErrorPosition` approach that was on this branch before today's review.
1 parent 9cb5439 commit 0e10d37

109 files changed

Lines changed: 818 additions & 831 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

crates/ironrdp-ainput/src/lib.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,8 @@ impl<'de> Decode<'de> for ServerPdu {
159159
fn decode(src: &mut ReadCursor<'de>) -> DecodeResult<Self> {
160160
ensure_fixed_part_size!(in: src);
161161

162-
let pdu_type =
163-
ServerPduType::from_u16(src.read_u16()).ok_or_else(|| invalid_field_err!("pduType", "invalid pdu type"))?;
162+
let pdu_type = ServerPduType::from_u16(src.read_u16())
163+
.ok_or_else(|| invalid_field_err!("pduType", "invalid pdu type", at: 0))?;
164164

165165
let server_pdu = match pdu_type {
166166
ServerPduType::Version => ServerPdu::Version(VersionPdu::decode(src)?),
@@ -256,8 +256,8 @@ impl<'de> Decode<'de> for ClientPdu {
256256
fn decode(src: &mut ReadCursor<'de>) -> DecodeResult<Self> {
257257
ensure_fixed_part_size!(in: src);
258258

259-
let pdu_type =
260-
ClientPduType::from_u16(src.read_u16()).ok_or_else(|| invalid_field_err!("pduType", "invalid pdu type"))?;
259+
let pdu_type = ClientPduType::from_u16(src.read_u16())
260+
.ok_or_else(|| invalid_field_err!("pduType", "invalid pdu type", at: 0))?;
261261

262262
let client_pdu = match pdu_type {
263263
ClientPduType::Mouse => ClientPdu::Mouse(MousePdu::decode(src)?),

crates/ironrdp-cfg/src/lib.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
mod target_addr;
2-
pub use target_addr::{ParseTargetAddrError, TargetAddr, TargetHost};
3-
42
use ironrdp_propertyset::PropertySet;
3+
pub use target_addr::{ParseTargetAddrError, TargetAddr, TargetHost};
54

65
/// Error returned when the `server port` property value is outside the valid port range (1–65535).
76
#[derive(Debug, Clone, Copy, PartialEq, Eq)]

crates/ironrdp-client/src/app.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,9 @@ use std::sync::Arc;
66
use std::time::Instant;
77

88
use anyhow::Context as _;
9+
use ironrdp::pdu::input::MousePdu;
910
use ironrdp::pdu::input::fast_path::FastPathInputEvent;
10-
use ironrdp::pdu::input::{MousePdu, mouse::PointerFlags};
11+
use ironrdp::pdu::input::mouse::PointerFlags;
1112
use raw_window_handle::{DisplayHandle, HasDisplayHandle as _};
1213
use smallvec::SmallVec;
1314
use tokio::sync::mpsc;

crates/ironrdp-cliprdr-format/src/bitmap.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -243,19 +243,19 @@ impl BitmapInfoHeader {
243243

244244
let width = src.read_i32();
245245
check_invariant(width != i32::MIN && width.abs() <= 10_000)
246-
.ok_or_else(|| invalid_field_err!("biWidth", "width is too big"))?;
246+
.ok_or_else(|| invalid_field_err!("biWidth", "width is too big", at: 0))?;
247247

248248
let height = src.read_i32();
249249
check_invariant(height != i32::MIN && height.abs() <= 10_000)
250-
.ok_or_else(|| invalid_field_err!("biHeight", "height is too big"))?;
250+
.ok_or_else(|| invalid_field_err!("biHeight", "height is too big", at: 0))?;
251251

252252
let planes = src.read_u16();
253253
if planes != 1 {
254-
return Err(invalid_field_err!("biPlanes", "invalid planes count"));
254+
return Err(invalid_field_err!("biPlanes", "invalid planes count", at: 0));
255255
}
256256

257257
let bit_count = src.read_u16();
258-
check_invariant(bit_count <= 32).ok_or_else(|| invalid_field_err!("biBitCount", "invalid bit count"))?;
258+
check_invariant(bit_count <= 32).ok_or_else(|| invalid_field_err!("biBitCount", "invalid bit count", at: 0))?;
259259

260260
let compression = BitmapCompression(src.read_u32());
261261
let size_image = src.read_u32();
@@ -320,7 +320,7 @@ impl<'a> Decode<'a> for BitmapInfoHeader {
320320
let size: usize = cast_int!("biSize", size)?;
321321

322322
if size != Self::FIXED_PART_SIZE {
323-
return Err(invalid_field_err!("biSize", "invalid V1 bitmap info header size"));
323+
return Err(invalid_field_err!("biSize", "invalid V1 bitmap info header size", at: 0));
324324
}
325325

326326
Ok(header)
@@ -406,7 +406,7 @@ impl<'a> Decode<'a> for BitmapV5Header {
406406
let size: usize = cast_int!("biSize", size)?;
407407

408408
if size != Self::FIXED_PART_SIZE {
409-
return Err(invalid_field_err!("biSize", "invalid V5 bitmap info header size"));
409+
return Err(invalid_field_err!("biSize", "invalid V5 bitmap info header size", at: 0));
410410
}
411411

412412
let red_mask = src.read_u32();

crates/ironrdp-cliprdr/src/pdu/capabilities.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -170,10 +170,8 @@ impl<'de> Decode<'de> for CapabilitySet {
170170
let general = GeneralCapabilitySet::decode(src)?;
171171
Ok(Self::General(general))
172172
}
173-
_ => Err(invalid_field_err!(
174-
"capabilitySetType",
175-
"invalid clipboard capability set type"
176-
)),
173+
_ => Err(invalid_field_err!( "capabilitySetType",
174+
"invalid clipboard capability set type", at: 0)),
177175
}
178176
}
179177
}

crates/ironrdp-cliprdr/src/pdu/client_temporary_directory.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ impl ClientTemporaryDirectory<'_> {
5252
let mut cursor = ReadCursor::new(&self.path_buffer);
5353

5454
read_string_from_cursor(&mut cursor, CharacterSet::Unicode, true)
55-
.map_err(|_| invalid_field_err!("wszTempDir", "failed to decode temp dir path"))
55+
.map_err(|_| invalid_field_err!("wszTempDir", "failed to decode temp dir path", at: 0))
5656
}
5757
}
5858

crates/ironrdp-cliprdr/src/pdu/file_contents.rs

Lines changed: 14 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -129,18 +129,15 @@ impl<'a> FileContentsResponse<'a> {
129129
/// Should not panic - the try_into conversion is guaranteed to succeed after length validation.
130130
pub fn data_as_size(&self) -> DecodeResult<u64> {
131131
if self.data.len() != 8 {
132-
return Err(invalid_field_err!(
133-
"requestedFileContentsData",
134-
"SIZE response must be exactly 8 bytes per MS-RDPECLIP 2.2.5.4"
135-
));
132+
return Err(invalid_field_err!( "requestedFileContentsData",
133+
"SIZE response must be exactly 8 bytes per MS-RDPECLIP 2.2.5.4", at: 0));
136134
}
137135

138136
// Per length check above, this conversion is infallible.
139-
let chunk: [u8; 8] = self
140-
.data
141-
.as_ref()
142-
.try_into()
143-
.map_err(|_| invalid_field_err!("requestedFileContentsData", "SIZE response data is not 8 bytes"))?;
137+
let chunk: [u8; 8] =
138+
self.data.as_ref().try_into().map_err(
139+
|_| invalid_field_err!("requestedFileContentsData", "SIZE response data is not 8 bytes", at: 0),
140+
)?;
144141
Ok(u64::from_le_bytes(chunk))
145142
}
146143
}
@@ -182,7 +179,7 @@ impl<'de> Decode<'de> for FileContentsResponse<'de> {
182179
ensure_size!(in: src, size: header.data_length());
183180

184181
if header.data_length() < Self::FIXED_PART_SIZE {
185-
return Err(invalid_field_err!("requestedFileContentsData", "invalid data size"));
182+
return Err(invalid_field_err!("requestedFileContentsData", "invalid data size", at: 0));
186183
};
187184

188185
let data_size = header.data_length() - Self::FIXED_PART_SIZE;
@@ -287,28 +284,22 @@ impl<'de> Decode<'de> for FileContentsRequest {
287284

288285
// [MS-RDPECLIP] 2.2.5.3 - Validate lindex is non-negative
289286
if index < 0 {
290-
return Err(invalid_field_err!(
291-
"lindex",
292-
"file index must be non-negative per MS-RDPECLIP 2.2.5.3"
293-
));
287+
return Err(invalid_field_err!( "lindex",
288+
"file index must be non-negative per MS-RDPECLIP 2.2.5.3", at: 0));
294289
}
295290

296291
// [MS-RDPECLIP] 2.2.5.3 - Validate flags are spec-compliant
297-
flags.validate().map_err(|e| invalid_field_err!("dwFlags", e))?;
292+
flags.validate().map_err(|e| invalid_field_err!("dwFlags", e, at: 0))?;
298293

299294
// [MS-RDPECLIP] 2.2.5.3 - Validate SIZE request constraints
300295
if flags.contains(FileContentsFlags::SIZE) {
301296
if requested_size != 8 {
302-
return Err(invalid_field_err!(
303-
"cbRequested",
304-
"SIZE request must have cbRequested=8 per MS-RDPECLIP 2.2.5.3"
305-
));
297+
return Err(invalid_field_err!( "cbRequested",
298+
"SIZE request must have cbRequested=8 per MS-RDPECLIP 2.2.5.3", at: 0));
306299
}
307300
if position != 0 {
308-
return Err(invalid_field_err!(
309-
"position",
310-
"SIZE request must have position=0 per MS-RDPECLIP 2.2.5.3"
311-
));
301+
return Err(invalid_field_err!( "position",
302+
"SIZE request must have position=0 per MS-RDPECLIP 2.2.5.3", at: 0));
312303
}
313304
}
314305

crates/ironrdp-cliprdr/src/pdu/format_data/file_list.rs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -184,10 +184,8 @@ impl Encode for FileDescriptor {
184184
// UTF-16 encoding: each code unit is 2 bytes, plus 2 bytes for null terminator.
185185
let encoded_len = wire_name.encode_utf16().count() * 2 + 2;
186186
if NAME_LENGTH < encoded_len {
187-
return Err(ironrdp_core::invalid_field_err!(
188-
"cFileName",
189-
"encoded wire name exceeds NAME_LENGTH (520 bytes)"
190-
));
187+
return Err(ironrdp_core::invalid_field_err!( "cFileName",
188+
"encoded wire name exceeds NAME_LENGTH (520 bytes)", at: 0));
191189
}
192190

193191
let written = encode_string(dst.remaining_mut(), &wire_name, CharacterSet::Unicode, true)?;
@@ -297,10 +295,8 @@ impl<'de> Decode<'de> for PackedFileList {
297295
let file_count: usize = cast_length!(Self::NAME, "cItems", src.read_u32())?;
298296

299297
if MAX_FILE_COUNT < file_count {
300-
return Err(ironrdp_core::invalid_field_err!(
301-
"cItems",
302-
"file count exceeds maximum of 100000"
303-
));
298+
return Err(ironrdp_core::invalid_field_err!( "cItems",
299+
"file count exceeds maximum of 100000", at: 0));
304300
}
305301

306302
// Cap pre-allocation against remaining bytes to prevent OOM from

crates/ironrdp-cliprdr/src/pdu/format_list.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -449,7 +449,7 @@ impl<'de> Decode<'de> for FormatListResponse {
449449
match header.message_flags {
450450
ClipboardPduFlags::RESPONSE_OK => Ok(FormatListResponse::Ok),
451451
ClipboardPduFlags::RESPONSE_FAIL => Ok(FormatListResponse::Fail),
452-
_ => Err(invalid_field_err!("msgFlags", "invalid format list message flags")),
452+
_ => Err(invalid_field_err!("msgFlags", "invalid format list message flags", at: 0)),
453453
}
454454
}
455455
}

crates/ironrdp-cliprdr/src/pdu/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ impl<'de> Decode<'de> for ClipboardPdu<'de> {
243243
MSG_TYPE_FILE_CONTENTS_RESPONSE => ClipboardPdu::FileContentsResponse(FileContentsResponse::decode(src)?),
244244
MSG_TYPE_LOCK_CLIPDATA => ClipboardPdu::LockData(LockDataId::decode(src)?),
245245
MSG_TYPE_UNLOCK_CLIPDATA => ClipboardPdu::UnlockData(LockDataId::decode(src)?),
246-
_ => return Err(invalid_field_err!("msgType", "unknown clipboard PDU type")),
246+
_ => return Err(invalid_field_err!("msgType", "unknown clipboard PDU type", at: 0)),
247247
};
248248

249249
Ok(pdu)

0 commit comments

Comments
 (0)