IronRDP
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_retain
andconst _ = !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
WITHOUTconst _ = !0
when strictness is required (almost never in IronRDP).
EDIT: see also https://github.com/bitflags/bitflags/issues/363 for background