Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash Tigervnc server/viewer Ultravnc viewer/server #8

Closed
RudiDeVos opened this issue Jan 3, 2021 · 19 comments
Closed

Crash Tigervnc server/viewer Ultravnc viewer/server #8

RudiDeVos opened this issue Jan 3, 2021 · 19 comments

Comments

@RudiDeVos
Copy link
Member

RudiDeVos commented Jan 3, 2021

There is a minor implementation difference between for ExtDesktop.
TigerVNC expect that the server send valid resolution in the "inform ExtDesktopSize message".

UltraVNC code changed to make it compatible.
Testbuild
https://www.uvnc.eu/download/132/UltraVnc_132Tiger.zip

@RudiDeVos
Copy link
Member Author

RudiDeVos commented Jan 3, 2021

rfbEncodingExtendedClipboard is different implemented in TigerVNC.
As test the rfbEncodingExtendedClipboard protocol was disabled in the UltraVNC viewer, then the clipboard works.

The rfbEncodingExtendedClipboard extension was implemented in UltraVNC in 2010.

Looks like the clipboard semi works, the viewer can manual request the clipboard data.
Server clipboard change
TigerVNC server send a clipNotify with notifyRemoteFormats =0
TigerVNC server should send a clipProvide with the clipboard in the formats that were set using the caps message
The UltraVNC viewer
-clipboard option -> send remote data now (manual request the clipboard data)
TigerVNC server send the correct clipboard data.

@CendioOssman
Copy link

TigerVNC server should send a clipProvide with the clipboard in the formats that were set using the caps message

It should indeed. Let me have a look.

I'm not seeing any notifications from the viewer for clipboard from client to server either. Did you happen to check that as well?

@CendioOssman
Copy link

Server to client should be fixed in TigerVNC/tigervnc#1169.

However I'll need your help with the other direction. I don't seem to get anything from the UltraVNC client, even if I manually select a clipboard sync.

@RudiDeVos
Copy link
Member Author

On clipboard change UltraVNC send a clipProvide, not a clipNotify.
I see a rfbClientCutText + clipProvide going out.

TigerVNC correct read the clipboard data, it's only not placed in the clipboard. If not, all would be out of sync.
Possible TigerVNC always expect a clipNotify

Flow on UltraVNC server
Read sz_rfbClientCutTextMsg
Read extendedClipboardDataMessage with length from sz_rfbClientCutTextMsg
We extract the action DWORD action = extendedClipboardDataMessage.GetFlags() & clipActionMask;

Relvant code

switch(action) {
						case clipCaps:
							m_client->m_clipboard.settings.HandleCapsPacket(extendedClipboardDataMessage, true);
							break;
						case clipProvide:
							m_server->UpdateLocalClipTextEx(extendedClipboardDataMessage, m_client);
							break;
						case clipRequest:
						case clipPeek:
							{
								ClipboardData clipboardData;
								
								// only need an owner window when setting clipboard data -- by using NULL we can rely on fewer locks
								if (clipboardData.Load(NULL)) {
									m_client->UpdateClipTextEx(clipboardData, extendedClipboardDataMessage.GetFlags());
								}
							}
							break;
						case clipNotify:	// irrelevant coming from viewer
						default:
							// unsupported or not implemented
							break;
					}

@CendioOssman
Copy link

Possible TigerVNC always expect a clipNotify

It does. We set the length in the caps message to 0 to indicate this. We want to avoid transferring clipboard data if not needed, and it allows us to better keep track of multiple clients in the server.

Is it possible to get the UltraVNC viewer to respect TigerVNC's caps setting and send notify instead?

@RudiDeVos
Copy link
Member Author

RudiDeVos commented Jan 5, 2021

Viewer send the caps he wants, Server send back the caps he wants..
The common caps are direct send as clipProvide message + data.
Current we don't use the ClipNotify, server ignore the message

Optional a ClipNotify message can be used to indicate the server that also other caps exist.
If used it's send after the clipProvide.
clipNotify = 0x08000000, // notify that the formats combined with the flags are available for transfer. // Message should be empty // When a clipProvide message is received, then all formats notified as being available are // invalidated. Therefore, when implementing, ensure that clipProvide messages are sent before // clipNotify messages, specifically when in response to a change in the clipboard

Changed viewer to send a clipNotify after the clipProvide.
Fast tested on the linux server, only difference is that the clipboard is erased, but still no data
https://www.uvnc.eu/download/132/UltraVnc_132Tiger_2.zip

Made an other change for the viewer
clipProvide is only send for caps >0
clipNotify is send for caps = 0 with data in the buffer
https://www.uvnc.eu/download/132/UltraVnc_132Tiger_3.zip
This is how it should be implmented i guess , but TigerVNC now disconnect after a clipboard viewer->server

