Segfault in x0vncserver after client disconnects

[gujjwal00]

Describe the bug After client disconnects from server, segfault is triggered in Timer callback VNCServerST::handleTimeout(). I can hit it about 7 times out of 10.

To Reproduce

1. Run x0vncserver and connect to it (tried from AVNC on Android and Remmina on Linux Mint)
2. Wait couple of seconds and disconnect the client
3. Server crashes

Looks like when client's socket is being removed, XDesktop is stopped, which clears pixel buffer, which cleans up the comparer field. git bisect points towards 28c3f121613807df6d53dde9ac653916dcf8902d as the culprit. Fix seems to be a simple null check but I don't know if there is a deeper issue because it doesn't happen every time.

[poppcicle]

I'm experiencing this too. Valgrind log:

==3183272== Invalid read of size 8
==3183272==    at 0x137E16: non-virtual thunk to rfb::VNCServerST::handleTimeout(rfb::Timer*) (in /usr/local/bin/x0vncserver)
==3183272==    by 0x1345B9: rfb::Timer::checkTimeouts() (in /usr/local/bin/x0vncserver)
==3183272==    by 0x11EE0D: main (in /usr/local/bin/x0vncserver)
==3183272==  Address 0x0 is not stack'd, malloc'd or (recently) free'd
==3183272== 
==3183272== 
==3183272== Process terminating with default action of signal 11 (SIGSEGV)
==3183272==  Access not within mapped region at address 0x0
==3183272==    at 0x137E16: non-virtual thunk to rfb::VNCServerST::handleTimeout(rfb::Timer*) (in /usr/local/bin/x0vncserver)
==3183272==    by 0x1345B9: rfb::Timer::checkTimeouts() (in /usr/local/bin/x0vncserver)
==3183272==    by 0x11EE0D: main (in /usr/local/bin/x0vncserver)
==3183272==  If you believe this happened as a result of a stack
==3183272==  overflow in your program's main thread (unlikely but
==3183272==  possible), you can try to increase the size of the
==3183272==  main thread stack using the --main-stacksize= flag.
==3183272==  The main thread stack size used in this run was 8388608.
==3183272== 
==3183272== HEAP SUMMARY:
==3183272==     in use at exit: 225,532 bytes in 1,387 blocks
==3183272==   total heap usage: 21,404 allocs, 20,017 frees, 44,235,557 bytes allocated
==3183272== 
==3183272== LEAK SUMMARY:
==3183272==    definitely lost: 180 bytes in 3 blocks
==3183272==    indirectly lost: 12,300 bytes in 43 blocks
==3183272==      possibly lost: 0 bytes in 0 blocks
==3183272==    still reachable: 213,052 bytes in 1,341 blocks
==3183272==         suppressed: 0 bytes in 0 blocks
==3183272== Rerun with --leak-check=full to see details of leaked memory
==3183272== 
==3183272== For lists of detected and suppressed errors, rerun with: -s
==3183272== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 25780 from 1)

I tried bisecting before I saw this issue and arrived at a different commit, https://github.com/TigerVNC/tigervnc/commit/37c9cedb21b48ac8a13e4e34f39a83faefb7d8c1, but it's possible I did something wrong. EDIT - my bisect is definitely wrong; I must've not tested some commit well enough

[CendioOssman]

Your analysis looks correctly. It looks like we are checking things in the wrong order. Can you check if this patch helps:

diff --git a/common/rfb/VNCServerST.cxx b/common/rfb/VNCServerST.cxx
index a8fa1739d..a39b046dd 100644
--- a/common/rfb/VNCServerST.cxx
+++ b/common/rfb/VNCServerST.cxx
@@ -627,7 +627,7 @@ void VNCServerST::handleTimeout(Timer* t)

     // We keep running until we go a full interval without any updates,
     // or there are no active clients anymore
-    if (comparer->is_empty() || !desktopStarted) {
+    if (!desktopStarted || comparer->is_empty()) {
       // Unless something waits for us to advance the frame count
       if (queuedMsc < msc)
         return;
@@ -642,7 +642,7 @@ void VNCServerST::handleTimeout(Timer* t)

     frameTimer.repeat(timeout);

-    if (!comparer->is_empty() && desktopStarted)
+    if (desktopStarted && !comparer->is_empty())
       writeUpdate();

     msc++;

[poppcicle]

Works for me; no more crash

[gujjwal00]

Yes, the patch works for me too.

[CendioOssman]

Thanks for testing! It is fixed in c4075860579ad33011fafee32f34c5d3da71a910, and will also be included in TigerVNC 1.14.0.