Skip to content

Commit 31b3532

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 Devolutions#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 Devolutions#1257, Devolutions#1120. Supersedes the cross-cutting `ErrorPosition` approach that was on this branch before today's review.
1 parent 9cb5439 commit 31b3532

100 files changed

Lines changed: 695 additions & 657 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!(at: 0, "pduType", "invalid pdu type"))?;
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!(at: 0, "pduType", "invalid pdu type"))?;
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!(at: 0, "biWidth", "width is too big"))?;
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!(at: 0, "biHeight", "height is too big"))?;
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!(at: 0, "biPlanes", "invalid planes count"));
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!(at: 0, "biBitCount", "invalid bit count"))?;
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!(at: 0, "biSize", "invalid V1 bitmap info header size"));
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!(at: 0, "biSize", "invalid V5 bitmap info header size"));
410410
}
411411

412412
let red_mask = src.read_u32();

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,8 +170,7 @@ 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",
173+
_ => Err(invalid_field_err!(at: 0, "capabilitySetType",
175174
"invalid clipboard capability set type"
176175
)),
177176
}

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!(at: 0, "wszTempDir", "failed to decode temp dir path"))
5656
}
5757
}
5858

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

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -129,18 +129,16 @@ 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",
132+
return Err(invalid_field_err!(at: 0, "requestedFileContentsData",
134133
"SIZE response must be exactly 8 bytes per MS-RDPECLIP 2.2.5.4"
135134
));
136135
}
137136

138137
// 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"))?;
138+
let chunk: [u8; 8] =
139+
self.data.as_ref().try_into().map_err(
140+
|_| invalid_field_err!(at: 0, "requestedFileContentsData", "SIZE response data is not 8 bytes"),
141+
)?;
144142
Ok(u64::from_le_bytes(chunk))
145143
}
146144
}
@@ -182,7 +180,7 @@ impl<'de> Decode<'de> for FileContentsResponse<'de> {
182180
ensure_size!(in: src, size: header.data_length());
183181

184182
if header.data_length() < Self::FIXED_PART_SIZE {
185-
return Err(invalid_field_err!("requestedFileContentsData", "invalid data size"));
183+
return Err(invalid_field_err!(at: 0, "requestedFileContentsData", "invalid data size"));
186184
};
187185

