#1577 tvnserver cash in Win8ScreenDriverImpl::execute() when trying to get screen dump from elevated desktop running with ordinary users rights in RDP session

[Alexander Simonov]

I'm using tightvnc server build from sources got here: https://www.tightvnc.com/download/2.8.63/tightvnc-2.8.63-src-gpl.zip with latest updated Windows 2016 Server in Terminal Services mode (user RDP sessions).

Users start tvnserver just as regular app and connect in reverse mode to viewer (support operator) when needed. Users have just ordinary Windows user rights (not admins). I've noticed that when user is connected to operator and operator will trigger some Windows function that asks elevated rights on system desktop, tvnserver will just crash with error, and user everytime need to restart it.

I've investigated the problem. It looks like tvnserver crashes because it can't get screen dump from elevated system desktop, because users do not have such system rights. I've found a place in sources where the problem resides and fixed it (it looks like there is missing nullptr check):

In file "Win8ScreenDriverImpl.cpp" in function "void Win8ScreenDriverImpl::execute()" I've wrapped "m_duplication->execute()" with null pointer checking for "m_duplication":

if (m_duplication) {
m_duplication->execute();
}

Because earlier when it tries to create "m_duplication" in "void Win8ScreenDriverImpl::initDxgi()" for system elevated desktop, it will throw exception in "new Win8DeskDuplication()" and also raise "m_hasRecoverableError" flag.

So checking for null pointer for "m_duplication" when "execute()" is called just fixed this erroneous behavior and now it works without any hassle.

Glad if this information will help. Thanks.

[Anton]

if (m_hasCriticalError) {
in constructor of Win8ScreenDriverImpl
should be replaced with
if (!isValid()) {

sorry for error, it will be fixed in the next version.
for now you could use previous https://www.tightvnc.com/download/2.8.59/tightvnc-2.8.59-gpl-setup-64bit.msi

[ofv]

Anton,

The change you propose prevents the server crash, but the connection closes. With the change proposed on the bug report, the connection remains.

Here is the diff, which includes both changes:

--- a/desktop/Win8ScreenDriverImpl.cpp
+++ b/desktop/Win8ScreenDriverImpl.cpp
@@ -70,7 +70,7 @@ Win8ScreenDriverImpl::Win8ScreenDriverImpl(LogWriter log, UpdateKeeper updateK
m_log->debug(_T("Win8ScreenDriverImpl init recoverable error"));
}

• if (m_hasCriticalError) {
• if (!isValid()) {
  m_log->debug(_T("Win8ScreenDriverImpl init critical error"));
  terminate();
  wait();
  @@ -182,7 +182,9 @@ void Win8ScreenDriverImpl::initDxgi()

void Win8ScreenDriverImpl::execute()
{

• m_duplication->execute();
• if (m_duplication) {
• m_duplication->execute();
• }

if (!isValid()) {
m_log->error(_T("Win8ScreenDriverImpl has an invalid state. The invalid state can be")