IronRDP icon indicating copy to clipboard operation
IronRDP copied to clipboard

Preserve unknown bitmask bits values in PDUs (eliminate use of 'from_bits_truncate')

Open pacmancoder opened this issue 1 year ago • 1 comments

To prevent losing information after PDU encode/decode roundtrip, we should always keep bitmask values unchanged, but currently, we use from_bits_truncate excessively in the codebase. As @CBenoit explained in the comment, we need to avoid usage of from_bits_truncate:

we should either use from_bits or from_bits_retain, but never from_bits_truncate, and I would tend to reach for from_bits_retain over from_bits

Rationale:

  • We want the round-tripping property to hold, and for this property to hold, parsing must be non destructive (lossless), but from_bits_truncate is destructive (undefined bits are discarded)
  • We generally want lenient parsing, ignoring unknown values as long as we don’t need them and/or as long as ignoring them is causing no harm, but from_bits is not lenient

However, it’s okay to use from_bits if strictness is required at some place, but I would like such places to be documented with a comment explaining why we have to refuse unknown flags

pacmancoder avatar Oct 12 '23 11:10 pacmancoder

Following up on this. Looks like a new syntax was added to the macro in bitflags v2.4: https://github.com/bitflags/bitflags/pull/371

bitflags! {
    pub struct Flags: u32 {
        const A = 0b00000001;
        const B = 0b00000010;
        const C = 0b00000100;

        // The source may set any bits
        const _ = !0; // <- this guy
    }
}

There is crate-level documentation for this: https://docs.rs/bitflags/2.4.0/bitflags/#externally-defined-flags

This is basically making from_bits_truncate behaves exactly like from_bits_retain because all bits are then considered to be known and defined. from_bits also never fails.

So I suggest this:

  • Use both from_bits_retain and const _ = !0 when lenient parsing is required.
    • const _ = !0 ensures we don’t accidentally have non lenient or destructive parsing.
    • from_bits_retain makes it clear at the call site that preserving all the bits is intentional.
  • Use from_bits WITHOUT const _ = !0 when strictness is required (almost never in IronRDP).

EDIT: see also https://github.com/bitflags/bitflags/issues/363 for background

CBenoit avatar Oct 12 '23 21:10 CBenoit