[vbox-dev] Null pointer access in VMEmt.cpp by vmR3EmulationThreadWithId

[Alexander Boettcher]

Hello,

occasionally we encounter a null pointer access in VMEmt.cpp. We added
an assertion like

+++ src/VBox/VMM/VMMR3/VMEmt.cpp
@@ -156,6 +156,10 @@
             PVM    pVM   = pUVM->pVM;
             PVMCPU pVCpu = pUVCpu->pVCpu;
             enmBefore = pVM->enmVMState;
+
+            Assert(pVM);
+            Assert(pVCpu);
+
             if (pUVM->vm.s.fTerminateEMT)
             {
                 rc = VINF_EM_TERMINATE;

The second assertion triggers from time to time during early bootstrap
of a VM and using multiple vCPUs.

After some debugging in turned out, that the assignment of the pUVM->pVM
pointer in VM.cpp is done to early, so that the actual pUVM->aCpus[] are
not yet initialized.

Following kind of quirk avoid the issue for us:

+++ src/VBox/VMM/VMMR3/VM.cpp
@@ -605,7 +605,7 @@
     rc = SUPR3CallVMMR0Ex(NIL_RTR0PTR, NIL_VMCPUID,
VMMR0_DO_GVMM_CREATE_VM, 0, &CreateVMReq.Hdr);
     if (RT_SUCCESS(rc))
     {
-        PVM pVM = pUVM->pVM = CreateVMReq.pVMR3;
+        PVM pVM = CreateVMReq.pVMR3;
         AssertRelease(VALID_PTR(pVM));
         AssertRelease(pVM->pVMR0 == CreateVMReq.pVMR0);
         AssertRelease(pVM->pSession == pUVM->vm.s.pSession);
@@ -635,6 +635,14 @@
             pUVM->aCpus[i].pVM              = pVM;
         }

+        /*
+         * vmR3EmulationThreadWithId checks (from within another
thread) for
+         * !pUVM->pVM. If not null the function also expects
+         * pUVM->aCpus[i].pVCpu to be not null. So, make the assignment
after
+         * pUVM->aCpus[i] are actually initialized.
+         */
+        ASMCompilerBarrier();
+        pUVM->pVM = pVM;

         /*
          * Init the configuration.


We encountered/have seen the issue occasionally in 5.1.10 - 5.1.16.

Cheers,

-- 
Alexander Boettcher
Genode Labs

http://www.genode-labs.com - http://www.genode.org

Genode Labs GmbH - Amtsgericht Dresden - HRB 28424 - Sitz Dresden
Geschäftsführer: Dr.-Ing. Norman Feske, Christian Helmuth