GUACAMOLE-2323: Abort the connection when FreeRDP reports a connection error - #709
GUACAMOLE-2323: Abort the connection when FreeRDP reports a connection error#709xelan wants to merge 2 commits into
Conversation
…n error. Leaving the message loop without aborting the guac_client returns from guac_rdp_handle_connection() with the client still running, which guac_rdp_client_thread() takes as a request to reconnect. RDP servers that end a session by tearing down the connection instead of sending a Set Error Info PDU (Windows XP on logoff) were therefore given a fresh, silently auto-logged-in session rather than a closed one. This path aborted the client before 1.6.0, where the early break was introduced with the render thread. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
necouchman
left a comment
necouchman
left a comment
There was a problem hiding this comment.
See my comment below - I think we've discussed this in the past, and I'm pretty sure the behavior we have currently is the desired behavior - generally speaking, failures in the RDP client should trigger a reconnect, and changing that behavior for the sake of broken versions of Windows that don't send the correct disconnect/logout event to the client I'm not sure is the way to go - at least, not without some other checks to distinguish between the two. Otherwise we might end up simply aborting connections in situations where we should actually retry them?
| /* Abort if the wait failed or FreeRDP recorded a connection error, as | ||
| * is the case when an RDP server tears down the connection without | ||
| * first sending a Set Error Info PDU. Simply leaving the loop would | ||
| * return from this function with the guac_client still running, which | ||
| * guac_rdp_client_thread() takes as a request to reconnect. */ | ||
| if (wait_result < 0) { | ||
| guac_rdp_client_abort(client, rdp_inst); | ||
| break; | ||
| } |
There was a problem hiding this comment.
How is guacd to distinguish events which should trigger a reconnection from the logout events which should not?
There was a problem hiding this comment.
Thanks for your feedback @necouchman! I've added another commit which improves the detection logic. Now the guac_rdp_client_abort is only done if FreeRDP reports an appropriate info code. Tested with Windows XP SP3. What do you think about this approach?
…ion ended. Restricts the previous commit's abort to connections for which FreeRDP holds an error info code, which it records both from a Set Error Info PDU and from a Disconnect Provider Ultimatum. Windows XP ends a session with the former, and FreeRDP notes the code without aborting the connection, so the error surfaces at the top of the message loop rather than through freerdp_shall_disconnect(); a logoff was therefore answered with a fresh, silently auto-logged-in session. Errors carrying no error info code are plain connection failures and stay eligible for the reconnect that guac_rdp_client_thread() performs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Fixes https://issues.apache.org/jira/browse/GUACAMOLE-2323