[libvirt-users] Making the Thread-safety issues with vbox driver ?

[Jean-Baptiste Rouault]

On Thursday 19 April 2012 16:23:19 Matthias Bolte wrote:
> Am 19. April 2012 12:51 schrieb Jean-Baptiste Rouault
> > 
> > A global mutex is needed in the vbox driver to protect access to
> > g_pVBoxGlobalData, the vboxRegister() function seems to be the best place
> > to initialize such a mutex unless there is another entry point to do
> > this ?
> 
> Such a mutex doesn't help.
> 
> Looking at g_pVBoxGlobalData tells me that we need to get rid of it in
> its current form for different reasons. The major reason is that it
> contains connection specific data such as the conn and domainEvents
> members. This means when you open a new connection you break all other
> open connections because connection specific data is overwritten. We
> need to redesign this.
> 
> A major reason for the existence of g_pVBoxGlobalData is given in line
> 209 of vbox_tmpl.c: g_pVBoxGlobalData needs to be global because event
> callbacks won't work otherwise. Actually that's not true. Who ever did
> the original implementation of this was not aware of how COM (MS and
> XP variants) works.
> 
> There is a vboxAllocCallbackObj function that creates an
> IVirtualBoxCallback COM object. The first member in a COM objects is a
> pointer to its vtbl that contains pointers to all the methods that can
> be called on it. After this vtbl the COM object contains its private
> data members, this aspect is not used in the current implementation.
> 
> So we could just allocate a bigger object and put the data that
> belongs to the IVirtualBoxCallback implementation into this additional
> memory. This includes at least vboxCallBackRefCount and domainEvents
> that are currently located in g_pVBoxGlobalData.
> 
> The only two members of g_pVBoxGlobalData that can stay global are
> caps and pFuncs, all other members are specific to a connection and
> cannot be global.

I'm not familiar with COM, but if all this connection-specific code can be 
moved out it's nice.

> Are you referring to data->pFuncs->pfnComInitialize and
> data->pFuncs->pfnComUninitialize when you say "VirtualBox C bindings
> initialization and uninitialization functions"? As those are used to
> create connection specific objects we cannot move them out of
> vboxOpen/vboxClose. If they are not thread-safe than we need to put a
> global mutex around these calls.

Yes these two functions aren't thread-safe, it is even worse than that.
They are located in src/VBox/Main/cbinding/VBoxXPCOMC.cpp in the VirtualBox 
source code. There are 4 static pointers which are replaced at each 
pfnComInitialize call, and "released" (via the NS_RELEASE macro) in 
pfnComUninitialize.

These accesses aren't protected at all, but what is worse is that when you 
call pfnComUninitialize, the pointers to the IVirtualBox, ISession and 
nsIEventQueue are released. So if you open multiple connections, then close 
one of them, these objects are deleted and your last opened connection is 
broken.

As you said, a global mutex is needed around these calls, but I think we 
should also use the VBOX_ADDREF() macro on the IVirtualBox, ISession and 
nsIEventQueue objects after pfnComInitialize, and VBOX_RELEASE() after 
pfnComUninitialize.

-- 
Jean-Baptiste ROUAULT
Ingénieur R&D - diateam : Architectes de l'information
Phone : +33 (0)2 98 050 050 Fax : +33 (0)2 98 050 051