GUACAMOLE-1196: Implement support for VNC display size updates

[necouchman]

This PR takes a crack at implementing the recently-added libvncclient support for SendExtDesktopSize(), which should allow Guacamole as a VNC client to send its size to VNC servers that support such functionality and have the display dynamically resized.

I'm creating as a draft for the moment since I have not actually tested the functionality. Among the things I'm not sure of:

• The RDP protocol contains a resolution setting which specifies the DPI resolution of the screen. I don't see any reason to try to mirror this into the VNC protocol, as I don't think VNC actually does anything with it (I think the DPI is fixed), but if I'm wrong let me know and I can add that support and try to update the code accordingly.
• In the Jira issue (https://issues.apache.org/jira/browse/GUACAMOLE-1196), @mike-jumper mentioned a way of possibly sending the resize command/message using WriteToRFBServer() - I'm wondering if it's worth trying to implement the display update functionality with that in cases where libvncclient doesn't contain the SendExtDesktopSize() function?
• I added the message_lock mutex for sending this message, as in the RDP protocol, but I don't know if there are other places where this lock should be used (authentication, for example), or if it's even required in this scenario?
• I moved a few bits from the RDP protocol to the common/display.h file - among them are the minimum and maximum display size. It wasn't clear to me from the comments in the code whether those minimum and maximum sizes are arbitrary or if there's some particular reason those values are used? Not sure if there's something specific to RDP that has those bounds?

[necouchman]

Okay, did some initial testing, and the desktop size messages are being sent correctly through to the VNC server. Unfortunately I'm hitting an issue with the VNC server (tigervnc) where it is rejecting the size updates due to missing modelines for Xvnc. Obviously having to load all the possible modelines for all of the possible resolutions that Guacamole might request is a bit silly, so I'm thinking that I might need to find another VNC server that doesn't have this requirement.

Good news, though, is that the changes on the Guacamole side seem sound.

[DrummyFloyd]

Hi , any update regarding this wonderfull and waited PR ? =D

[necouchman]

Hi , any update regarding this wonderfull and waited PR ? =D

Looks like Mike has started reviewing it. Any chance you'd be able to test it out? I've been unable to get access to a VNC server that actually supports it...

[DrummyFloyd]

Hi , any update regarding this wonderfull and waited PR ? =D

Looks like Mike has started reviewing it. Any chance you'd be able to test it out? I've been unable to get access to a VNC server that actually supports it...

tbh, i only know tigerVnc, i can try the latest one , because it seems taht the options is presnet in latest tigerVNC + vncserver =>-AcceptSetDesktopSize

[necouchman]

Hi , any update regarding this wonderfull and waited PR ? =D

Looks like Mike has started reviewing it. Any chance you'd be able to test it out? I've been unable to get access to a VNC server that actually supports it...

tbh, i only know tigerVnc, i can try the latest one , because it seems taht the options is presnet in latest tigerVNC + vncserver =>-AcceptSetDesktopSize

If you're able to give it a try with the changes from this PR, that'd be great. I'm also using TigerVNC, and, according to the Xvnc man page, AcceptSetDesktopSize is enabled by default, so it should be working, but I was getting errors indicating that any resolution I wanted to set needed a modeline in the X configuration, which just isn't tenable.

[necouchman]

Okay, I was able to test it and am successfully sending the SendExtDesktopSize() message size, but immediately after the resize it's failing on the HandleRFBServerMessage() call, here:

https://github.com/necouchman/guacamole-server/blob/12c98e331c1dc85b36f2c194fc067ba7fbd29cfe/src/protocols/vnc/vnc.c#L473-L479

So, something about the resize is not working correctly - I'm not sure if there's something that needs to be updated in the rfbClient when the resize is sent, or it's a bug, or...?

[x-7]

Hi , any update regarding this wonderfull and waited PR ? =D

Looks like Mike has started reviewing it. Any chance you'd be able to test it out? I've been unable to get access to a VNC server that actually supports it...

tbh, i only know tigerVnc, i can try the latest one , because it seems taht the options is presnet in latest tigerVNC + vncserver =>-AcceptSetDesktopSize

If you're able to give it a try with the changes from this PR, that'd be great. I'm also using TigerVNC, and, according to the Xvnc man page, AcceptSetDesktopSize is enabled by default, so it should be working, but I was getting errors indicating that any resolution I wanted to set needed a modeline in the X configuration, which just isn't tenable.

[necouchman]

@x-7 Thank you for testing! I'm glad to hear that it works for you - maybe the error that I'm currently seeing is actually a bug in the VNC Server, and not something with how I've implemented it. I'll try to find some additional VNC servers to test with - maybe even grab an eval copy of RealVNC or something like that and see if that works.

[x-7]

@x-7 Thank you for testing! I'm glad to hear that it works for you - maybe the error that I'm currently seeing is actually a bug in the VNC Server, and not something with how I've implemented it. I'll try to find some additional VNC servers to test with - maybe even grab an eval copy of RealVNC or something like that and see if that works.

@x-7 Thank you for testing! I'm glad to hear that it works for you - maybe the error that I'm currently seeing is actually a bug in the VNC Server, and not something with how I've implemented it. I'll try to find some additional VNC servers to test with - maybe even grab an eval copy of RealVNC or something like that and see if that works.

yes,I used two clients for testing: next-terminal(https://github.com/dushixiang/next-terminal) and gucalmole-client.

1. next-terminal "autoresize" is ok, but when the mouse is placed on the edge of the window, the mouse style does not change to the zoom style.

2. gucalmole-client client "autoresize" is also ok

[necouchman]

Well, finally tracked down what's going on, here, and it appears that there is a bug in libvncclient, and not all of the required data is initialized to send a screen update. In particular, the RFB protocol specifies that a the screen update message should include the screen id, x and y offset, width and height, and (unused but initialized) flags. libvncclient currently only initializes and sends the x and y offset, resulting in bogus data being sent for the id, offset, and flags. While some VNC servers may be forgiving of this (or ignore it), TigerVNC does not, and this results in its refusal to resize correctly.

I've put in a pull request in the LibVNC/libvncserver repo to correct this, and it appears to work. However, until they accept that and release a new version, this effectively will not work. The options, then, are:

• Don't merge this code until libvncserver/libvncclient has a release that will actually make use of it.
• Merge this code, anyway, with a warning that it will probably not work with most VNC servers.
• Write a function that uses the WriteToRFBServer function and generate our own resize message, with the expected data, and then update Guacamole to use the corrected/built-in method when it gets fixed.

I'm tempted to go with the last option so that this feature can be used, so I'll see what I can come up with.

[necouchman]

Okay - one more change I've tried to make is to set the initial display size as soon as the connection is active; however, I'm having trouble making this work correctly, as the places where I could do it (as part of the user join handler, or before the main client loop for the VNC protocol) don't actually work because the display isn't fully initialized, yet. If I put it inside the main CLIENT_RUNNING loop, the callback runs repeatedly, which isn't the end of the world, but also isn't ideal - it really only needs to run as soon as the VNC connection is fully established. I'm open to any hints anyone has on better placement...

[x-7]

@necouchman
i found the screen id is alway 0 return from tigervncserver,which will no pass the screen validation
finally, i fix my resize problem by the pr
and it still work ok when i comment the code guac_client_for_owner(client, guac_vnc_display_set_owner_size, rfb_client);

[necouchman]

Thanks @x-7, appreciate the testing and information on libvncclient. Sounds like we'll still need something to bridge the gap until they accept that pull request and it gets released, so I'll leave most of this intact as-is.

[michaelwangroy]

Sounds like the function sends request to vnc server which attempts to resize remote desktop. i'm wondering what will happen when multiple users resize the screen at the same time?

[necouchman]

Sounds like the function sends request to vnc server which attempts to resize remote desktop. i'm wondering what will happen when multiple users resize the screen at the same time?

@michaelwangroy: Two points:

• From a Guacamole concurrency perspective, only the owner of the connection can resize the display - if you're sharing the connection via Guacamole, then additional, non-owner Guacamole users will not resize the display.
• If you have multiple users connecting over VNC, either from different clients, or who are all owners of their own individual Guacamole connections to it (the VNC server allows multiple clients to connect simultaneously), then this is up to the VNC server how to handle this, and there isn't really anything Guacamole can - or should - do to mitigate this. This could easily be an issue with people connecting from non-Guacamole VNC clients, and presumably VNC servers have been written to handle these situations.

[michaelwangroy]

Sounds like the function sends request to vnc server which attempts to resize remote desktop. i'm wondering what will happen when multiple users resize the screen at the same time?

@michaelwangroy: Two points:

• From a Guacamole concurrency perspective, only the owner of the connection can resize the display - if you're sharing the connection via Guacamole, then additional, non-owner Guacamole users will not resize the display.
• If you have multiple users connecting over VNC, either from different clients, or who are all owners of their own individual Guacamole connections to it (the VNC server allows multiple clients to connect simultaneously), then this is up to the VNC server how to handle this, and there isn't really anything Guacamole can - or should - do to mitigate this. This could easily be an issue with people connecting from non-Guacamole VNC clients, and presumably VNC servers have been written to handle these situations.

Got it, thanks! It seems to me that there could be a toggle for the feature somewhere. Because, as you mentioned, it could easily be an issue under certain circumstances.

[necouchman]

Got it, thanks! It seems to me that there could be a toggle for the feature somewhere. Because, as you mentioned, it could easily be an issue under certain circumstances.

Seems reasonable - I've added the option, here, to the server code - I'll just need to add the matching guacamole-client option, as well.

[Zer0Risk]

I don't know if feedback is acceptable this way, but I used this PR and it worked great, thank you so much. 👍

[necouchman]

Thanks @Zer0Risk, appreciate the testing and confirmation!