CLIPRDR(clipboard) processing - initialization sequence

[pacmancoder]

Adds initialization sequence of CLIPRDR static virtual channel

• Changes ironrdp-svc API to accommodate the need for adding SVC PDU flags on a per-message basis
• Reduces boilerplate code for AsAny

Part of #107

[ibeckermayer]

It's unclear to me what the advantage of maintaining this explicit state machine here is. Regardless of state, we are matching on pdu and responding based on its type. AFAICT, this logic is only checking that the server gives us PDUs in the expected order -- however I think we can safely assume that the server is working appropriately, and simply respond based on what it's giving us.

[pacmancoder]

The advantage of this is that when we keep track of the state via a state machine, it gives us the opportunity to make error reporting more clean (on which exact communication stage we received unexpected/faulty PDU), it also allows to more logically split code into methods/types when code will grow upon adding different formats handling (initialization code is almost independent of an actual data transition logic). Also, it leaves less place for human error (the states will eventually store operation-specific fields which will be only accessible when matched on the correct state).

I think for such a complex thing as a clipboard it makes sense, but I may be wrong. @CBenoit what is your opinion on that?

[CBenoit]

Though no state machine at all is perfectly viable and more readable for a small channel, I agree with you @pacmancoder
If you think the CLIPRDR channel implementation is going to grow into something way more complex that it currently is, then it makes sense to me. 👍

[ibeckermayer]

it gives us the opportunity to make error reporting more clean (on which exact communication stage we received unexpected/faulty PDU)

I think this point still begs the question of whether tracking unexpected/faulty PDUs is actually a valid concern for the client. IMO a faulty server implementation isn't worth designing for.

it also allows to more logically split code into methods/types when code will grow upon adding different formats handling (initialization code is almost independent of an actual data transition logic)

I'm not seeing how this is the case. From my perspective it seems that we can break down the internal methods/types used by process to whatever degree of resolution we want to in either approach.

Also, it leaves less place for human error (the states will eventually store operation-specific fields which will be only accessible when matched on the correct state).

I suppose that's true at least theoretically. IMO, presuming my other critiques hold water, then this concern alone isn't large enough to justify all of the additional code needed for explicit state management.

I do notice that there's a Sequence trait which we impl for various state machine types that do something similar, e.g.

IronRDP/crates/ironrdp-connector/src/connection.rs

Line 262 in b9de8ba

impl Sequence for ClientConnector {

I can see an argument for keeping consistency there, however in my view the "implicit state machine" version gets the job done just fine and is much easier to read / follow, e.g. 9c7e9da:

impl StaticVirtualChannel for Cliprdr {
    // ...

    fn process(&mut self, payload: &[u8]) -> PduResult<Vec<SvcMessage>> {
        let pdu = decode::<ClipboardPdu>(payload)?;

        match pdu {
            ClipboardPdu::Capabilites(caps) => self.handle_server_capabilities(caps),
            ClipboardPdu::MonitorReady => self.handle_monitor_ready(),
            ClipboardPdu::FormatListResponse(response) => self.handle_format_list_response(response),
            _ => todo!(),
        }
    }

    // ...
}

[CBenoit]

Fair arguments.

Also, it leaves less place for human error (the states will eventually store operation-specific fields which will be only accessible when matched on the correct state).

I suppose that's true at least theoretically. IMO, presuming my other critiques hold water, then this concern alone isn't large enough to justify all of the additional code needed for explicit state management.

Operation-specific fields only accessibles when matching on the correct state is still an interesting pattern, and maybe we can benefit from this without having an explicit state for absolutely each step in the sequence.

When it’s really needed, some sort of "state on demand" approach could give the benefits with less boilerplate:

#[derive(Default)]
enum CliprdrState {
    #[default]
    Idle,
    // Only a few special states are required
    SpecialState { some_value: u64 },
}

impl Cliprdr {
    fn handle_start_special_thing(&mut self, context: SpecialContext) -> PduResult<Vec<SvcMessage>> {
        // Do something

        // Transition to the special state, keeping some important context to use later
        self.state = CliprdrState::SpecialState {
            special_value: context.value,
        };

        Ok(vec![…])
    }

