ConsoleImpl.cpp race condition

[a.urakov]

There is possible race condition in doMediumChange function of ConsoleImpl.cpp. It appears in code

    /*
     * Call worker in EMT, that's faster and safer than doing everything
     * using VMR3ReqCall. Note that we separate VMR3ReqCall from VMR3ReqWait
     * here to make requests from under the lock in order to serialize them.
     */
    PVMREQ pReq;
    int vrc = VMR3ReqCallU(pUVM, VMCPUID_ANY, &pReq, 0 /* no wait! */, VMREQFLAGS_VBOX_STATUS,
                           (PFNRT)changeRemovableMedium, 8,
                           this, pUVM, pszDevice, uInstance, enmBus, fUseHostIOCache, aMediumAttachment, fForce);

    /* release the lock before waiting for a result (EMT will call us back!) */
    alock.release();

    if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
    {
        vrc = VMR3ReqWait(pReq, RT_INDEFINITE_WAIT);
        AssertRC(vrc);
        if (RT_SUCCESS(vrc))
            vrc = pReq->iStatus;
    }
    VMR3ReqFree(pReq);

when EMT thread completes request earlier than VMR3ReqWait (with zero wait time) in VMR3ReqQueue function is called:

        /*
         * Notify EMT.
         */
        if (pUVM->pVM)
            VMCPU_FF_SET(pVCpu, VMCPU_FF_REQUEST);
        VMR3NotifyCpuFFU(pUVCpu, fFlags & VMREQFLAGS_POKE ? VMNOTIFYFF_FLAGS_POKE : 0);

        //
        // !!! Request is already completed here !!!
        //

        /*
         * Wait and return.
         */
        if (!(fFlags & VMREQFLAGS_NO_WAIT))
            rc = VMR3ReqWait(pReq, cMillies);
        LogFlow(("VMR3ReqQueue: returns %Rrc\n", rc));

In this case VMR3ReqWait of VMR3ReqQueue function (and all its callers VMR3ReqCallVU and VMR3ReqCallU) returns VINF_SUCCESS instead of VERR_TIMEOUT and decreases pReq->EventSem. So VMR3ReqWait(pReq, RT_INDEFINITE_WAIT) of doMediumChange indefinitely waits.

Possible solution (works for us) is to call VMR3ReqWait of doMediumChange only when it was not completed:

    if (vrc == VERR_TIMEOUT || RT_SUCCESS(vrc))
    {
        if (!RT_SUCCESS(vrc))
            vrc = VMR3ReqWait(pReq, RT_INDEFINITE_WAIT);
        AssertRC(vrc);
        if (RT_SUCCESS(vrc))
            vrc = pReq->iStatus;
    }

A similar problem exists in doCPURemove, doCPUAdd, doStorageDeviceAttach, doStorageDeviceDetach etc.

[a.urakov]

VBoxHeadless log

[Frank Mehnert]

I think you are right. Though passing VMREQFLAGS_NO_WAIT and adapting the caller code would be probably more correct.

[a.urakov]

But (if I'm not mistaken) function VMR3ReqCallU doesn't return pReq (needed for waiting) when passing VMREQFLAGS_NO_WAIT flag?

[Frank Mehnert]

Thanks again, you are right! We will discuss how to fix this.

[a.urakov]

Thank you!

[Frank Mehnert]

Attached the diff against VBox 4.3.20.

[a.urakov]

Thank you again!

[Frank Mehnert]

Fix is part of VBox 4.3.22.