Skip to content

Commit 381b3ae

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 381b3ae

100 files changed

Lines changed: 804 additions & 817 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-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)

crates/ironrdp-core/src/decode.rs

Lines changed: 74 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,16 @@ pub type DecodeResult<T> = Result<T, DecodeError>;
1414
/// An error type specifically for encoding operations, wrapping an [`DecodeErrorKind`].
1515
pub type DecodeError = ironrdp_error::Error<DecodeErrorKind>;
1616

17-
/// Enum representing different kinds of decode errors.
17+
/// Structured decode errors carry the fields needed to describe the failure,
18+
/// including a byte `offset` when the error can be associated with
19+
/// a position in the input stream.
20+
///
21+
/// The `offset` is the cursor position at, or nearest to, where the error was
22+
/// detected. Producers without a stream cursor (a `try_from` on a primitive,
23+
/// a constructor, a validator) pass `0`.
24+
///
25+
/// [`DecodeErrorKind::Other`] is reserved for errors that do not fit one of the
26+
/// structured variants and therefore does not carry an offset.
1827
#[non_exhaustive]
1928
#[derive(Clone, Debug)]
2029
pub enum DecodeErrorKind {
@@ -24,23 +33,40 @@ pub enum DecodeErrorKind {
2433
received: usize,
2534
/// Number of bytes expected.
2635
expected: usize,
36+
/// Byte offset in the input stream where the shortage was detected.
37+
///
38+
/// `0` indicates that the producer had no stream cursor in scope
39+
/// (a `try_from` on a primitive, a constructor, a validator).
40+
offset: usize,
2741
},
2842
/// Error when a field is invalid.
2943
InvalidField {
3044
/// Name of the invalid field.
3145
field: &'static str,
3246
/// Reason for invalidity.
3347
reason: &'static str,
48+
/// Byte offset in the input stream where the invalid field was decoded.
49+
///
50+
/// `0` indicates that the producer had no stream cursor in scope.
51+
offset: usize,
3452
},
3553
/// Error when an unexpected message type is encountered.
3654
UnexpectedMessageType {
3755
/// The unexpected message type received.
3856
got: u8,
57+
/// Byte offset in the input stream where the unexpected type was read.
58+
///
59+
/// `0` indicates that the producer had no stream cursor in scope.
60+
offset: usize,
3961
},
4062
/// Error when an unsupported version is encountered.
4163
UnsupportedVersion {
4264
/// The unsupported version received.
4365
got: u8,
66+
/// Byte offset in the input stream where the unsupported version was read.
67+
///
68+
/// `0` indicates that the producer had no stream cursor in scope.
69+
offset: usize,
4470
},
4571
/// Error when an unsupported value is encountered (with allocation feature).
4672
#[cfg(feature = "alloc")]
@@ -49,14 +75,26 @@ pub enum DecodeErrorKind {
4975
name: &'static str,
5076
/// The unsupported value.
5177
value: String,
78+
/// Byte offset in the input stream where the unsupported value was read.
79+
///
80+
/// `0` indicates that the producer had no stream cursor in scope.
81+
offset: usize,
5282
},
5383
/// Error when an unsupported value is encountered (without allocation feature).
5484
#[cfg(not(feature = "alloc"))]
5585
UnsupportedValue {
5686
/// Name of the unsupported value.
5787
name: &'static str,
88+
/// Byte offset in the input stream where the unsupported value was read.
89+
///
90+
/// `0` indicates that the producer had no stream cursor in scope.
91+
offset: usize,
5892
},
5993
/// Generic error for other cases.
94+
///
95+
/// Does not carry an offset: producers of this variant typically do not
96+
/// have stream-cursor access, and the variant exists precisely for those
97+
/// cases.
6098
Other {
6199
/// Description of the error.
62100
description: &'static str,
@@ -69,26 +107,30 @@ impl core::error::Error for DecodeErrorKind {}
69107
impl fmt::Display for DecodeErrorKind {
70108
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
71109
match self {
72-
Self::NotEnoughBytes { received, expected } => write!(
110+
Self::NotEnoughBytes {
111+
received,
112+
expected,
113+
offset,
114+
} => write!(
73115
f,
74-
"not enough bytes provided to decode: received {received} bytes, expected {expected} bytes"
116+
"not enough bytes provided to decode at offset {offset}: received {received} bytes, expected {expected} bytes"
75117
),
76-
Self::InvalidField { field, reason } => {
77-
write!(f, "invalid `{field}`: {reason}")
118+
Self::InvalidField { field, reason, offset } => {
119+
write!(f, "invalid `{field}` at offset {offset}: {reason}")
78120
}
79-
Self::UnexpectedMessageType { got } => {
80-
write!(f, "invalid message type ({got})")
121+
Self::UnexpectedMessageType { got, offset } => {
122+
write!(f, "invalid message type ({got}) at offset {offset}")
81123
}
82-
Self::UnsupportedVersion { got } => {
83-
write!(f, "unsupported version ({got})")
124+
Self::UnsupportedVersion { got, offset } => {
125+
write!(f, "unsupported version ({got}) at offset {offset}")
84126
}
85127
#[cfg(feature = "alloc")]
86-
Self::UnsupportedValue { name, value } => {
87-
write!(f, "unsupported {name} ({value})")
128+
Self::UnsupportedValue { name, value, offset } => {
129+
write!(f, "unsupported {name} ({value}) at offset {offset}")
88130
}
89131
#[cfg(not(feature = "alloc"))]
90-
Self::UnsupportedValue { name } => {
91-
write!(f, "unsupported {name}")
132+
Self::UnsupportedValue { name, offset } => {
133+
write!(f, "unsupported {name} at offset {offset}")
92134
}
93135
Self::Other { description } => {
94136
write!(f, "other ({description})")
@@ -98,37 +140,44 @@ impl fmt::Display for DecodeErrorKind {
98140
}
99141

100142
impl NotEnoughBytesErr for DecodeError {
101-
fn not_enough_bytes(context: &'static str, received: usize, expected: usize) -> Self {
102-
Self::new(context, DecodeErrorKind::NotEnoughBytes { received, expected })
143+
fn not_enough_bytes(context: &'static str, received: usize, expected: usize, offset: usize) -> Self {
144+
Self::new(
145+
context,
146+
DecodeErrorKind::NotEnoughBytes {
147+
received,
148+
expected,
149+
offset,
150+
},
151+
)
103152
}
104153
}
105154

106155
impl InvalidFieldErr for DecodeError {
107-
fn invalid_field(context: &'static str, field: &'static str, reason: &'static str) -> Self {
108-
Self::new(context, DecodeErrorKind::InvalidField { field, reason })
156+
fn invalid_field(context: &'static str, field: &'static str, reason: &'static str, offset: usize) -> Self {
157+
Self::new(context, DecodeErrorKind::InvalidField { field, reason, offset })
109158
}
110159
}
111160

112161
impl UnexpectedMessageTypeErr for DecodeError {
113-
fn unexpected_message_type(context: &'static str, got: u8) -> Self {
114-
Self::new(context, DecodeErrorKind::UnexpectedMessageType { got })
162+
fn unexpected_message_type(context: &'static str, got: u8, offset: usize) -> Self {
163+
Self::new(context, DecodeErrorKind::UnexpectedMessageType { got, offset })
115164
}
116165
}
117166

118167
impl UnsupportedVersionErr for DecodeError {
119-
fn unsupported_version(context: &'static str, got: u8) -> Self {
120-
Self::new(context, DecodeErrorKind::UnsupportedVersion { got })
168+
fn unsupported_version(context: &'static str, got: u8, offset: usize) -> Self {
169+
Self::new(context, DecodeErrorKind::UnsupportedVersion { got, offset })
121170
}
122171
}
123172

124173
impl UnsupportedValueErr for DecodeError {
125174
#[cfg(feature = "alloc")]
126-
fn unsupported_value(context: &'static str, name: &'static str, value: String) -> Self {
127-
Self::new(context, DecodeErrorKind::UnsupportedValue { name, value })
175+
fn unsupported_value(context: &'static str, name: &'static str, value: String, offset: usize) -> Self {
176+
Self::new(context, DecodeErrorKind::UnsupportedValue { name, value, offset })
128177
}
129178
#[cfg(not(feature = "alloc"))]
130-
fn unsupported_value(context: &'static str, name: &'static str) -> Self {
131-
Self::new(context, DecodeErrorKind::UnsupportedValue { name })
179+
fn unsupported_value(context: &'static str, name: &'static str, offset: usize) -> Self {
180+
Self::new(context, DecodeErrorKind::UnsupportedValue { name, offset })
132181
}
133182
}
134183

0 commit comments

Comments
 (0)