IronRDP
                                
                                 IronRDP copied to clipboard
                                
                                    IronRDP copied to clipboard
                            
                            
                            
                        Preserve unknown bitmask bits values in PDUs (eliminate use of 'from_bits_truncate')
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
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_retainandconst _ = !0when lenient parsing is required.- const _ = !0ensures we don’t accidentally have non lenient or destructive parsing.
- from_bits_retainmakes it clear at the call site that preserving all the bits is intentional.
 
- Use from_bitsWITHOUTconst _ = !0when strictness is required (almost never in IronRDP).
EDIT: see also https://github.com/bitflags/bitflags/issues/363 for background