feat(error): record byte offset on decode and encode error variants

[Greg Lamberson (glamberson)]

Summary

Records a byte offset on every DecodeErrorKind and EncodeErrorKind variant that can know one, so decode and encode errors surface the position in the input stream where the failure was detected. Reshaped from the original design in this PR after review feedback; see "Reshape history" below if you'd reviewed the earlier shape.

Contributes to the structured-fuzzing roadmap in #1120 by giving fuzz-crash triage and Wireshark-style malformed-PDU reporting the byte-offset dimension that source Location (#1262) alone does not provide.

API

Variants that gain offset: usize:

• DecodeErrorKind::NotEnoughBytes { received, expected, offset }
• DecodeErrorKind::InvalidField { field, reason, offset }
• DecodeErrorKind::UnexpectedMessageType { got, offset }
• DecodeErrorKind::UnsupportedVersion { name, got, offset }
• DecodeErrorKind::UnsupportedValue { name, expected, got, offset }
• EncodeErrorKind mirrors the same shape for the encode side

*::Other is unchanged — producers of Other typically lack stream-cursor access and the variant exists for those cases. OtherErr and other_err! keep their current signatures.

Five traits take an additional offset: usize parameter:

• NotEnoughBytesErr::not_enough_bytes(context, received, expected, offset)
• InvalidFieldErr::invalid_field(context, field, reason, offset)
• UnexpectedMessageTypeErr::unexpected_message_type(context, got, offset)
• UnsupportedVersionErr::unsupported_version(context, name, got, offset)
• UnsupportedValueErr::unsupported_value(context, name, expected, got, offset)

invalid_field_err_with_source gains an offset parameter in the same position. The matching *_err! macros expose two prefix forms:

• in: $cursor, — extracts $cursor.pos() at the call site (preferred when a ReadCursor or WriteCursor is in scope)
• at: $offset, — passes a literal or computed value (use 0 when the producer has no stream-cursor access)

Display behavior

The Display impl includes the offset when the variant carries one:

invalid `serverRandom` at offset 47: bad length
not enough bytes at offset 12: expected 4, received 2
unexpected message type 3 at offset 35
unsupported version 175 at offset 12 for "x224"
unsupported value 0x80 at offset 5 for "tpdu code" (expected DATA)

Other and the [ctx @ file:line] context+location prefix from #1262 are unchanged.

Reshape history

The earlier version of this PR (filed 2026-05-12) added an Option<ErrorPosition> slot to Error<Kind>'s ErrorMeta with composable at_offset / in_field builders. Reshaped after review feedback on two grounds:

1. The foundational ironrdp-error crate stays kind-agnostic. Position metadata that only applies to decode and encode surfaces should live on those variants, not in ErrorMeta.
2. Error::set_context already covers the "which symbolic field" question. No parallel in_field channel is introduced.

offset: usize is therefore non-Optional on every variant that can know one, and ErrorMeta is unchanged from #1269.

Call-site migration

All 84 affected files updated to pass an explicit offset. Most existing call sites pass at: 0, (the documented "offset unknown" sentinel) in this PR; an audit-driven workspace sweep upgrading those to in: <cursor>, is being prepared as a separate follow-up PR so this one stays a reviewable foundational change.

Two patterns to note:

• cast_length! and cast_int! now require the in: or at: prefix; the bare forms are removed. This is intentional: the bare forms silently passed 0, which was the bug being fixed.
• OtherErr and other_err! are unchanged.

Diff

