VirtualBox

Changeset 72703 in vbox for trunk/src/VBox


Ignore:
Timestamp:
Jun 27, 2018 3:27:32 PM (7 years ago)
Author:
vboxsync
Message:

Main/Medium: Fix a number of potential deadlocks due to lock order violation. Several are still present and need to be fixed later.

Location:
trunk/src/VBox/Main
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/src/VBox/Main/idl/VirtualBox.xidl

    r72478 r72703  
    1518615186    </attribute>
    1518715187
    15188     <attribute name="description" type="wstring">
     15188    <attribute name="description" type="wstring" wrap-hint-server="passcaller">
    1518915189      <desc>
    1519015190        Optional description of the medium. For a newly created medium the value
    … …  
    1614516145    <!-- other methods -->
    1614616146
    16147     <method name="setLocation">
     16147    <method name="setLocation" wrap-hint-server="passcaller">
    1614816148      <desc>
    1614916149        Changes the location of this medium. Some medium types may support
    … …  
    1630716307    </method>
    1630816308
    16309     <method name="getEncryptionSettings" const="yes">
     16309    <method name="getEncryptionSettings" const="yes" wrap-hint-server="passcaller">
    1631016310      <desc>
    1631116311        Returns the encryption settings for this medium.

  • trunk/src/VBox/Main/include/MediumImpl.h

    r69500 r72703  
    230230    // wrapped IMedium properties
    231231    HRESULT getId(com::Guid &aId);
    232     HRESULT getDescription(com::Utf8Str &aDescription);
    233     HRESULT setDescription(const com::Utf8Str &aDescription);
     232    HRESULT getDescription(AutoCaller &autoCaller, com::Utf8Str &aDescription);
     233    HRESULT setDescription(AutoCaller &autoCaller, const com::Utf8Str &aDescription);
    234234    HRESULT getState(MediumState_T *aState);
    235235    HRESULT getVariant(std::vector<MediumVariant_T> &aVariant);
    … …  
    293293                        const std::vector<MediumVariant_T> &aVariant,
    294294                        ComPtr<IProgress> &aProgress);
    295     HRESULT setLocation(const com::Utf8Str &aLocation,
     295    HRESULT setLocation(AutoCaller &autoCaller,
     296                        const com::Utf8Str &aLocation,
    296297                        ComPtr<IProgress> &aProgress);
    297298    HRESULT compact(ComPtr<IProgress> &aProgress);
    … …  
    302303                             const com::Utf8Str &aNewPassword, const com::Utf8Str &aNewPasswordId,
    303304                             ComPtr<IProgress> &aProgress);
    304     HRESULT getEncryptionSettings(com::Utf8Str &aCipher, com::Utf8Str &aPasswordId);
     305    HRESULT getEncryptionSettings(AutoCaller &autoCaller, com::Utf8Str &aCipher, com::Utf8Str &aPasswordId);
    305306    HRESULT checkEncryptionPassword(const com::Utf8Str &aPassword);
    306307

  • trunk/src/VBox/Main/src-server/MediumImpl.cpp

    r71097 r72703  
    16071607}
    16081608
    1609 HRESULT Medium::getDescription(com::Utf8Str &aDescription)
    1610 {
     1609HRESULT Medium::getDescription(AutoCaller &autoCaller, com::Utf8Str &aDescription)
     1610{
     1611    NOREF(autoCaller);
    16111612    AutoReadLock alock(this COMMA_LOCKVAL_SRC_POS);
    16121613
    … …  
    16161617}
    16171618
    1618 HRESULT Medium::setDescription(const com::Utf8Str &aDescription)
     1619HRESULT Medium::setDescription(AutoCaller &autoCaller, const com::Utf8Str &aDescription)
    16191620{
    16201621    /// @todo update m->strDescription and save the global registry (and local
    … …  
    16281629    try
    16291630    {
     1631        autoCaller.release();
     1632
    16301633        // locking: we need the tree lock first because we access parent pointers
    16311634        // and we need to write-lock the media involved
    1632         uint32_t    cHandles    = 2;
    1633         LockHandle* pHandles[2] = { &m->pVirtualBox->i_getMediaTreeLockHandle(),
    1634                                     this->lockHandle() };
    1635 
    1636         AutoWriteLock alock(cHandles,
    1637                             pHandles
    1638                             COMMA_LOCKVAL_SRC_POS);
     1635        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
     1636
     1637        autoCaller.add();
     1638        AssertComRCThrowRC(autoCaller.rc());
     1639
     1640        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    16391641
    16401642        /* Build the lock list. */
    16411643        alock.release();
     1644        autoCaller.release();
     1645        treeLock.release();
    16421646        rc = i_createMediumLockList(true /* fFailIfInaccessible */,
    16431647                                    this /* pToLockWrite */,
    … …  
    16451649                                    NULL,
    16461650                                    *pMediumLockList);
     1651        treeLock.acquire();
     1652        autoCaller.add();
     1653        AssertComRCThrowRC(autoCaller.rc());
    16471654        alock.acquire();
    16481655
    … …  
    16551662
    16561663        alock.release();
     1664        autoCaller.release();
     1665        treeLock.release();
    16571666        rc = pMediumLockList->Lock();
     1667        treeLock.acquire();
     1668        autoCaller.add();
     1669        AssertComRCThrowRC(autoCaller.rc());
    16581670        alock.acquire();
    16591671
    … …  
    16731685        // save the settings
    16741686        alock.release();
     1687        autoCaller.release();
     1688        treeLock.release();
    16751689        i_markRegistriesModified();
    16761690        m->pVirtualBox->i_saveModifiedRegistries();
    … …  
    19061920    // save the settings
    19071921    mlock.release();
     1922    autoCaller.release();
    19081923    treeLock.release();
    19091924    i_markRegistriesModified();
    … …  
    26382653                                  ComPtr<IProgress> &aProgress)
    26392654{
    2640     /** @todo r=klaus The code below needs to be double checked with regard
    2641      * to lock order violations, it probably causes lock order issues related
    2642      * to the AutoCaller usage. */
    26432655    IMedium *aT = aTarget;
    26442656    ComObjPtr<Medium> diff = static_cast<Medium*>(aT);
    … …  
    26562668    if (FAILED(autoCaller.rc())) return autoCaller.rc();
    26572669
    2658     AutoMultiWriteLock2 alock(this->lockHandle(), diff->lockHandle() COMMA_LOCKVAL_SRC_POS);
     2670    AutoMultiWriteLock2 alock(this, diff COMMA_LOCKVAL_SRC_POS);
    26592671
    26602672    if (m->type == MediumType_Writethrough)
    … …  
    26742686    MediumLockList *pMediumLockList(new MediumLockList());
    26752687    alock.release();
     2688    autoCaller.release();
    26762689    treeLock.release();
    26772690    HRESULT rc = diff->i_createMediumLockList(true /* fFailIfInaccessible */,
    … …  
    26812694                                              *pMediumLockList);
    26822695    treeLock.acquire();
     2696    autoCaller.add();
     2697    if (FAILED(autoCaller.rc()))
     2698        rc = autoCaller.rc();
    26832699    alock.acquire();
    26842700    if (FAILED(rc))
    … …  
    26892705
    26902706    alock.release();
     2707    autoCaller.release();
    26912708    treeLock.release();
    26922709    rc = pMediumLockList->Lock();
    26932710    treeLock.acquire();
     2711    autoCaller.add();
     2712    if (FAILED(autoCaller.rc()))
     2713        rc = autoCaller.rc();
    26942714    alock.acquire();
    26952715    if (FAILED(rc))
    … …  
    27072727        diff->m->llRegistryIDs.push_back(parentMachineRegistry);
    27082728        alock.release();
     2729        autoCaller.release();
    27092730        treeLock.release();
    27102731        diff->i_markRegistriesModified();
    27112732        treeLock.acquire();
     2733        autoCaller.add();
    27122734        alock.acquire();
    27132735    }
    27142736
    27152737    alock.release();
     2738    autoCaller.release();
    27162739    treeLock.release();
    27172740
    … …  
    27392762                        ComPtr<IProgress> &aProgress)
    27402763{
    2741 
    2742     /** @todo r=klaus The code below needs to be double checked with regard
    2743      * to lock order violations, it probably causes lock order issues related
    2744      * to the AutoCaller usage. */
    27452764    IMedium *aT = aTarget;
    27462765
    … …  
    29262945}
    29272946
    2928 HRESULT Medium::setLocation(const com::Utf8Str &aLocation, ComPtr<IProgress> &aProgress)
    2929 {
    2930 
     2947HRESULT Medium::setLocation(AutoCaller &autoCaller, const com::Utf8Str &aLocation, ComPtr<IProgress> &aProgress)
     2948{
    29312949    ComObjPtr<Medium> pParent;
    29322950    ComObjPtr<Progress> pProgress;
    … …  
    29452963    /// this medium), this will also require to add the mRegistered flag to data
    29462964
     2965        autoCaller.release();
     2966
    29472967        // locking: we need the tree lock first because we access parent pointers
    29482968        // and we need to write-lock the media involved
    2949         uint32_t    cHandles    = 2;
    2950         LockHandle* pHandles[2] = { &m->pVirtualBox->i_getMediaTreeLockHandle(),
    2951                                     this->lockHandle() };
    2952 
    2953         AutoWriteLock alock(cHandles,
    2954                             pHandles
    2955                             COMMA_LOCKVAL_SRC_POS);
     2969        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
     2970
     2971        autoCaller.add();
     2972        AssertComRCThrowRC(autoCaller.rc());
     2973
     2974        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    29562975
    29572976        /* play with locations */
    … …  
    30963115
    30973116            alock.release();
     3117            autoCaller.release();
     3118            treeLock.release();
    30983119            rc = m->pVirtualBox->i_findMachine(id, false, true, &aMachine);
     3120            treeLock.acquire();
     3121            autoCaller.add();
     3122            AssertComRCThrowRC(autoCaller.rc());
    30993123            alock.acquire();
    31003124
    … …  
    31053129
    31063130                alock.release();
     3131                autoCaller.release();
     3132                treeLock.release();
    31073133                bool ses = aMachine->i_isSessionOpenVM(sm, &ctl);
     3134                treeLock.acquire();
     3135                autoCaller.add();
     3136                AssertComRCThrowRC(autoCaller.rc());
    31083137                alock.acquire();
    31093138
    … …  
    31233152        MediumLockList *pMediumLockList(new MediumLockList());
    31243153        alock.release();
     3154        autoCaller.release();
     3155        treeLock.release();
    31253156        rc = i_createMediumLockList(true /* fFailIfInaccessible */,
    31263157                                    this /* pToLockWrite */,
    … …  
    31283159                                    NULL,
    31293160                                    *pMediumLockList);
     3161        treeLock.acquire();
     3162        autoCaller.add();
     3163        AssertComRCThrowRC(autoCaller.rc());
    31303164        alock.acquire();
    31313165        if (FAILED(rc))
    … …  
    31373171        }
    31383172        alock.release();
     3173        autoCaller.release();
     3174        treeLock.release();
    31393175        rc = pMediumLockList->Lock();
     3176        treeLock.acquire();
     3177        autoCaller.add();
     3178        AssertComRCThrowRC(autoCaller.rc());
    31403179        alock.acquire();
    31413180        if (FAILED(rc))
    … …  
    35513590}
    35523591
    3553 HRESULT Medium::getEncryptionSettings(com::Utf8Str &aCipher, com::Utf8Str &aPasswordId)
     3592HRESULT Medium::getEncryptionSettings(AutoCaller &autoCaller, com::Utf8Str &aCipher, com::Utf8Str &aPasswordId)
    35543593{
    35553594#ifndef VBOX_WITH_EXTPACK
    … …  
    35603599    try
    35613600    {
     3601        autoCaller.release();
    35623602        ComObjPtr<Medium> pBase = i_getBase();
     3603        autoCaller.add();
     3604        if (FAILED(autoCaller.rc()))
     3605            throw rc;
    35633606        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    35643607
    … …  
    49975040                              bool aWait)
    49985041{
    4999     /** @todo r=klaus The code below needs to be double checked with regard
    5000      * to lock order violations, it probably causes lock order issues related
    5001      * to the AutoCaller usage. */
    50025042    AssertReturn(aProgress != NULL || aWait == true, E_FAIL);
    5003 
    5004     AutoCaller autoCaller(this);
    5005     if (FAILED(autoCaller.rc())) return autoCaller.rc();
    50065043
    50075044    HRESULT rc = S_OK;
    … …  
    50125049    {
    50135050        /* we're accessing the media tree, and canClose() needs it too */
    5014         AutoMultiWriteLock2 multilock(&m->pVirtualBox->i_getMediaTreeLockHandle(),
    5015                                       this->lockHandle()
    5016                                       COMMA_LOCKVAL_SRC_POS);
     5051        AutoWriteLock treelock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
     5052
     5053        AutoCaller autoCaller(this);
     5054        AssertComRCThrowRC(autoCaller.rc());
     5055
     5056        AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
     5057
    50175058        LogFlowThisFunc(("aWait=%RTbool locationFull=%s\n", aWait, i_getLocationFull().c_str() ));
    50185059
    … …  
    50305071            while (m->queryInfoRunning)
    50315072            {
    5032                 multilock.release();
     5073                alock.release();
     5074                autoCaller.release();
     5075                treelock.release();
    50335076                /* Must not hold the media tree lock or the object lock, as
    50345077                 * Medium::i_queryInfo needs this lock and thus we would run
    … …  
    50395082                    AutoReadLock qlock(m->queryInfoSem COMMA_LOCKVAL_SRC_POS);
    50405083                }
    5041                 multilock.acquire();
     5084                treelock.acquire();
     5085                autoCaller.add();
     5086                AssertComRCThrowRC(autoCaller.rc());
     5087                alock.acquire();
    50425088            }
    50435089        }
    … …  
    50975143        /* Build the medium lock list. */
    50985144        MediumLockList *pMediumLockList(new MediumLockList());
    5099         multilock.release();
     5145        alock.release();
     5146        autoCaller.release();
     5147        treelock.release();
    51005148        rc = i_createMediumLockList(true /* fFailIfInaccessible */,
    51015149                                    this /* pToLockWrite */,
    … …  
    51035151                                    NULL,
    51045152                                    *pMediumLockList);
    5105         multilock.acquire();
     5153        treelock.acquire();
     5154        autoCaller.add();
     5155        AssertComRCThrowRC(autoCaller.rc());
     5156        alock.acquire();
    51065157        if (FAILED(rc))
    51075158        {
    … …  
    51105161        }
    51115162
    5112         multilock.release();
     5163        alock.release();
     5164        autoCaller.release();
     5165        treelock.release();
    51135166        rc = pMediumLockList->Lock();
    5114         multilock.acquire();
     5167        treelock.acquire();
     5168        autoCaller.add();
     5169        AssertComRCThrowRC(autoCaller.rc());
     5170        alock.acquire();
    51155171        if (FAILED(rc))
    51165172        {
    … …  
    51295185            throw rc;
    51305186        // no longer need lock
    5131         multilock.release();
     5187        alock.release();
     5188        autoCaller.release();
     5189        treelock.release();
    51325190        i_markRegistriesModified();
    51335191
    … …  
    52865344                                               bool &fMergeForward)
    52875345{
    5288     /** @todo r=klaus The code below needs to be double checked with regard
    5289      * to lock order violations, it probably causes lock order issues related
    5290      * to the AutoCaller usage. Likewise the code using this method seems
    5291      * problematic. */
    52925346    AssertReturn(pOther != NULL, E_FAIL);
    52935347    AssertReturn(pOther != this, E_FAIL);
    52945348
    5295     AutoCaller autoCaller(this);
    5296     AssertComRCReturnRC(autoCaller.rc());
    5297 
    5298     AutoCaller otherCaller(pOther);
    5299     AssertComRCReturnRC(otherCaller.rc());
    5300 
    53015349    HRESULT rc = S_OK;
    53025350    bool fThisParent = false; /**<< Flag whether this medium is the parent of pOther. */
    … …  
    53065354        // locking: we need the tree lock first because we access parent pointers
    53075355        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
     5356
     5357        AutoCaller autoCaller(this);
     5358        AssertComRCThrowRC(autoCaller.rc());
     5359
     5360        AutoCaller otherCaller(pOther);
     5361        AssertComRCThrowRC(otherCaller.rc());
    53085362
    53095363        /* more sanity checking and figuring out the current merge direction */
    … …  
    54155469                                 MediumLockList * &aMediumLockList)
    54165470{
    5417     /** @todo r=klaus The code below needs to be double checked with regard
    5418      * to lock order violations, it probably causes lock order issues related
    5419      * to the AutoCaller usage. Likewise the code using this method seems
    5420      * problematic. */
    54215471    AssertReturn(pTarget != NULL, E_FAIL);
    54225472    AssertReturn(pTarget != this, E_FAIL);
    5423 
    5424     AutoCaller autoCaller(this);
    5425     AssertComRCReturnRC(autoCaller.rc());
    5426 
    5427     AutoCaller targetCaller(pTarget);
    5428     AssertComRCReturnRC(targetCaller.rc());
    54295473
    54305474    HRESULT rc = S_OK;
    … …  
    54415485        AutoWriteLock treeLock(m->pVirtualBox->i_getMediaTreeLockHandle() COMMA_LOCKVAL_SRC_POS);
    54425486
     5487        AutoCaller autoCaller(this);
     5488        AssertComRCThrowRC(autoCaller.rc());
     5489
     5490        AutoCaller targetCaller(pTarget);
     5491        AssertComRCThrowRC(targetCaller.rc());
     5492
    54435493        /* more sanity checking and figuring out the merge direction */
    54445494        ComObjPtr<Medium> pMedium = i_getParent();
    … …  
    54715521        /* Build the lock list. */
    54725522        aMediumLockList = new MediumLockList();
     5523        targetCaller.release();
     5524        autoCaller.release();
    54735525        treeLock.release();
    54745526        if (fMergeForward)
    … …  
    54855537                                        *aMediumLockList);
    54865538        treeLock.acquire();
     5539        autoCaller.add();
     5540        AssertComRCThrowRC(autoCaller.rc());
     5541        targetCaller.add();
     5542        AssertComRCThrowRC(targetCaller.rc());
    54875543        if (FAILED(rc))
    54885544            throw rc;
    … …  
    56275683            if (fLockMedia && aChildrenToReparent)
    56285684            {
     5685                targetCaller.release();
     5686                autoCaller.release();
    56295687                treeLock.release();
    56305688                rc = aChildrenToReparent->Lock();
    56315689                treeLock.acquire();
     5690                autoCaller.add();
     5691                AssertComRCThrowRC(autoCaller.rc());
     5692                targetCaller.add();
     5693                AssertComRCThrowRC(targetCaller.rc());
    56325694                if (FAILED(rc))
    56335695                    throw rc;
    … …  
    56745736        if (fLockMedia)
    56755737        {
     5738            targetCaller.release();
     5739            autoCaller.release();
    56765740            treeLock.release();
    56775741            rc = aMediumLockList->Lock();
    56785742            treeLock.acquire();
     5743            autoCaller.add();
     5744            AssertComRCThrowRC(autoCaller.rc());
     5745            targetCaller.add();
     5746            AssertComRCThrowRC(targetCaller.rc());
    56795747            if (FAILED(rc))
    56805748            {
    … …  
    57955863
    57965864    AutoCaller autoCaller(this);
    5797     if (FAILED(autoCaller.rc())) return autoCaller.rc();
     5865    AssertComRCReturnRC(autoCaller.rc());
    57985866
    57995867    AutoCaller targetCaller(pTarget);
Note: See TracChangeset for help on using the changeset viewer.

© 2024 Oracle Support Privacy / Do Not Sell My Info Terms of Use Trademark Policy Automated Access Etiquette