From 11c55f8dcd64d74ea2348aae2b70b311cd709c5c Mon Sep 17 00:00:00 2001 From: ilammy Date: Mon, 15 Aug 2016 22:45:20 +0300 Subject: [PATCH] client/X11: cache original clipboard data for raw transfers FreeRDP uses clipboard->data to cache the result of the Windows->X11 clipboard format conversion, and xf_cliprdr_process_selection_request() immediately provides this result to local applications if they request the same clipboard format again. This saves us a possibly costly conversion in case where the user pastes data repeatedly. However, this caching mechanism did not support raw clipboard transfers where the unmodified data is passed between two FreeRDP clients. We use the same XClipboard protocol for this, so the clipboard->data is in play. We clear the cached value when we receive new data from the server, so initially raw transfers are fine. But if some local application (e.g., a clipboard manager) asks for some data format before the data is pasted into the second FreeRDP session then clipboard->data will contain the *converted* data. And this converted cached data will be provided to the second FreeRDP session as a part of the raw data transfer. Instead we should have provided the original data. In order to achieve this we are now caching the original data in the same way as the converted one, and the original data is now correctly provided when the second FreeRDP session asks for a raw data transfer. --- client/X11/xf_cliprdr.c | 72 ++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 18 deletions(-) diff --git a/client/X11/xf_cliprdr.c b/client/X11/xf_cliprdr.c index e2af8cc25..47068e95b 100644 --- a/client/X11/xf_cliprdr.c +++ b/client/X11/xf_cliprdr.c @@ -81,10 +81,12 @@ struct xf_clipboard int requestedFormatId; BYTE* data; + BYTE* data_raw; BOOL data_raw_format; UINT32 data_format_id; const char* data_format_name; int data_length; + int data_raw_length; XEvent* respond; Window owner; @@ -774,6 +776,23 @@ static BOOL xf_cliprdr_process_selection_notify(xfClipboard* clipboard, } } +static void xf_cliprdr_clear_cached_data(xfClipboard* clipboard) +{ + if (clipboard->data) + { + free(clipboard->data); + clipboard->data = NULL; + } + clipboard->data_length = 0; + + if (clipboard->data_raw) + { + free(clipboard->data_raw); + clipboard->data_raw = NULL; + } + clipboard->data_raw_length = 0; +} + static BOOL xf_cliprdr_process_selection_request(xfClipboard* clipboard, XEvent* xevent) { @@ -785,6 +804,7 @@ static BOOL xf_cliprdr_process_selection_request(xfClipboard* clipboard, BYTE* data = NULL; BOOL delayRespond; BOOL rawTransfer; + BOOL matchingFormat; unsigned long length; unsigned long bytes_left; CLIPRDR_FORMAT* format; @@ -847,14 +867,24 @@ static BOOL xf_cliprdr_process_selection_request(xfClipboard* clipboard, } } - if ((clipboard->data != 0) && (formatId == clipboard->data_format_id) - && (formatName == clipboard->data_format_name)) + /* We can compare format names by pointer value here as they are both + * taken from the same clipboard->serverFormats array */ + matchingFormat = (formatId == clipboard->data_format_id) + && (formatName == clipboard->data_format_name); + + if (matchingFormat && (clipboard->data != 0) && !rawTransfer) { - /* Cached clipboard data available. Send it now */ + /* Cached converted clipboard data available. Send it now */ respond->xselection.property = xevent->xselectionrequest.property; xf_cliprdr_provide_data(clipboard, respond, clipboard->data, clipboard->data_length); } + else if (matchingFormat && (clipboard->data_raw != 0) && rawTransfer) + { + /* Cached raw clipboard data available. Send it now */ + respond->xselection.property = xevent->xselectionrequest.property; + xf_cliprdr_provide_data(clipboard, respond, clipboard->data_raw, clipboard->data_raw_length); + } else if (clipboard->respond) { /* duplicate request */ @@ -865,11 +895,7 @@ static BOOL xf_cliprdr_process_selection_request(xfClipboard* clipboard, * Send clipboard data request to the server. * Response will be postponed after receiving the data */ - if (clipboard->data) - { - free(clipboard->data); - clipboard->data = NULL; - } + xf_cliprdr_clear_cached_data(clipboard); respond->xselection.property = xevent->xselectionrequest.property; clipboard->respond = respond; @@ -1128,11 +1154,7 @@ static UINT xf_cliprdr_server_format_list(CliprdrClientContext* context, xfContext* xfc = clipboard->xfc; UINT ret; - if (clipboard->data) - { - free(clipboard->data); - clipboard->data = NULL; - } + xf_cliprdr_clear_cached_data(clipboard); clipboard->data_format_id = -1; clipboard->data_format_name = NULL; @@ -1276,11 +1298,7 @@ static UINT xf_cliprdr_server_format_data_response(CliprdrClientContext* if (!clipboard->respond) return CHANNEL_RC_OK; - if (clipboard->data) - { - free(clipboard->data); - clipboard->data = NULL; - } + xf_cliprdr_clear_cached_data(clipboard); pDstData = NULL; DstSize = 0; @@ -1345,8 +1363,25 @@ static UINT xf_cliprdr_server_format_data_response(CliprdrClientContext* } } + /* Cache converted and original data to avoid doing a possibly costly + * conversion again on subsequent requests */ clipboard->data = pDstData; clipboard->data_length = DstSize; + + /* We have to copy the original data again, as pSrcData is now owned + * by clipboard->system. Memory allocation failure is not fatal here + * as this is only a cached value. */ + clipboard->data_raw = (BYTE*) malloc(size); + if (clipboard->data_raw) + { + CopyMemory(clipboard->data_raw, data, size); + clipboard->data_raw_length = size; + } + else + { + WLog_WARN(TAG, "failed to allocate %"PRIu32" bytes for a copy of raw clipboard data", size); + } + xf_cliprdr_provide_data(clipboard, clipboard->respond, pDstData, DstSize); XSendEvent(xfc->display, clipboard->respond->xselection.requestor, 0, 0, clipboard->respond); @@ -1491,6 +1526,7 @@ void xf_clipboard_free(xfClipboard* clipboard) ClipboardDestroy(clipboard->system); free(clipboard->data); + free(clipboard->data_raw); free(clipboard->respond); free(clipboard->incr_data); free(clipboard);