    fn handle_continue_special_thing(&mut self, other_context: SpecialOtherContext) -> PduResult<Vec<SvcMessage>> {
        // Consume the special state, returning an error if it’s unexpected (replacing the state
        // by `Idle`, the default, in all cases)
        let CliprdrState::SpecialState { special_value } = std::mem::take(&mut self.state) else {
            return Err(…);
        };

        // do something with `special_value` and `other_context`
    }
}

impl StaticVirtualChannel for Cliprdr {
    // ...

    fn process(&mut self, payload: &[u8]) -> PduResult<Vec<SvcMessage>> {
        let pdu = decode::<ClipboardPdu>(payload)?;

        match pdu {
            ClipboardPdu::Capabilites(caps) => self.handle_server_capabilities(caps),
            ClipboardPdu::MonitorReady => self.handle_monitor_ready(),
            ClipboardPdu::FormatListResponse(response) => self.handle_format_list_response(response),
            ClipboardPdu::StartSpecialThing(context) => self.handle_start_special_thing(context),
            ClipboardPdu::ContinueSpecialThing(other_context) => self.handle_continue_special_thing(other_context),
            _ => todo!(),
        }
    }

    // ...
}

It all depends on how much there is to keep track of imo. In the example above, an Option would do the same job.

[pacmancoder]

Ok, let's do this -- I'll remove the state machine, Currently for initialization we don't need it anyway. Later we will see if we need something to improve state management and refactor code appropriately later.

[ibeckermayer]

Good idea, I'm going to make a point to refactor rdpdr to do it like this.

[ibeckermayer]

Some info on where these come from could be helpful in case you want to support more formats in the future. Teleport's code for this is here

[ibeckermayer]

Consider making a ClipboardFormatId enum so that it's clear what this id means.

[pacmancoder]

I thought about that but rejected the idea because clipboard IDs could be actually arbitrary, OS-specific, enums are not ideal for such things, I think.

Maybe the right solution would be the new type struct struct ClipboardFromatId(pub u32)
and then define constants inside its impl:

impl ClipboardFromatId {
    const CF_TEXT: Self = Self(1);
    const CF_UNICODE_TEXT: Self = Self(13);
    // ...
}

This should make usage more clear:

let format_id = ClipboardFromatId::CF_TEXT;

// some custom id
let format_id = ClipboardFromatId(42);

I think it will be good compromise between raw u32 and too constrained enum

[CBenoit]

Maybe the right solution would be the new type struct struct ClipboardFromatId(pub u32) and then define constants inside its impl

Yes, this is generally a good approach in network protocols for parsing resilience. 👍

There is already some occurrence for this pattern:

• Code in ironrdp_graphics::rle module
• FailureCode in ironrdp_pdu::nego module
• SecurityType in IronVNC (repository is private)

That’s the way to go when knowledge of ALL the values is not required, not desired or not possible and we need to handle gracefully these situations.

[ibeckermayer]

In what cases is name empty? Maybe this type should be Optional<String> instead

[pacmancoder]

AFIK It is non-empty for os-specific formats, so it could be used for mapping remote and local clipboard IDs between each other.

I am not sure that we need Option here. because CLIPRDR specification tells us that when the name is not specified, it should be represented as the single null terminator, which translates to an empty string, Idk if it will make an API cleaner, @CBenoit what is your opinion on that?

[CBenoit]

In Rust, it’s generally more idiomatic to distinguish between absence of value, and the empty string, so here are the questions:

• Do we actually need to distinguish the empty string from the "no string" state?
• How ignoring this distinction could be problematic? What kind of issues the consumer of the API may run into?

In this case, what is the meaning of the empty string in the context of ClipboardFormat? I believe it means "non standard" or "OS-specific" format. So my understanding is that the empty value is actually something different than just an empty string. It’s likely that special handling is required in such situations.

Another way to see it: the consumer of the API may not even be aware that the empty string requires any special handling and may miss this detail, or may not know how to initialize the value properly. Could the user end up asking himself "I don’t know what I’m supposed to put inside this string"? It’s likely that Option will make it obvious that None is a valid avenue.

Not only Option would be a valid avenue, sum types in general are a good fit, and another way to make this point crystal clear would be to define another enum such as:

enum FormatKind {
    Standard,
    NonStandard { name: String },
}

I believe that all of this also applies to people reading, reviewing or auditing the code later (including the author himself after a few months 😛). The information conveyed by String, Option<String> or FormatKind is obviously not the same and plays a role in readability. It helps with local reasoning.

How the value itself is actually decoded or encoded can remain an implementation detail in the body of the encoding and decoding functions, i.e.: if a single null terminator means that "no name is specified" and that it also means the format is "standard", then we can say that Option::None or FormatKind::Standard can be represented by this single null terminator.

I didn’t review everything yet though, so it’s possible I’m misunderstanding something. I hope it helps you to decide on that!

[ibeckermayer]

Makes sense to use an empty string if it translates correctly to bytes.

[pacmancoder]

@CBenoit Well, in the case of ClipboardFormat PDU structure there is no option to specify absence of string, only an empty string (null terminator only) is possible, therefore I decided to reflect that in the Rust strucutre, But I agree that is it not idiomatic.

I just don't like the fact that if we go with Option we will have the following situation when comparing encoded and decode:

let original = FormatList::new_unicode(&[ClipboardFormat { id: 42, name: Some("".to_string()) }], true);
let encoded = encode(&original);

let decoded = decode::<FormatList>(&encoded);

let actual = FormatList::new_unicode(&[ClipboardFormat { id: 42, name: None }], true)
assert_eq!(decoded, actual);

assert_eq!(decoded, original); // Will fail, re-encoding produces different results

I think the best idea would be to hide implementation details of ClibpoardFormat (make fields private) and automatically store empty strings as None.

pub struct ClipboardFormat {
    id: u32,
    name: Option<String>
}

impl ClipboardFormat  {
    pub fn new(id: u32) -> Self { ... }
    /// NOTE: Method does nothing if empty string is specified
    pub fn with_name(self, name: &str) -> Self { ... }