188186
let data_size = header.data_length() - Self::FIXED_PART_SIZE;
@@ -287,26 +285,23 @@ impl<'de> Decode<'de> for FileContentsRequest {
287285

288286
// [MS-RDPECLIP] 2.2.5.3 - Validate lindex is non-negative
289287
if index < 0 {
290-
return Err(invalid_field_err!(
291-
"lindex",
288+
return Err(invalid_field_err!(at: 0, "lindex",
292289
"file index must be non-negative per MS-RDPECLIP 2.2.5.3"
293290
));
294291
}
295292

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

299296
// [MS-RDPECLIP] 2.2.5.3 - Validate SIZE request constraints
300297
if flags.contains(FileContentsFlags::SIZE) {
301298
if requested_size != 8 {
302-
return Err(invalid_field_err!(
303-
"cbRequested",
299+
return Err(invalid_field_err!(at: 0, "cbRequested",
304300
"SIZE request must have cbRequested=8 per MS-RDPECLIP 2.2.5.3"
305301
));
306302
}
307303
if position != 0 {
308-
return Err(invalid_field_err!(
309-
"position",
304+
return Err(invalid_field_err!(at: 0, "position",
310305
"SIZE request must have position=0 per MS-RDPECLIP 2.2.5.3"
311306
));
312307
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,7 @@ 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",
187+
return Err(ironrdp_core::invalid_field_err!(at: 0, "cFileName",
189188
"encoded wire name exceeds NAME_LENGTH (520 bytes)"
190189
));
191190
}
@@ -297,8 +296,7 @@ impl<'de> Decode<'de> for PackedFileList {
297296
let file_count: usize = cast_length!(Self::NAME, "cItems", src.read_u32())?;
298297

299298
if MAX_FILE_COUNT < file_count {
300-
return Err(ironrdp_core::invalid_field_err!(
301-
"cItems",
299+
return Err(ironrdp_core::invalid_field_err!(at: 0, "cItems",
302300
"file count exceeds maximum of 100000"
303301
));
304302
}

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!(at: 0, "msgFlags", "invalid format list message flags")),
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!(at: 0, "msgType", "unknown clipboard PDU type")),
247247
};
248248

249249
Ok(pdu)

crates/ironrdp-core/src/decode.rs

Lines changed: 61 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,16 @@ pub type DecodeResult<T> = Result<T, DecodeError>;
1515
pub type DecodeError = ironrdp_error::Error<DecodeErrorKind>;
1616

1717
/// Enum representing different kinds of decode errors.
18+
///
19+
/// Variants that arise while reading the wire stream carry a non-optional
20+
/// `offset` field set to the cursor position at which the error was detected.
21+
/// This makes fuzz-crash triage tractable without classifier traits or
22+
/// position-aware metadata on the foundational error type.
23+
///
24+
/// Callers that produce these variants from contexts without a stream cursor
25+
/// pass `0`, which is documented in the [`NotEnoughBytesErr`], [`InvalidFieldErr`],
26+
/// [`UnexpectedMessageTypeErr`], [`UnsupportedVersionErr`], and [`UnsupportedValueErr`]
27+
/// traits as "offset unknown."
1828
#[non_exhaustive]
1929
#[derive(Clone, Debug)]
2030
pub enum DecodeErrorKind {
@@ -24,23 +34,31 @@ pub enum DecodeErrorKind {
2434
received: usize,
2535
/// Number of bytes expected.
2636
expected: usize,
37+
/// Byte offset in the input stream where the shortage was detected.
38+
offset: usize,
2739
},
2840
/// Error when a field is invalid.
2941
InvalidField {
3042
/// Name of the invalid field.
3143
field: &'static str,
3244
/// Reason for invalidity.
3345
reason: &'static str,
46+
/// Byte offset in the input stream where the invalid field was decoded.
47+
offset: usize,
3448
},
3549
/// Error when an unexpected message type is encountered.
3650
UnexpectedMessageType {
3751
/// The unexpected message type received.
3852
got: u8,
53+
/// Byte offset in the input stream where the unexpected type was read.
54+
offset: usize,
3955
},
4056
/// Error when an unsupported version is encountered.
4157
UnsupportedVersion {
4258
/// The unsupported version received.
4359
got: u8,
60+
/// Byte offset in the input stream where the unsupported version was read.
61+
offset: usize,
4462
},
4563
/// Error when an unsupported value is encountered (with allocation feature).
4664
#[cfg(feature = "alloc")]
@@ -49,14 +67,22 @@ pub enum DecodeErrorKind {
4967
name: &'static str,
5068
/// The unsupported value.
5169
value: String,
70+
/// Byte offset in the input stream where the unsupported value was read.
71+
offset: usize,
5272
},
5373
/// Error when an unsupported value is encountered (without allocation feature).
5474
#[cfg(not(feature = "alloc"))]
5575
UnsupportedValue {
5676
/// Name of the unsupported value.
5777
name: &'static str,
78+
/// Byte offset in the input stream where the unsupported value was read.
79+
offset: usize,
5880
},
5981
/// Generic error for other cases.
82+
///
83+
/// Does not carry an offset: producers of this variant typically do not
84+
/// have stream-cursor access, and the variant exists precisely for those
85+
/// cases.
6086
Other {
6187
/// Description of the error.
6288
description: &'static str,
@@ -69,26 +95,30 @@ impl core::error::Error for DecodeErrorKind {}
6995
impl fmt::Display for DecodeErrorKind {
7096
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
7197
match self {
72-
Self::NotEnoughBytes { received, expected } => write!(
98+
Self::NotEnoughBytes {
99+
received,
100+
expected,
101+
offset,
102+
} => write!(
73103
f,
74-
"not enough bytes provided to decode: received {received} bytes, expected {expected} bytes"
104+
"not enough bytes provided to decode at offset {offset}: received {received} bytes, expected {expected} bytes"
75105
),
76-
Self::InvalidField { field, reason } => {
77-
write!(f, "invalid `{field}`: {reason}")
106+
Self::InvalidField { field, reason, offset } => {
107+
write!(f, "invalid `{field}` at offset {offset}: {reason}")
78108
}
79-
Self::UnexpectedMessageType { got } => {
80-
write!(f, "invalid message type ({got})")
109+
Self::UnexpectedMessageType { got, offset } => {
110+
write!(f, "invalid message type ({got}) at offset {offset}")
81111
}
82-
Self::UnsupportedVersion { got } => {
83-
write!(f, "unsupported version ({got})")
112+
Self::UnsupportedVersion { got, offset } => {
113+
write!(f, "unsupported version ({got}) at offset {offset}")
84114
}
85115
#[cfg(feature = "alloc")]
86-
Self::UnsupportedValue { name, value } => {
87-
write!(f, "unsupported {name} ({value})")
116+
Self::UnsupportedValue { name, value, offset } => {
117+
write!(f, "unsupported {name} ({value}) at offset {offset}")
88118
}
89119
#[cfg(not(feature = "alloc"))]
90-
Self::UnsupportedValue { name } => {
91-
write!(f, "unsupported {name}")
120+
Self::UnsupportedValue { name, offset } => {
121+
write!(f, "unsupported {name} at offset {offset}")
92122
}
93123
Self::Other { description } => {
94124
write!(f, "other ({description})")
@@ -98,37 +128,44 @@ impl fmt::Display for DecodeErrorKind {
98128
}
99129

100130
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 })
131+
fn not_enough_bytes(context: &'static str, received: usize, expected: usize, offset: usize) -> Self {
132+
Self::new(
133+
context,
134+
DecodeErrorKind::NotEnoughBytes {
135+
received,
136+
expected,
137+
offset,
138+
},
139+
)
103140
}
104141
}
105142

106143
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 })
144+
fn invalid_field(context: &'static str, field: &'static str, reason: &'static str, offset: usize) -> Self {
145+
Self::new(context, DecodeErrorKind::InvalidField { field, reason, offset })
109146
}
110147
}
111148

112149
impl UnexpectedMessageTypeErr for DecodeError {
113-
fn unexpected_message_type(context: &'static str, got: u8) -> Self {
114-
Self::new(context, DecodeErrorKind::UnexpectedMessageType { got })
150+
fn unexpected_message_type(context: &'static str, got: u8, offset: usize) -> Self {
151+
Self::new(context, DecodeErrorKind::UnexpectedMessageType { got, offset })
115152
}
116153
}
117154

118155
impl UnsupportedVersionErr for DecodeError {
119-
fn unsupported_version(context: &'static str, got: u8) -> Self {
120-
Self::new(context, DecodeErrorKind::UnsupportedVersion { got })
156+
fn unsupported_version(context: &'static str, got: u8, offset: usize) -> Self {
157+
Self::new(context, DecodeErrorKind::UnsupportedVersion { got, offset })
121158
}
122159
}
123160

124161
impl UnsupportedValueErr for DecodeError {
125162
#[cfg(feature = "alloc")]
126-
fn unsupported_value(context: &'static str, name: &'static str, value: String) -> Self {
127-
Self::new(context, DecodeErrorKind::UnsupportedValue { name, value })
163+
fn unsupported_value(context: &'static str, name: &'static str, value: String, offset: usize) -> Self {
164+
Self::new(context, DecodeErrorKind::UnsupportedValue { name, value, offset })
128165
}
129166
#[cfg(not(feature = "alloc"))]
130-
fn unsupported_value(context: &'static str, name: &'static str) -> Self {
131-
Self::new(context, DecodeErrorKind::UnsupportedValue { name })
167+
fn unsupported_value(context: &'static str, name: &'static str, offset: usize) -> Self {
168+
Self::new(context, DecodeErrorKind::UnsupportedValue { name, offset })
132169
}
133170
}
134171

0 commit comments

Comments
 (0)