Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
pacmancoder opened this issue Oct 12, 2023 · 1 comment
Labels
A-technical-debt Area: Internal cleanup work bug Something isn't working good first issue Good for newcomers P-medium Medium priority

Comments

@pacmancoder
Copy link
Contributor

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

@CBenoit CBenoit added enhancement New feature or request A-technical-debt Area: Internal cleanup work bug Something isn't working good first issue Good for newcomers and removed enhancement New feature or request labels Oct 12, 2023
@CBenoit
Copy link
Member

CBenoit commented Oct 12, 2023

Following up on this. Looks like a new syntax was added to the macro in bitflags v2.4: bitflags/bitflags#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 bitflags/bitflags#363 for background

elmarco added a commit to elmarco/IronRDP that referenced this issue Oct 16, 2023
Be tolerant when parsing unknown flags.

Related to:
Devolutions#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
elmarco added a commit to elmarco/IronRDP that referenced this issue Oct 18, 2023
Be tolerant when parsing unknown flags.

Related to:
Devolutions#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
@CBenoit CBenoit added the P-medium Medium priority label Nov 1, 2023
elmarco added a commit to elmarco/IronRDP that referenced this issue Nov 13, 2023
Be tolerant when parsing unknown flags.

Related to:
Devolutions#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
elmarco added a commit to elmarco/IronRDP that referenced this issue Nov 15, 2023
Be tolerant when parsing unknown flags.

Related to:
Devolutions#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
elmarco added a commit to elmarco/IronRDP that referenced this issue Nov 29, 2023
Be tolerant when parsing unknown flags.

Related to:
Devolutions#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
elmarco added a commit to elmarco/IronRDP that referenced this issue Dec 5, 2023
Be tolerant when parsing unknown flags.

Related to:
Devolutions#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
elmarco added a commit to elmarco/IronRDP that referenced this issue Dec 5, 2023
Be tolerant when parsing unknown flags.

Related to:
Devolutions#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
CBenoit pushed a commit that referenced this issue Dec 5, 2023
Be tolerant when parsing unknown flags.

Related to:
#217

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-technical-debt Area: Internal cleanup work bug Something isn't working good first issue Good for newcomers P-medium Medium priority
Development

No branches or pull requests

2 participants