Single commit e935058c on top of master (master already includes #1269's ErrorMeta boxing). 94 files changed, +494 / -247.

CI

cargo xtask check fmt | lints | locks | typos | tests all pass. cargo check -p ironrdp-error builds clean under --all-features, --no-default-features (no_std), and --no-default-features --features alloc.

Refs #1257 (Change 2), #1120.

[Benoît Cortier (CBenoit)]

Thanks for pushing on triage ergonomics. The byte-offset signal is genuinely useful for fuzz-crash analysis, and #[cold] + #[must_use] on the builders is the right call. That said, I'd like to push back on the layering before this lands.

The position slot is at the wrong layer: ironrdp-error is the foundational, kind-agnostic error crate. Error<Kind> is parameterized over every error kind in the workspace such as connection state, channel routing, CredSSP, future kinds we haven't written yet. Almost none of those have a meaningful "byte offset in the input stream", and the ones that have a "field being processed" already carry it as structured data on the kind itself (DecodeErrorKind::InvalidField { field, .. }, UnsupportedValue { name, .. }).

We'd be paying for a metadata slot on every Error<Kind> in the workspace to serve essentially one consumer: the decode side. Encode-side doesn't really qualify: there is no input stream, and if the audit-pass direction from #1267 plays out, the encode-side bug-shaped variants likely go away anyway.

Here are two alternatives worth weighing.

Option A: extend the enum. Put the offset on the DecodeErrorKind variants that can produce it:

pub enum DecodeErrorKind {
    NotEnoughBytes { received: usize, expected: usize, offset: usize },
    InvalidField   { field: &'static str, reason: &'static str, offset: usize },
    UnexpectedMessageType { got: u8, offset: usize },
    UnsupportedVersion    { got: u8, offset: usize },
    UnsupportedValue { name: &'static str, value: String, offset: usize },
    Other { description: &'static str },
}

Plus a small accessor on the kind, and ensure_size! / cursor read helpers populating it from cursor.pos() (which is where the offset is naturally known). The offset is non-optional on every variant that can know one; Other and future kinds without a stream cursor never grow the field; although it could still make sense to include the offset we were at when the error occurred.

Option B: promote Kind from enum to struct. The Kind generic parameter on Error<Kind> doesn't have to be an enum. We could redefine DecodeErrorKind as a struct holding a sub-kind enum plus the metadata:

pub struct DecodeErrorKind {
    pub kind: DecodeErrorSubKind, // the current enum
    pub offset: usize,
    pub field: Option<&'static str>,
}

Since every decode error happens at a known cursor position, offset is plain usize: no Option dance. This threads offset and field uniformly across every sub-kind without per-variant churn, and keeps the metadata where it semantically belongs (decode-specific) rather than on the foundational Error<Kind>. It's basically the PR's design, but scoped to the one kind that wants it.

Both options are better than the cross-cutting version because they keep the cost on the consumer that benefits. Option A is more honest typing per-variant; Option B is less code churn and lets in_field propagate uniformly through ? if we want it.

On in_field specifically: The dotted-path use case ("ServerSecurityData.serverRandom") for nested PDU navigation across ?-propagation is the one thing Option A doesn't cover cleanly. But that's a propagation-context concern, and we already have Error::set_context for exactly that. I'd like to see a concrete argument for why a parallel structured channel beats enriching the existing context slot before we commit to a new mechanism. Option B sidesteps the question by keeping it scoped to decode, which is also fine.

Happy to discuss.

[Greg Lamberson (glamberson)]

You're right on the layering, and Option A is the right shape: per-variant offset fields make the type system enforce that variants which can know an offset always have one, and Other plus future stream-less kinds never grow the field.

I'll reshape and force-push. The migration cost lands on every invalid_field_err! / ensure_size! / cursor read helper that produces a DecodeErrorKind, but the offset is already available via cursor.pos() at every one of those call sites, so the change is mechanical.

On in_field specifically: agreed that enriching the existing context slot is the right approach rather than a parallel structured channel. I'll drop in_field from the reshape; the dotted-path use case ("ServerSecurityData.serverRandom") is exactly what set_context is for.

[Greg Lamberson (glamberson)]

Reshape pushed: e935058c.

Per the prior reply (and your Option A direction), this drops the ErrorMeta-level ErrorPosition slot entirely and gives every DecodeErrorKind / EncodeErrorKind variant that can know an offset a non-Optional offset: usize:

• NotEnoughBytes { received, expected, offset }
• InvalidField { field, reason, offset }
• UnexpectedMessageType { got, offset }
• UnsupportedVersion { name, got, offset }
• UnsupportedValue { name, expected, got, offset }

Other is unchanged on both sides (OtherErr and other_err! have no offset parameter) because that variant exists for producers without stream-cursor access. The in_field builder is gone; Error::set_context already covers the dotted-path use case.

Five traits in ironrdp-error (NotEnoughBytesErr, InvalidFieldErr, UnexpectedMessageTypeErr, UnsupportedVersionErr, UnsupportedValueErr) gain the offset parameter. The matching *_err! macros now require a position prefix: in: $cursor, (extracts $cursor.pos()) or at: $offset, (literal). cast_length! and cast_int! lost their bare forms for the same reason: the bare forms silently passed 0, which was the bug being fixed.

Most call sites in this PR pass at: 0, to keep the diff focused on the foundational shape. An audit-driven workspace sweep upgrading those to in: <cursor>, is prepared as a separate follow-up so this PR stays reviewable. PR body has been rewritten to describe the shipped shape (Reshape history section preserved for prior reviewers).

By now you might start to realize I don't like compromising. I prefer to go all in. So here we go again. The follow-up sweep is not a minimal pass: it classifies every callsite individually by looking at the immediate enclosing function's signature. 333 sites are upgraded to in: <cursor> using the parameter's actual name (src, dst, cursor, or stream). 57 are left at at: 0 because their enclosing function has no buffer in scope (try_from impls on enum discriminants, new/with_* constructors, accessors on already-decoded structs, dimension validators). 9 sites that lived inside local helper macros (validate_dimensions! in displaycontrol, check_masks_alignment! in pointer) were handled by adapting those macros to take an explicit offset argument. 9 direct-function-call sites the macro-form audit missed were upgraded the same way after a second pass. Total reviewed: 408.

cargo xtask check fmt | lints | locks | typos | tests green on e935058c. cargo check -p ironrdp-error clean under --all-features, --no-default-features, and --no-default-features --features alloc.

[Copilot]

Pull request overview

This PR adds a non-Optional offset: usize field to most DecodeErrorKind and EncodeErrorKind variants in ironrdp-core, surfacing the byte position in the input/output stream where decode/encode failures were detected, and renders it in the Display output. It threads the new parameter through five error-construction traits and their corresponding macros, which gain new in: $cursor, and at: $offset, prefix forms. Almost every call site in the workspace is migrated to pass at: 0 as a "offset unknown" sentinel, with a follow-up PR planned to upgrade those to in: <cursor> after an audit.

Changes:

• Decode/EncodeErrorKind variants (NotEnoughBytes, InvalidField, UnexpectedMessageType, UnsupportedVersion, UnsupportedValue) gain a non-Optional offset: usize; Other is unchanged. Display strings now include at offset {offset}.
• The five *Err traits and their helper functions (including invalid_field_err_with_source) take an extra offset parameter; macros gain in: $cursor, and at: $offset, forms; cast_length!/cast_int! lose their bare forms.
• ~80 call-site files updated, mostly with at: 0; a few expect_test snapshots in the testsuite updated to include the new offset: 0 field.

Reviewed changes

Copilot reviewed 100 out of 100 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
crates/ironrdp-core/src/decode.rs, encode.rs	Add offset to error kind variants and Display impls.
crates/ironrdp-core/src/error.rs	Extend the five error-construction traits and helpers with offset.
crates/ironrdp-core/src/macros.rs	New in:/at: macro arms; ensure_size!, cast_length!, cast_int! hardcode 0 internally.
crates/ironrdp-pdu/**	Mechanical migration of all *_err! call sites to at: 0 (and a few at: 0 to non-macro helper calls).
crates/ironrdp-rdpdr/**, ironrdp-rdpsnd/**, ironrdp-cliprdr*/**, ironrdp-egfx/**, ironrdp-dvc/**, ironrdp-displaycontrol/**, ironrdp-str/**, ironrdp-mstsgu/**, ironrdp-ainput/**, ironrdp-graphics/**, ironrdp-server/**, ironrdp-session/**	Same mechanical call-site migration; one destructure update in fast_path.rs adds ...
crates/ironrdp-testsuite-core/tests/**	Snapshot tests updated to include offset: 0, lines.

Comments suppressed due to low confidence (2)

crates/ironrdp-core/src/macros.rs:322

• The in: form of ensure_size! could detect that $buf is a cursor and call $buf.pos() to provide a real offset, since the failure inside ensure_size! is by far the most common producer of NotEnoughBytes errors in this codebase. Hardcoding offset 0 here is inconsistent with the stated rationale for removing the bare forms of cast_length!/cast_int! ("the bare forms silently passed 0, which was the bug being fixed"): the ensure_size! failure path now silently does the same thing for the in: cursor form, defeating most of the diagnostic value the PR sets out to add. Consider providing a separate path for ReadCursor/WriteCursor (e.g. via two macro arms or a small helper trait) that extracts .pos() automatically, and keeping the offset-0 fallback only for the raw-slice case.

This issue also appears on line 385 of the same file.

macro_rules! ensure_size {
    (ctx: $ctx:expr, in: $buf:ident, size: $expected:expr) => {{
        let received = $buf.len();
        let expected = $expected;
        if !(received >= expected) {
            // ensure_size!'s $buf binding is either a ReadCursor, a WriteCursor,
            // or a plain `&[u8]` / `&mut [u8]`. The first two expose `.pos()`,
            // the latter do not. We pass offset 0 from this macro; callers that
            // want a real offset use `not_enough_bytes_err!(in: cursor, ...)`
            // directly.
            return Err($crate::not_enough_bytes_err($ctx, received, expected, 0));
        }
    }};
    (in: $buf:ident, size: $expected:expr) => {{
        $crate::ensure_size!(ctx: $crate::function!(), in: $buf, size: $expected)
    }};
}

crates/ironrdp-core/src/macros.rs:425

• cast_length! and cast_int! now hardcode offset: 0 internally, while the PR description states that the bare forms of these macros were removed because "they silently passed 0, which was the bug being fixed." The new implementations still silently pass 0 from inside the macro body, so the documented motivation is not actually addressed: callers cannot supply a real offset even when they have a cursor in scope. Consider extending these macros with the same in: $cursor, / at: $offset, prefix forms used elsewhere in this PR (e.g. not_enough_bytes_err!, invalid_field_err!), so call sites can plumb through a meaningful position.

macro_rules! cast_length {
    ($ctx:expr, $field:expr, $len:expr) => {{
        $len.try_into()
            .map_err(|e| $crate::invalid_field_err_with_source($ctx, $field, "too many elements", 0, e))
    }};
    ($field:expr, $len:expr) => {{ $crate::cast_length!($crate::function!(), $field, $len) }};
}

/// Safely casts an integer to a different integer type.
///
/// This macro attempts to convert an integer value to a different integer type,
/// returning an error if the conversion fails due to out-of-range issues.
///
/// # Arguments
///
/// * `ctx` - The context for the error message (optional)
/// * `field` - The name of the field being cast
/// * `len` - The integer value to cast
///
/// # Examples
///
/// ```
/// use ironrdp_core::cast_int;
///
/// fn process_value(value: u64) -> Result<i32, Error> {
///     let casted_value: i32 = cast_int!("input value", value)?;
///     Ok(casted_value)
/// }
/// ```
///
/// # Note
///
/// If the context is not provided, it will use the current function name.
#[macro_export]
macro_rules! cast_int {
    ($ctx:expr, $field:expr, $len:expr) => {{
        $len.try_into().map_err(|e| {
            $crate::invalid_field_err_with_source($ctx, $field, "out of range integral type conversion", 0, e)
        })
    }};
    ($field:expr, $len:expr) => {{ $crate::cast_int!($crate::function!(), $field, $len) }};

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.