You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by "necouchman (via GitHub)" <gi...@apache.org> on 2026/05/16 13:24:40 UTC
Re: [PR] GUACAMOLE-2263: Harden CLIPRDR channel against FreeRDP 3.x clipboard state machine. [guacamole-server]
necouchman commented on code in PR #661:
URL: https://github.com/apache/guacamole-server/pull/661#discussion_r3252840702
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -357,21 +384,20 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
static UINT guac_rdp_cliprdr_format_data_request(CliprdrClientContext* cliprdr,
CLIPRDR_CONST CLIPRDR_FORMAT_DATA_REQUEST* format_data_request) {
- /* FreeRDP-specific handlers for CLIPRDR are not assigned, and thus not
- * callable, until after the relevant guac_rdp_clipboard structure is
- * allocated and associated with the CliprdrClientContext */
guac_rdp_clipboard* clipboard = (guac_rdp_clipboard*) cliprdr->custom;
assert(clipboard != NULL);
guac_client* client = clipboard->client;
guac_rdp_client* rdp_client = (guac_rdp_client*) client->data;
guac_rdp_settings* settings = rdp_client->settings;
- guac_client_log(client, GUAC_LOG_TRACE, "CLIPRDR: Received format data request.");
+ guac_client_log(client, GUAC_LOG_TRACE, "CLIPRDR: Received format data "
+ "request for format 0x%X.", format_data_request->requestedFormatId);
guac_iconv_write* remote_writer;
+ int output_buf_size = clipboard->clipboard->available;
const char* input = clipboard->clipboard->buffer;
- char* output = guac_mem_alloc(GUAC_COMMON_CLIPBOARD_MAX_LENGTH);
+ char* output = guac_mem_alloc(output_buf_size);
Review Comment:
I'm not sure if this overlaps with #659, or is related to the discussion on that thread regarding allocating `available * 2` or `available * 4`? If this is an overlap, and is already covered by the changes in that change request, then it should probably not be, here.
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -313,23 +337,26 @@ static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
.msgFlags = CB_RESPONSE_OK
#endif
};
- /* Report successful processing of format list */
Review Comment:
Is this comment not accurate?
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -659,11 +723,10 @@ void guac_rdp_clipboard_load_plugin(guac_rdp_clipboard* clipboard,
void guac_rdp_clipboard_free(guac_rdp_clipboard* clipboard) {
- /* Do nothing if the clipboard is not actually allocated */
if (clipboard == NULL)
return;
- /* Free clipboard and underlying storage */
Review Comment:
Why has this comment been removed?
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -292,9 +308,6 @@ static int guac_rdp_cliprdr_format_supported(const CLIPRDR_FORMAT_LIST* format_l
static UINT guac_rdp_cliprdr_format_list(CliprdrClientContext* cliprdr,
CLIPRDR_CONST CLIPRDR_FORMAT_LIST* format_list) {
- /* FreeRDP-specific handlers for CLIPRDR are not assigned, and thus not
- * callable, until after the relevant guac_rdp_clipboard structure is
- * allocated and associated with the CliprdrClientContext */
Review Comment:
Is this comment incorrect?
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -450,17 +494,35 @@ static UINT guac_rdp_cliprdr_format_data_request(CliprdrClientContext* cliprdr,
static UINT guac_rdp_cliprdr_format_data_response(CliprdrClientContext* cliprdr,
CLIPRDR_CONST CLIPRDR_FORMAT_DATA_RESPONSE* format_data_response) {
- /* FreeRDP-specific handlers for CLIPRDR are not assigned, and thus not
- * callable, until after the relevant guac_rdp_clipboard structure is
- * allocated and associated with the CliprdrClientContext */
Review Comment:
Why is this comment being removed?
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -172,22 +172,23 @@ static UINT guac_rdp_cliprdr_send_capabilities(CliprdrClientContext* cliprdr) {
static UINT guac_rdp_cliprdr_monitor_ready(CliprdrClientContext* cliprdr,
CLIPRDR_CONST CLIPRDR_MONITOR_READY* monitor_ready) {
- /* FreeRDP-specific handlers for CLIPRDR are not assigned, and thus not
- * callable, until after the relevant guac_rdp_clipboard structure is
- * allocated and associated with the CliprdrClientContext */
guac_rdp_clipboard* clipboard = (guac_rdp_clipboard*) cliprdr->custom;
assert(clipboard != NULL);
guac_client_log(clipboard->client, GUAC_LOG_TRACE, "CLIPRDR: Received "
"monitor ready.");
- /* Respond with capabilities ... */
int status = guac_rdp_cliprdr_send_capabilities(cliprdr);
if (status != CHANNEL_RC_OK)
- return status;
+ guac_client_log(clipboard->client, GUAC_LOG_WARNING, "CLIPRDR: "
+ "Failed to send capabilities (error %d).", status);
Review Comment:
I understand the goal of this - to continue the connection even if clipboard redirection fails to sync up capabilities with the server. That said, shouldn't we at least still `return CHANNEL_RC_OK`, here, and not try to send a format list if we've failed to send capabilities?
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -418,14 +461,15 @@ static UINT guac_rdp_cliprdr_format_data_request(CliprdrClientContext* cliprdr,
#endif
};
- guac_client_log(client, GUAC_LOG_TRACE, "CLIPRDR: Sending format data response.");
+ guac_client_log(client, GUAC_LOG_TRACE, "CLIPRDR: Sending format data "
+ "response (%d bytes).", (int)(((BYTE*) output) - start));
pthread_mutex_lock(&(rdp_client->message_lock));
- UINT result = cliprdr->ClientFormatDataResponse(cliprdr, &data_response);
+ cliprdr->ClientFormatDataResponse(cliprdr, &data_response);
pthread_mutex_unlock(&(rdp_client->message_lock));
guac_mem_free(start);
- return result;
+ return CHANNEL_RC_OK;
Review Comment:
The documentation for this function should be updated to indicate either that it always returns `CHANNEL_RC_OK` despite failures, or the situations where failures are returned vs. `CHANNEL_RC_OK`.
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -659,11 +723,10 @@ void guac_rdp_clipboard_load_plugin(guac_rdp_clipboard* clipboard,
void guac_rdp_clipboard_free(guac_rdp_clipboard* clipboard) {
- /* Do nothing if the clipboard is not actually allocated */
Review Comment:
Why has this comment been removed?
##########
src/protocols/rdp/channels/cliprdr.c:
##########
@@ -384,25 +410,42 @@ static UINT guac_rdp_cliprdr_format_data_request(CliprdrClientContext* cliprdr,
remote_writer = settings->clipboard_crlf ? GUAC_WRITE_UTF16_CRLF : GUAC_WRITE_UTF16;
break;
- /* Warn if clipboard data cannot be sent as intended due to a violation
- * of the CLIPRDR spec */
+ /* Send a FAIL response for unsupported formats — FreeRDP 3.x will
+ * time out and disconnect if the server never responds */
default:
guac_client_log(client, GUAC_LOG_WARNING, "Received clipboard "
"data cannot be sent to the RDP server because the RDP "
"server has requested a clipboard format which was not "
"declared as available. This violates the specification "
"for the CLIPRDR channel.");
guac_mem_free(output);
+
+ CLIPRDR_FORMAT_DATA_RESPONSE fail_response = {
+ .requestedFormatData = NULL,
+#ifdef HAVE_CLIPRDR_HEADER
+ .common = {
+ .msgType = CB_FORMAT_DATA_RESPONSE,
+ .msgFlags = CB_RESPONSE_FAIL,
+ .dataLen = 0,
+ }
+#else
+ .dataLen = 0,
+ .msgFlags = CB_RESPONSE_FAIL
+#endif
+ };
+
+ pthread_mutex_lock(&(rdp_client->message_lock));
+ cliprdr->ClientFormatDataResponse(cliprdr, &fail_response);
+ pthread_mutex_unlock(&(rdp_client->message_lock));
return CHANNEL_RC_OK;
}
- /* Send received clipboard data to the RDP server in the format
- * requested */
Review Comment:
Why is this comment being removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org