Skip to content

Commit

Permalink
Server: Don't lock up if network connection drops
Browse files Browse the repository at this point in the history
Regression introduced by 94d4718

Referring to the discussions at
https://groups.google.com/g/turbovnc-users/c/NzoZq_mIuP0/m/znjQm-3uAQAJ
and
TigerVNC/tigervnc@a99d14d#r57748408
the overhauled congestion control algorithms
(a0f5670,
ffdb526,
9f31536) caused a regression whereby
large framebuffer updates were sometimes not delivered in a timely
manner over high-latency connections.  My attempt to fix the regression
(94d4718) caused another issue (#359)
whereby Xvnc froze if the network connection dropped and a connected VNC
viewer exited or disconnected before the network connection was restored
(thus causing the TCP connection to become orphaned at the server end.)

More specifically, the TigerVNC v1.9+ implementation of the congestion
control algorithms treats an uncongested ETA of 0 and -1 as congested,
but it only sets a deferred update timer if the ETA is >= 0.  If
congestion control is reset, due to an idle connection, before the
deferred update can be sent, then the update is effectively dropped
(spoiled.)  94d4718 modified the
algorithms so that an uncongested ETA of 0 or -1 was treated as
uncongested, but when the network connection dropped, that caused the
dropped connection to be treated as uncongested rather than congested.
Thus, the TurboVNC Server attempted to send framebuffer updates through
the dropped connection, which quickly filled the TCP buffers and caused
write()/SSL_write() to block.  When the VNC viewer disconnected or
exited, the TCP connection became orphaned at the server end, so
write()/SSL_write() continued to block when the network connection was
restored.  The orphaned TCP connection was eventually disconnected,
usually after many minutes, but Xvnc could not do anything else in the
meantime.

Fixes #359
  • Loading branch information
dcommander committed Feb 8, 2023
1 parent fbaadc2 commit 7e7ff2f
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 9 deletions.
4 changes: 4 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ algorithms that support certificates, such as
`rsa-sha2-256-cert-v01@openssh.com`. This can be changed by specifying the
`PubkeyAcceptedAlgorithms` keyword in an OpenSSH config file.

6. Fixed a regression introduced by 3.0 beta1[24] that caused the TurboVNC
Server to become unresponsive if the network connection dropped and a VNC
viewer disconnected before the network connection was restored.


3.0.2
=====
Expand Down
14 changes: 5 additions & 9 deletions unix/Xvnc/programs/Xserver/hw/vnc/flowcontrol.c
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
* flowcontrol.c - implement RFB flow control extensions
*/

/* Copyright (C) 2012, 2014, 2017-2018, 2021 D. R. Commander.
* All Rights Reserved.
/* Copyright (C) 2012, 2014, 2017-2018, 2021, 2023 D. R. Commander.
* All Rights Reserved.
* Copyright (C) 2018 Peter Åstrand for Cendio AB. All Rights Reserved.
* Copyright (C) 2011, 2015 Pierre Ossman for Cendio AB. All Rights Reserved.
*
Expand Down Expand Up @@ -297,13 +297,9 @@ Bool rfbIsCongested(rfbClientPtr cl)
return FALSE;

eta = GetUncongestedETA(cl);
if (eta >= 1) {
cl->congestionTimer = TimerSet(cl->congestionTimer, 0, eta,
congestionCallback, cl);
return TRUE;
}

return FALSE;
cl->congestionTimer = TimerSet(cl->congestionTimer, 0, eta <= 0 ? 1 : eta,
congestionCallback, cl);
return TRUE;
}


Expand Down

0 comments on commit 7e7ff2f

Please sign in to comment.