Improve text clipboard interoperability with strict clients#3805
Improve text clipboard interoperability with strict clients#3805YTjungle666 wants to merge 2 commits into
Conversation
7f3f75c to
de91730
Compare
de91730 to
2a7793e
Compare
matt335672
left a comment
There was a problem hiding this comment.
Some minor comments below.
It should also be noted that we also have a number of XFreeRDP clients, and these may require other text formats being advertised.
If I RDP in to an XFCE session using XFreeRDP and copy some text in the session, I get this result in the session:
$ xclip -o -selection clipboard -t TARGETS
TIMESTAMP
TARGETS
MULTIPLE
SAVE_TARGETS
UTF8_STRING
COMPOUND_TEXT
TEXT
STRING
text/plain;charset=utf-8
text/plain
but on the host I get:
xclip -o -selection clipboard -t TARGETS
TIMESTAMP
TARGETS
MULTIPLE
text/plain;charset=utf-8
UTF8_STRING
I don't think this is a regression as such; using the latest devel I also get text/plain and TEXT targets advertised, but they don't actually work.
| } | ||
| else if (atom == XA_STRING) | ||
| text_priority = clipboard_text_target_priority(atom); | ||
| if (text_priority > 0) |
There was a problem hiding this comment.
This test combined with the next seems to be redundant, as got_text_priority is always >= 0
| return 1; | ||
| } | ||
|
|
||
| len = g_strlen(text_list[0]); |
There was a problem hiding this comment.
I've been reviewing a few security advisories recently, so need to check that len is not equal to INT_MAX before adding 1 to it.
| return 1; | ||
| } | ||
|
|
||
| text = (char *)g_malloc(utf8_size + 1, 0); |
There was a problem hiding this comment.
Check utf8_size is not equal to INT_MAX before adding 1`.
| out_uint16_le(s, 12); /* lengthCapability */ | ||
| out_uint32_le(s, g_cliprdr_version); /* version */ | ||
| out_uint32_le(s, g_cliprdr_flags); /* generalFlags */ | ||
| out_uint32_le(s, 0); /* extra 4 bytes ? */ |
There was a problem hiding this comment.
Thanks for removing these - they've been here a long time.
| /* Advertise the lossless text format only. Windows can | ||
| * synthesize CF_TEXT and CF_OEMTEXT from CF_UNICODETEXT, and | ||
| * some strict clients reject a redundant text format list. */ | ||
| out_uint32_le(s, 0x0000000d); |
There was a problem hiding this comment.
FWIW, CF_UNICODETEXT is defined in common/xrdp_constants.h, and can replace the magic number here.
| /* Advertise the lossless text format only. Windows can | ||
| * synthesize CF_TEXT and CF_OEMTEXT from CF_UNICODETEXT, and | ||
| * some strict clients reject a redundant text format list. */ | ||
| out_uint32_le(s, 0x0000000d); |
Address review feedback for text target handling by converting legacy X text targets through Xlib text properties and adding allocation overflow guards. Also replace the CF_UNICODETEXT magic number with the existing constant and simplify redundant target priority logic.
|
Thanks for the review and for the detailed XFreeRDP TARGETS comparison. I've updated the patch:
For the XFreeRDP case, I don't currently have an XFreeRDP clipboard test environment available, so I couldn't verify that path directly. However, I used your TARGETS output as guidance and updated the X-side text target handling so these targets are treated more consistently instead of advertising text targets which can't actually be served. In particular, the updated patch now handles The advertised/requested text target order has also been adjusted to cover the legacy/interoperability targets you pointed out: Hopefully this improves compatibility with XFreeRDP-style clients without regressing the strict Windows client behaviour this PR was originally targeting. |
|
Thanks @YTjungle666 I don't think I managed to explain the XFreeRDP client interoperability very well. Previously we were advertising all text formats to 'Windows' clients, and now we're relying on the client to do the conversion from Before this change, that meant an XFreeRDP client would see I'm wondering whether we could simplify a lot of this by simply dropping support for older formats. Converting from UTF-8 to I get this on the xrdp side:- I can't see any usefulness in the I can see two ways in which xrdp should behave in this situation:
Option 2) is a lot closer to where we are at the moment. @metalefty - do you have any thoughts on this? Are we gaining anything by supporting older text formats other than a potential maintenance burden in the future? |
Summary
This updates chansrv clipboard text handling to improve interoperability with stricter RDP clients, including current Microsoft Remote Desktop / Windows App clients.
The change:
text/plain;charset=utf-8,text/plain,COMPOUND_TEXT, andTEXT) in addition toUTF8_STRINGandSTRINGCOMPOUND_TEXTto/from UTF-8 so older X11 clients can still exchange text through the RDP clipboarddataLenfrom clipboard PDUsCF_UNICODETEXTformat for server-to-client text clipboard updates, allowing Windows to synthesizeCF_TEXT/CF_OEMTEXTlocally instead of rejecting a redundant text format listCF_LOCALE,CF_TEXT,CF_OEMTEXT, andCF_UNICODETEXTwhen those formats are requestedRationale
The CLIPRDR payload length should match the
dataLenfield. Some clients tolerate extra trailing bytes, but stricter clients can reject the format list. In local testing, the Windows client returnedCB_FORMAT_LIST_RESPONSEwithCB_RESPONSE_FAILwhen the server advertised a redundant text format list. AdvertisingCF_UNICODETEXTonly for server-to-client text avoids the rejection while keeping the Unicode path lossless. Windows can synthesize the legacy text formats fromCF_UNICODETEXTwhen applications request them.The X11 selection conversion request is only issued after an XFixes owner notification for a new server-side copy operation. This avoids synchronizing clipboard content that existed before the RDP client connected, while still ensuring server-side copies made after connection are announced without waiting for another X11 event.
Testing
scripts/run_astyle.sh -v 3.4.14./bootstrap./configure --prefix=/usr --sysconfdir=/etc --localstatedir=/var --runstatedir=/run --libdir=/usr/lib/x86_64-linux-gnu --libexecdir=/usr/lib/x86_64-linux-gnu --with-pkgconfigdir=/usr/lib/x86_64-linux-gnu/pkgconfig --with-socketdir=/run/xrdp/sockdir --with-systemdsystemunitdir=/usr/lib/systemd/system --enable-ipv6 --enable-jpeg --enable-fuse --enable-rfxcodec --enable-painter --enable-pammake -j$(nproc)make check