VirtualBox

Ticket #13722 (closed defect: fixed)

Opened 3 years ago

Last modified 3 years ago

ConsoleImpl.cpp race condition

Reported by: a.urakov Owned by:
Priority: major Component: other
Version: VirtualBox 4.3.20 Keywords: race condition, ConsoleImpl.cpp
Cc: Guest type: Windows
Host type: Linux

Description

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.

Attachments

2014-12-30-07-45-23.053-VBoxHeadless-14685.log.bz2 Download (90.2 KB) - added by a.urakov 3 years ago.
VBoxHeadless log
ConsoleImpl.diff Download (7.7 KB) - added by frank 3 years ago.

Change History

Changed 3 years ago by a.urakov

VBoxHeadless log

comment:1 Changed 3 years ago by frank

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

comment:2 Changed 3 years ago by a.urakov

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

comment:3 Changed 3 years ago by frank

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

comment:4 Changed 3 years ago by a.urakov

Thank you!

Changed 3 years ago by frank

comment:5 Changed 3 years ago by frank

Attached the diff against VBox 4.3.20.

comment:6 Changed 3 years ago by a.urakov

Thank you again!

comment:7 Changed 3 years ago by frank

  • Status changed from new to closed
  • Resolution set to fixed

Fix is part of VBox 4.3.22.

Note: See TracTickets for help on using tickets.

www.oracle.com
ContactPrivacy policyTerms of Use