    pub fn id(&self) -> u32 { ... }
    pub fn name(&self) -> Option<&str> {...}
}

[CBenoit]

Oh, I see. Sounds good to me!

[CBenoit]

Note: since this is an implementation detail, you could also use String internally, same as now, and simply check for emptiness when name() is called.
At your preference.

Would be cool to document what it means for name() to returns a Some(…) vs None as well.

[ibeckermayer]

Looks like this is currently unused, leftover or do you expect to need it in the future?

[pacmancoder]

It is already used for error messages when unexpected messages are received, it will be used more in the follow-up PRs

[ibeckermayer]

When I grep for message_name, only this definition comes up.

[pacmancoder]

oops, you are right, I am using name() from PduEncode instead of message_name() thanks for spotting this! 👍

[CBenoit]

Looking pretty good to me. Small rustdoc nitpicks below.

[CBenoit]

This is looking good to me. I’ll wait for ARC-143 branch to be merged on master before approving.

[CBenoit]

Do you think you can rebase on top of master? @pacmancoder

[pacmancoder]

@CBenoit Sure, I am already doing it 😄 , I'll update PR soon

[github-actions]

Coverage Report 🤖 ⚙️

Past:
Total lines: 22147
Covered lines: 15573 (70.32%)

New:
Total lines: 22374
Covered lines: 15625 (69.84%)

Diff: -0.48%

[this comment will be updated automatically]

[pacmancoder]

@CBenoit Done!

[CBenoit]

Thank you! LGTM