if (m_clipboard.m_bNeedToNotify) { is code was added,

All caps were 0 (text/rtf..), no m_bNeedToProvide = false and m_bNeedToNotify = true with text and rtf
After the WriteExact((char*)(m_clipboard.extendedClipboardDataNotifyMessage.. TigerVNC disconnect

		if (m_clipboard.m_bNeedToProvide) {
				m_clipboard.m_bNeedToProvide = false;
				int actualLen = m_clipboard.extendedClipboardDataMessage.GetDataLength();

				rfbClientCutTextMsg message;
				memset(&message, 0, sizeof(rfbClientCutTextMsg));
				message.type = rfbClientCutText;

				message.length = Swap32IfLE(-actualLen);
				
				//adzm 2010-09
				WriteExactQueue((char*)&message, sz_rfbClientCutTextMsg, rfbClientCutText);
				WriteExact((char*)(m_clipboard.extendedClipboardDataMessage.GetData()), m_clipboard.extendedClipboardDataMessage.GetDataLength());
			
				vnclog.Print(6, _T("Sent extended clipboard\n"));
			}

                               if (m_clipboard.m_bNeedToNotify) {
				m_clipboard.m_bNeedToNotify = false;
				if (m_clipboard.settings.m_bSupportsEx) {

					int actualLen =m_clipboard.extendedClipboardDataNotifyMessage.GetDataLength();

					rfbServerCutTextMsg message;
					memset(&message, 0, sizeof(rfbServerCutTextMsg));
					message.type = rfbServerCutText;

					message.length = Swap32IfLE(-actualLen);

					WriteExactQueue((char*)&message, sz_rfbClientCutTextMsg, rfbClientCutText);
					WriteExact((char*)(m_clipboard.extendedClipboardDataNotifyMessage.GetData()), m_clipboard.extendedClipboardDataNotifyMessage.GetDataLength());

				}
				m_clipboard.extendedClipboardDataNotifyMessage.Reset();
			}

@CendioOssman
Copy link

The bug is here:

					message.type = rfbServerCutText;

It should be rfbClientCutText. Right now the server will read this as FrameBufferUpdateRequest (since that has the same value as rfbServerCutText), but the message size doesn't match and everything goes nuts after that.

@MarcusWichelmann
Copy link

There is a minor implementation difference between for ExtDesktop.
TigerVNC expect that the server send valid resolution in the "inform ExtDesktopSize message".

UltraVNC code changed to make it compatible.
Testbuild
https://www.uvnc.eu/download/132/UltraVnc_132Tiger.zip

A vnc client implementation by me was affected by this, too. I just tested your testbuild and can confirm that the bug is fixed there. Is it's source code already pushed anywhere? Will this fix land in master?

@RudiDeVos
Copy link
Member Author

extDesktop is Pushed in development
https://github.com/ultravnc/UltraVNC/tree/development

@MarcusWichelmann
Copy link

@RudiDeVos Thank you!
I just noticed, that, with your test build, the server does not send ExtendedDesktopSize pseudo encodings when the server size changes, although the server knows that the client supports this extension. Instead it sends a DesktopSize now, which looks wrong.
Maybe you'd like to take one more look?

@RudiDeVos
Copy link
Member Author

RudiDeVos commented Jan 10, 2021

Final got the clipboard working from viewer to TigerVNC server.
UltraViewer send clipNotify.
TigerServer send clipRequest(actual he is sending multiple clipRequests, one request for each format.
UltraVncViewer answer to each clipRequests with a clipprovide.

https://www.uvnc.eu/download/132/UltraVNC_viewer_132Tiger.zip

Code updated in development branch

Remark:
When the viewer doesn't answer direct, the sever keeps sending clipRequests until he get a clipProvide.
If the viewer doesn't send a clipProvide , then the server clipboard/app lock. (caused by the flood of clipRequests)

For one clipNotify, the viewer get usual 6 cliprequests.. even on local LAN the answer is to slow to prevent the server sending a new request.

You can ask text/rtl/html simultanious in a single clipRequests, why using 3 request ?

@RudiDeVos
Copy link
Member Author

Marcus,
ExtendedDesktopSize is used to allow the viewer "once, on initialization" request a server desktop size.
UltraVNC viewer can request a TigerVNC server desktop size
TigerVNC viewer can request a UltraVNC server desktop size
Current that's the only part supported of the ExtendedDesktopSize protocol.

On windows, you can only clone the current desktop. changing the desktop is changing the is video card resolutuion.
On linux a vnc session is like a RDP session, you can easy resize. There is no need for the on the fly viewer sync to server.

On the fly server framebuffer changes are already implemented with the rfbEncodingNewFBSize protocol.
Didn't reimplemented something that's working.

The ExtendedDesktopSize is also extended to allow virtual displays or use your screen as extra monitor.
Still need to map this out for non UltraVNC servers

@MarcusWichelmann
Copy link

Ah, thank you for clarification.

So the ExtendedDesktopSize protocol is only partially implemented and the server does not use it to inform the clients about later server-side size changes?

Okay, then I'll remove a check from my code to still allow the use of DesktopSize in this case.

MarcusWichelmann added a commit to MarcusWichelmann/MarcusW.VncClient that referenced this issue Jan 13, 2021
@CendioOssman
Copy link

When the viewer doesn't answer direct, the sever keeps sending clipRequests until he get a clipProvide.
If the viewer doesn't send a clipProvide , then the server clipboard/app lock. (caused by the flood of clipRequests)

For one clipNotify, the viewer get usual 6 cliprequests.. even on local LAN the answer is to slow to prevent the server sending a new request.

You can ask text/rtl/html simultanious in a single clipRequests, why using 3 request ?

We just pass along the requests from the application, so this sounds like some application issue. Which application did you test with? I just used a simple xterm here.

And we're just interested in "text" right now. We don't support anything fancier yet. :)

This new version breaks server to client for me. The new viewer sets the length for text in caps to 0, so the server only sends a notify when the clipboard changes. I would expect a request packet after that from the viewer, at which point the server would send a provide.

@RudiDeVos
Copy link
Member Author

re-tested viewer UltraVNC server TigerVNC
Only desktop, no app on it
Vns is started
rudi@rudi-Virtual-Machine:~/.vnc$ more xstartup

#!/bin/sh
[ -x /etc/vnc/xstartup ] && exec /etc/vnc/xstartup
[ -r $HOME/.Xresources ] && xrdb $HOME/.Xresources
xsetroot -solid grey
autocutsel -fork
vncconfig -iconic &
vncconfig -nowin &
dbus-launch --exit-with-session gnome-session & 

IN clipCaps
OUT clipNotify <<< ctr-c on the viewer pc
IN clipRequest << request txt
IN clipRequest << request txt
IN clipRequest << request txt
IN clipRequest << request txt
OUT clipProvide >> send txt
IN clipRequest << request txt
OUT clipProvide
IN clipRequest
IN clipRequest
OUT clipProvide
IN clipRequest
IN clipRequest
OUT clipProvide
IN clipRequest
IN clipRequest
OUT clipProvide
IN clipRequest
OUT clipProvide
done, clipdata is transferred and working, paste OK
The server for some reason keeps requesting data until he handled the request..and the viewer keeps answering to all the packages. Lucky it isn't a bitmap of a few MB.

In UltraVNC caps are used to find the common supported clipboard format. Common formats are auto sync.
Because the server caps are all 0, no common format is found and the auto clipboard sync is disabled and a manual request is needed.

Image
As soon as the server send a notify message, the format is added as indicate some data type is available. (v Text(Unicode)
Pressing "request All Formats Now.." download the clipboard data.
If the server would have set txt to on in the caps, this format would be autosynced. Rtf,Html and bitmaps would require a manual request.

afbeelding

I hopes this clarify it.

@RudiDeVos
Copy link
Member Author

Latest development build to fix TigerVNC issue can be found
http://www.uvnc.eu/download/133/

@CendioOssman
Copy link

The server for some reason keeps requesting data until he handled the request..and the viewer keeps answering to all the packages. Lucky it isn't a bitmap of a few MB.

Yeah, that's indeed inefficient. Not clear how to improve it though and also staying reliable. The protocol has no way of indicating a failure so we aren't guaranteed a response, and hence can't just wait.

I'm unable to reproduce the issue in my end though. At best I'm able to get two requests, but those as handled by the clipboard cache so it's never sent to the client.

Do the requests come right away? Or when you try to paste something?

I can see you have "autocutsel" in your xstartup. That is a very spammy clipboard client that fetches the clipboard twice a second. So it could cause some extra requests. But even with that running here it gets the clipboard from the cache and I only see a single request to the client.

In UltraVNC caps are used to find the common supported clipboard format. Common formats are auto sync.
Because the server caps are all 0, no common format is found and the auto clipboard sync is disabled and a manual request is needed.

But there is both the caps flags, and the sizes. So it seemed odd that the size would indicate support? Isn't the flag enough?

The TigerVNC sets the "Text" caps flag, indicating that it supports text. But it also sets the size to 0 since it doesn't want to get any data unless an application is actually interested.

@RudiDeVos
Copy link
Member Author

RudiDeVos commented Feb 4, 2021

I erxplained it wrong
Caps flags indicate what you support
Size indicate provide.
Server caps txt=1 size 0: I support TXT and provide me the TXT when < 0 else send a Notification. -> Alwyas send Notification
Viewer send Caps 1 with size 10000: I support TXT and provide me the TXT when size < 10000 else send a notify.
Viewer send Caps 1 with size 10000: I support RTFand provide me the RTF when size < 10000 else send a notify.
Viewer send Caps 1 with size 10000: I support HTML and provide me the HTML when size < 10000 else send a notify.
If we both send what the other site request i should work.

@CendioOssman
Copy link

I think everything should be in place in that case. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants