fix(server): drop unexpected PDUs during deactivation-reactivation #630
Conversation
| unmatched: Option<&mut Vec<Bytes>>, | ||
| ignore_unexpected_pdu: bool, | ||
| ) -> ConnectorResult<()> | ||
| where | ||
| S: FramedWrite + FramedRead, | ||
| { | ||
| buf.clear(); | ||
| let written = single_sequence_step_read(framed, sequence, buf, unmatched).await?; | ||
| let written = match single_sequence_step_read(framed, sequence, buf).await { | ||
| Ok(written) => written, | ||
| Err(err) => { | ||
| return if ignore_unexpected_pdu && matches!(err.kind(), ConnectorErrorKind::Decode(_)) { | ||
| Ok(()) | ||
| } else { | ||
| Err(err) | ||
| }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
question: Correct me if I’m wrong. read_by_hint which is called underneath is already ignoring the PDU when it does not match the hint. (matched is false.)
This error is arising when we get a PDU kind we expected, and we were no able to decode it. As such, the error should not be ignored, I would think.
I understand my question boils down to what you said here:
The current behaviour of handling unmatched PDUs in fn read_by_hint() isn't good enough. An unexpected PDUs may be received and fail to be decoded during Acceptor::step().
Do I understand correctly that these unexpected PDUs are matching the expected hint, but are still considered "unexpected" in some other way?
There was a problem hiding this comment.
Yes, the received PDU matches the expected hint but fails to be decoded. This will break the connection during deact-react. I have no better idea how we could flush/sync/fence the client PDUs before.
There was a problem hiding this comment.
Thank you!
Change the code to simply drop unexpected PDUs (as opposed to attempting to replay the unmatched leftover, which isn't clearly needed)
Sounds reasonable!
This require Acceptor::step() to be idempotent on error, so we have to clone the state fields, instead of taking the state.
To be honest, I’m not very inclined into making this change, as I believe there is a better approach.
Before going further, I want to check my understanding at this place: https://github.com/Devolutions/IronRDP/pull/630/files#r1905600306
The current behaviour of handling unmatched PDUs in fn read_by_hint() isn't good enough. An unexpected PDUs may be received and fail to be decoded during Acceptor::step(). Change the code to simply drop unexpected PDUs (as opposed to attempting to replay the unmatched leftover, which isn't clearly needed) and make single_sequence_step_read() resilient to decode issues when ignore_unexpected_pdu. This require Acceptor::step() to be idempotent on error, so we have to clone the state fields, instead of taking the state. Signed-off-by: Marc-André Lureau <[email protected]>
|
I added a few other deact-react fixes to this PR |
I’m not sure what deact-react means in this context (EDIT: deactivation-reactivation, got you 😂), but all of these new commits are looking good to be merged. 😄 |
Signed-off-by: Marc-André Lureau <[email protected]>
For completeness, this error is used by FreeRDP. Signed-off-by: Marc-André Lureau <[email protected]>
I couldn't find any explicit behaviour described in the specification, but apparently, we must just keep the channel state as they were during reactivation. This fixes various state issues during client resize. Signed-off-by: Marc-André Lureau <[email protected]>
The current behaviour of handling unmatched PDUs in fn read_by_hint() isn't good enough. An unexpected PDUs may be received and fail to be decoded during Acceptor::step().
Change the code to simply drop unexpected PDUs (as opposed to attempting to replay the unmatched leftover, which isn't clearly needed) and make single_sequence_step_read() resilient to decode issues when ignore_unexpected_pdu. This require Acceptor::step() to be idempotent on error, so we have to clone the state fields, instead of taking the state.