TurboVNC / turbovnc

Main TurboVNC repository
https://TurboVNC.org
GNU General Public License v2.0
746 stars 136 forks source link

Abrupt client disconnection can cause pointer to stop working #417

Closed marc-legendre closed 5 hours ago

marc-legendre commented 3 days ago

Context

There is a locking mechanism for pointer events in rfbserver.c where a client "owns" pointer events while dragging the pointer to avoid interference from other clients during collaborative sessions.

Issue

We have observed deadlock issues when a client disconnects abruptly while dragging the pointer (and thus holding the lock). Server-side, the connection can stay open for a while, and the lock is kept until the connection closes. Meanwhile, if the client reconnects to the server, all its pointer events will be ignored.

This issue regularly affected a user whose ungodly network configuration caused their IP address to change frequently. I could reproduce the issue by deactivating the network while dragging the mouse.

Workaround

It is important for us to be robust to such issues. So I worked around it by implementing a timeout on the lock. More precisely, the lock is freed automatically after a short period without new pointer events from the client holding the lock.

This solution has worked well for us, so I thought I'd share my understanding of the issue, as well as my implementation (0001-Add-a-timeout-to-the-pointer-lock-during-drags.patch.txt). Perhaps you might consider integrating something similar, otherwise this issue could serve as documentation for others who encounter this problem :-)

Risk Analysis

What can go wrong? The lock might be freed too soon, which can result in:

dcommander commented 3 days ago

The mutex in question dates back to TightVNC 1.2.9, and I notice that TigerVNC uses a similar solution to yours (although they don't allow the timeout to be adjusted from its default value of 10 seconds.)

Playing devil's advocate, would it be possible to also address the issue by twiddling the RFB client timeout (e.g. by passing -rfbwait 3000 to the server?) Or is it desirable to have a separate timeout for the pointer?

dcommander commented 2 days ago

I can confirm that -rfbwait does not address the issue. Proceeding with integrating your patch (although I would like to make it a legitimate option rather than an environment variable.)

dcommander commented 5 hours ago

I implemented the feature in a more product-friendly manner, using a documented Xvnc option rather than an environment variable, but otherwise it should work the same as your patch.

dcommander commented 5 hours ago

Please feel free to submit any other downstream patches that you think might be applicable upstream.