VNC extension not initialised on Xorg server regeneration

[twaugh]

In tigervnc 1.3.0, with the extension module configured, the VNC extension is lost on server regeneration.

Here's how to demonstrate the problem:

1.Boot to text mode so no X server is running
2.Xorg &
3.DISPLAY=:0 xterm &
4.In the xterm, check the extension is loaded: xdpyinfo | grep VNC
5.Also, from another system, try connecting a vncviewer
6.Now exit the xterm
7.Repeat steps 3-5

The second time xdpyinfo is run, VNC-EXTENSION is not listed.
The second time a vncviewer is run, it connects but just sits there and does not negotiate encodings.

I think it's due to the 'static char once' guard in vncExtensionInitWithParams() in xf86vncModule.cc, introduced in commit fc23895:

Author: Adam Tkac [email protected]
Date: Thu Mar 14 17:52:53 2013 +0000

Initialize VNC extension from libvnc.so only once.

Setting a breakpoint on that function and manually invoking vncExtensionInit() causes xdpyinfo to list VNC-EXTENSION again, but vncviewer still has trouble connecting so it isn't as simple as that.

Original report:
https://bugzilla.redhat.com/show_bug.cgi?id=1116956

[CendioOssman]

Have you tried completely backing out fc23895?

Your upstream bug also references another bug, where this change supposedly originated. Unfortunately that second bug is restricted. Could you open it up so we can see the reason for the change?

[twaugh]

Completely backing out fc23895 shows up the reason for the original change: once the server regenerates, it gets stuck in a loop here:

#3  0x00007f51359dd9f8 in vncExtensionInitWithParams () at xf86vncModule.cc:105
#4  0x00000000004c0441 in InitExtensions (argc=argc@entry=1, 
    argv=argv@entry=0x7fffef19eef8) at ../../../mi/miinitext.c:337
#5  0x00000000004286c0 in main (argc=1, argv=0x7fffef19eef8, 
    envp=<optimized out>) at main.c:208

...which is this loop:

100     for (ParameterIterator i; i.param; i.next()) {
101       const char *val;
102 #if XORG < 112
103       val = xf86FindOptionValue(pScrn->options, i.param->getName());
104 #else
105       val = xf86FindOptionValue((XF86OptionPtr)pScrn->options, i.param->getName());
106 #endif
107       if (val)
108         i.param->setParam(val);
109     }

The trouble is that rfb::Configuration has an element whose next element is itself, and that happens at this point:

Old value = (rfb::Configuration *) 0x0
New value = (rfb::Configuration *) 0x291c080
0x00007f2700a011e0 in appendConfiguration ()
    at ../../../../common/rfb/Configuration.h:152
152       conf->_next = _next; _next = conf;
(gdb) bt
#0  0x00007f2700a011e0 in appendConfiguration ()
    at ../../../../common/rfb/Configuration.h:152
#1  enableServerParams () at ../../../../common/rfb/Configuration.h:104
#2  vncExtensionInitWithParams () at xf86vncModule.cc:95
#3  0x00000000004c1e79 in InitExtensions (argc=<value optimized out>, 
    argv=<value optimized out>) at ../../../mi/miinitext.c:334
#4  0x000000000047c9db in main (argc=4, argv=<value optimized out>, 
    envp=<value optimized out>) at main.c:208

I hope to make the bug describing this public soon (making sure there are no objections), but those are the details from it.

[twaugh]

https://bugzilla.redhat.com/show_bug.cgi?id=1004093 is public now.

[twaugh]

Moving the 'once' guard so that only rfb::Configuration::enableServerParams() is guarded improves the situation a bit, but now it looks like the InputDevices need re-initialisation. Everything works as expected when closing xterm and starting a new xterm, except for the occasional segfault like:

#0  0x00007f8557012c86 in InputDevice::getKeyboardState (
    this=this@entry=0x1514220) at InputXKB.cc:235
#1  0x00007f8557011fad in InputDevice::keyEvent (this=0x1514220, keysym=65507, 
    down=<optimized out>) at Input.cc:473
#2  0x00007f8557042f91 in rfb::VNCSConnectionST::keyEvent (this=0x1dc3260, 
    key=65507, down=<optimized out>)
    at /usr/src/debug/tigervnc-1.3.0/common/rfb/VNCSConnectionST.cxx:546
#3  0x00007f8557044327 in rfb::VNCSConnectionST::processMessages (
    this=0x1dc3260)
    at /usr/src/debug/tigervnc-1.3.0/common/rfb/VNCSConnectionST.cxx:165
#4  0x00007f855700e89f in XserverDesktop::wakeupHandler (this=0x1512760, 
    fds=fds@entry=0x8285c0 <LastSelectMask>, nfds=nfds@entry=1)
    at XserverDesktop.cc:679
#5  0x00007f85570063c4 in vncWakeupHandler (data=<optimized out>, nfds=1, 
    readmask=0x8285c0 <LastSelectMask>) at vncExtInit.cc:321
#6  0x000000000043e60d in WakeupHandler (result=result@entry=1, 
    pReadmask=pReadmask@entry=0x8285c0 <LastSelectMask>) at dixutils.c:423
#7  0x000000000046a85f in WaitForSomething (
    pClientsReady=pClientsReady@entry=0x1d588f0) at WaitFor.c:224
#8  0x0000000000439f71 in Dispatch () at dispatch.c:361
#9  0x000000000042888a in main (argc=1, argv=0x7fff53a36688, 
    envp=<optimized out>) at main.c:298

[CendioOssman]

Looking at the code, it looks like everything except vncExtensionInit() should be protected by once.

As for the input devices, it does indeed look suspect. I have not experienced any crashes with Xvnc though, which should suffer from the same issue. I'll have to investigate and see how it is best solved.

[CendioOssman]

I have not experienced any crashes with Xvnc though, which should suffer from the same issue.

Aha. Xvnc has a hack that prevents server resets:

static void vfbClientStateChange(CallbackListPtr*, pointer, pointer) {
  dispatchException &= ~DE_RESET;
}

Should probably remove that. If nothing else so that it behaves the same as libvnc.so.

[CendioOssman]

I'm thinking that maybe all the VNC stuff should also reset as part of the server reset. Two problems though:

a) It will disconnect any VNC clients. Probably not what you want. Maybe delay the reset until there are truly no clients, X11 nor VNC. The Xorg code is not really designed for this though, so it would have to be a bit hacky.

b) It won't avoid the crashes just by doing that. The extensions are removed after the input devices, which means that there might still be an event or two that fires as we clean things up.

[CendioOssman]

Can you test the patch in pull request #6 and see if it solves the issue with the input devices not getting reset?

(Please don't use the patch for anything permanent though. I'll clean it up once I know it actually solves your issue.)

[twaugh]

That works well for me. I'm testing with: fc23895 reverted; an if(!once) guard only around rfb::configuration::enableServerParams(); and your changes in the inputreset branch.

[CendioOssman]

I've updated #6. Could you retest? I've included the fix for the whole once mess as well.

[twaugh]

This works well for me. Thanks! (Sorry, added that comment to the pull request instead of here.)