VirtualBox

Changeset 86501 in vbox for trunk


Ignore:
Timestamp:
Oct 9, 2020 12:28:32 PM (4 years ago)
Author:
vboxsync
svn:sync-xref-src-repo-rev:
140816
Message:

VBoxSVC: Fix Medium leak when encoutering duplicate instances while loading a media registry (from settings) via VirtualBox::initMedia(). The media registration is now moved to before any children are loaded, so that we use the right parent for them. This essentially changes the operation to a merge. An unaddressed issue is that on child load failure, we will leave the parent(s) still registered but non-functioning (uninit was done). (The old code would just leave any successfully loaded children registered with non-working parents only accessible via their parent attribute.) bugref:9841

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

Legend:

Unmodified
Added
Removed

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

    r85284 r86501  
    7979                    const settings::Medium &data,
    8080                    const Utf8Str &strMachineFolder);
    81     HRESULT init(VirtualBox *aVirtualBox,
    82                  Medium *aParent,
    83                  DeviceType_T aDeviceType,
    84                  const Guid &uuidMachineRegistry,
    85                  const settings::Medium &data,
    86                  const Utf8Str &strMachineFolder,
    87                  AutoWriteLock &mediaTreeLock);
     81    HRESULT initFromSettings(VirtualBox *aVirtualBox,
     82                             Medium *aParent,
     83                             DeviceType_T aDeviceType,
     84                             const Guid &uuidMachineRegistry,
     85                             const settings::Medium &data,
     86                             const Utf8Str &strMachineFolder,
     87                             AutoWriteLock &mediaTreeLock,
     88                             ComObjPtr<Medium> *ppRegistered);
    8889
    8990    // initializer for host floppy/DVD
    … …  
    119120    /* handles caller/locking itself */
    120121    bool i_addRegistry(const Guid &id);
     122    bool i_addRegistryNoCallerCheck(const Guid &id);
    121123    /* handles caller/locking itself, caller is responsible for tree lock */
    122124    bool i_addRegistryRecursive(const Guid &id);

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

    r86069 r86501  
    277277    void i_copyPathRelativeToConfig(const Utf8Str &strSource, Utf8Str &strTarget);
    278278    HRESULT i_registerMedium(const ComObjPtr<Medium> &pMedium, ComObjPtr<Medium> *ppMedium,
    279                              AutoWriteLock &mediaTreeLock);
     279                             AutoWriteLock &mediaTreeLock, bool fCalledFromMediumInit = false);
    280280    HRESULT i_unregisterMedium(Medium *pMedium);
    281281    void i_pushMediumToListWithChildren(MediaList &llMedia, Medium *pMedium);

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

    r86295 r86501  
    209209    const Guid uuidParentImage;
    210210
     211/** @todo r=bird: the boolean bitfields are pointless if they're not grouped! */
    211212    bool hostDrive : 1;
    212213
    … …  
    13421343
    13431344/**
    1344  * Initializes the medium object and its children by loading its data from the
    1345  * given settings node. The medium will always be opened read/write.
     1345 * Initializes and registers the medium object and its children by loading its
     1346 * data from the given settings node. The medium will always be opened
     1347 * read/write.
     1348 *
     1349 * @todo r=bird: What's that stuff about 'always be opened read/write'?
    13461350 *
    13471351 * In this case, since we're loading from a registry, uuidMachineRegistry is
    13481352 * always set: it's either the global registry UUID or a machine UUID when
    13491353 * loading from a per-machine registry.
     1354 *
     1355 * The only caller is currently VirtualBox::initMedia().
    13501356 *
    13511357 * @param aVirtualBox   VirtualBox object.
    … …  
    13581364 *                      paths; if empty, then we use the VirtualBox home directory
    13591365 * @param mediaTreeLock Autolock.
     1366 * @param ppRegistered  Where to return the registered Medium object.  This is
     1367 *                      for handling the case where the medium object we're
     1368 *                      constructing from settings already has a registered
     1369 *                      instance that should be used instead of this one.
    13601370 *
    13611371 * @note Locks the medium tree for writing.
    13621372 */
    1363 HRESULT Medium::init(VirtualBox *aVirtualBox,
    1364                      Medium *aParent,
    1365                      DeviceType_T aDeviceType,
    1366                      const Guid &uuidMachineRegistry,
    1367                      const settings::Medium &data,
    1368                      const Utf8Str &strMachineFolder,
    1369                      AutoWriteLock &mediaTreeLock)
     1373HRESULT Medium::initFromSettings(VirtualBox *aVirtualBox,
     1374                                 Medium *aParent,
     1375                                 DeviceType_T aDeviceType,
     1376                                 const Guid &uuidMachineRegistry,
     1377                                 const settings::Medium &data,
     1378                                 const Utf8Str &strMachineFolder,
     1379                                 AutoWriteLock &mediaTreeLock,
     1380                                 ComObjPtr<Medium> *ppRegistered)
    13701381{
    13711382    using namespace settings;
    … …  
    13741385    AssertReturn(aVirtualBox, E_INVALIDARG);
    13751386
    1376     /* Enclose the state transition NotReady->InInit->Ready */
    1377     AutoInitSpan autoInitSpan(this);
    1378     AssertReturn(autoInitSpan.isOk(), E_FAIL);
    1379 
    1380     unconst(m->pVirtualBox) = aVirtualBox;
    1381 
    1382     // Do not inline this method call, as the purpose of having this separate
    1383     // is to save on stack size. Less local variables are the key for reaching
    1384     // deep recursion levels with small stack (XPCOM/g++ without optimization).
    1385     HRESULT rc = initOne(aParent, aDeviceType, uuidMachineRegistry, data, strMachineFolder);
    1386 
    1387 
    1388     /* Don't call Medium::i_queryInfo for registered media to prevent the calling
    1389      * thread (i.e. the VirtualBox server startup thread) from an unexpected
    1390      * freeze but mark it as initially inaccessible instead. The vital UUID,
    1391      * location and format properties are read from the registry file above; to
    1392      * get the actual state and the rest of the data, the user will have to call
    1393      * COMGETTER(State). */
    1394 
    1395     /* load all children */
    1396     for (settings::MediaList::const_iterator it = data.llChildren.begin();
    1397          it != data.llChildren.end();
    1398          ++it)
    1399     {
    1400         const settings::Medium &med = *it;
    1401 
    1402         ComObjPtr<Medium> pMedium;
    1403         pMedium.createObject();
    1404         rc = pMedium->init(aVirtualBox,
    1405                            this,            // parent
    1406                            aDeviceType,
    1407                            uuidMachineRegistry,
    1408                            med,               // child data
    1409                            strMachineFolder,
    1410                            mediaTreeLock);
    1411         if (FAILED(rc)) break;
    1412 
    1413         rc = m->pVirtualBox->i_registerMedium(pMedium, &pMedium, mediaTreeLock);
    1414         if (FAILED(rc)) break;
    1415     }
    1416 
    1417     /* Confirm a successful initialization when it's the case */
    1418     if (SUCCEEDED(rc))
    1419         autoInitSpan.setSucceeded();
     1387    /*
     1388     * Enclose the state transition NotReady->InInit->Ready.
     1389     *
     1390     * Note! This has to be scoped so that we can temporarily drop the
     1391     *       mediaTreeLock ownership on failure.
     1392     */
     1393    HRESULT rc;
     1394    bool    fRetakeLock = false;
     1395    {
     1396        AutoInitSpan autoInitSpan(this);
     1397        AssertReturn(autoInitSpan.isOk(), E_FAIL);
     1398
     1399        unconst(m->pVirtualBox) = aVirtualBox;
     1400        ComObjPtr<Medium> pActualThis = this;
     1401
     1402        // Do not inline this method call, as the purpose of having this separate
     1403        // is to save on stack size. Less local variables are the key for reaching
     1404        // deep recursion levels with small stack (XPCOM/g++ without optimization).
     1405        rc = initOne(aParent, aDeviceType, uuidMachineRegistry, data, strMachineFolder);
     1406        if (SUCCEEDED(rc))
     1407        {
     1408            /* In order to avoid duplicate instances of the medium object, we need
     1409               to register the parent before loading any children.  */
     1410            rc = m->pVirtualBox->i_registerMedium(pActualThis, &pActualThis, mediaTreeLock, true /*fCalledFromMediumInit*/);
     1411            if (SUCCEEDED(rc))
     1412            {
     1413                /* Don't call Medium::i_queryInfo for registered media to prevent the calling
     1414                 * thread (i.e. the VirtualBox server startup thread) from an unexpected
     1415                 * freeze but mark it as initially inaccessible instead. The vital UUID,
     1416                 * location and format properties are read from the registry file above; to
     1417                 * get the actual state and the rest of the data, the user will have to call
     1418                 * COMGETTER(State). */
     1419
     1420                /* load all children */
     1421                for (settings::MediaList::const_iterator it = data.llChildren.begin();
     1422                     it != data.llChildren.end();
     1423                     ++it)
     1424                {
     1425                    const settings::Medium &med = *it;
     1426
     1427                    ComObjPtr<Medium> pMedium;
     1428                    rc = pMedium.createObject();
     1429                    if (FAILED(rc)) break;
     1430
     1431                    rc = pMedium->initFromSettings(aVirtualBox,
     1432                                                   pActualThis,       // parent
     1433                                                   aDeviceType,
     1434                                                   uuidMachineRegistry,
     1435                                                   med,               // child data
     1436                                                   strMachineFolder,
     1437                                                   mediaTreeLock,
     1438                                                   NULL /*ppRegistered - must _not_ be &pMedium*/);
     1439                    if (FAILED(rc)) break;
     1440                }
     1441            }
     1442        }
     1443
     1444        /* Confirm a successful initialization when it's the case.  Do not confirm duplicates. */
     1445        if (SUCCEEDED(rc) && (Medium *)pActualThis == this)
     1446            autoInitSpan.setSucceeded();
     1447        /* Otherwise we have release the media tree lock so that Medium::uninit()
     1448           doesn't freak out when it is called by AutoInitSpan::~AutoInitSpan().
     1449           In the case of duplicates, the uninit will i_deparent this object. */
     1450        else
     1451        {
     1452            /** @todo Non-duplicate: Deregister? What about child load errors, should they
     1453             *        affect the parents? */
     1454            mediaTreeLock.release();
     1455            fRetakeLock = true;
     1456        }
     1457
     1458        /* Must be done after the mediaTreeLock.release above. */
     1459        if (ppRegistered)
     1460            *ppRegistered = pActualThis;
     1461    }
     1462
     1463    if (fRetakeLock)
     1464        mediaTreeLock.acquire();
    14201465
    14211466    return rc;
    … …  
    40594104
    40604105/**
     4106 * Same as i_addRegistry() except that we don't check the object state, making
     4107 * it safe to call with initFromSettings() on the call stack.
     4108 */
     4109bool Medium::i_addRegistryNoCallerCheck(const Guid &id)
     4110{
     4111    AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
     4112
     4113    bool fAdd = true;
     4114
     4115    // hard disks cannot be in more than one registry
     4116    if (   m->devType == DeviceType_HardDisk
     4117        && m->llRegistryIDs.size() > 0)
     4118        fAdd = false;
     4119
     4120    // no need to add the UUID twice
     4121    if (fAdd)
     4122    {
     4123        for (GuidList::const_iterator it = m->llRegistryIDs.begin();
     4124             it != m->llRegistryIDs.end();
     4125             ++it)
     4126        {
     4127            if ((*it) == id)
     4128            {
     4129                fAdd = false;
     4130                break;
     4131            }
     4132        }
     4133    }
     4134
     4135    if (fAdd)
     4136        m->llRegistryIDs.push_back(id);
     4137
     4138    return fAdd;
     4139}
     4140
     4141/**
    40614142 * This adds the given UUID to the list of media registries in which this
    40624143 * medium should be registered. The UUID can either be a machine UUID,
    … …  
    40724153 * @return true if the registry was added; false if the given id was already on the list.
    40734154 */
    4074 bool Medium::i_addRegistry(const Guid& id)
     4155bool Medium::i_addRegistry(const Guid &id)
    40754156{
    40764157    AutoCaller autoCaller(this);
    40774158    if (FAILED(autoCaller.rc()))
    40784159        return false;
    4079     AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS);
    4080 
    4081     bool fAdd = true;
    4082 
    4083     // hard disks cannot be in more than one registry
    4084     if (   m->devType == DeviceType_HardDisk
    4085         && m->llRegistryIDs.size() > 0)
    4086         fAdd = false;
    4087 
    4088     // no need to add the UUID twice
    4089     if (fAdd)
    4090     {
    4091         for (GuidList::const_iterator it = m->llRegistryIDs.begin();
    4092              it != m->llRegistryIDs.end();
    4093              ++it)
    4094         {
    4095             if ((*it) == id)
    4096             {
    4097                 fAdd = false;
    4098                 break;
    4099             }
    4100         }
    4101     }
    4102 
    4103     if (fAdd)
    4104         m->llRegistryIDs.push_back(id);
    4105 
    4106     return fAdd;
     4160    return i_addRegistryNoCallerCheck(id);
    41074161}
    41084162
    … …  
    41294183        return false;
    41304184
    4131     bool fAdd = i_addRegistry(id);
     4185    bool fAdd = i_addRegistryNoCallerCheck(id);
    41324186
    41334187    // protected by the medium tree lock held by our original caller

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

    r86069 r86501  
    921921
    922922        ComObjPtr<Medium> pHardDisk;
    923         if (SUCCEEDED(rc = pHardDisk.createObject()))
    924             rc = pHardDisk->init(this,
    925                                  NULL,          // parent
    926                                  DeviceType_HardDisk,
    927                                  uuidRegistry,
    928                                  xmlHD,         // XML data; this recurses to processes the children
    929                                  strMachineFolder,
    930                                  treeLock);
     923        rc = pHardDisk.createObject();
    931924        if (FAILED(rc)) return rc;
    932 
    933         rc = i_registerMedium(pHardDisk, &pHardDisk, treeLock);
     925        ComObjPtr<Medium> pHardDiskActual(pHardDisk);
     926        rc = pHardDisk->initFromSettings(this,
     927                                         NULL,          // parent
     928                                         DeviceType_HardDisk,
     929                                         uuidRegistry,
     930                                         xmlHD,         // XML data; this recurses to processes the children
     931                                         strMachineFolder,
     932                                         treeLock,
     933                                         &pHardDiskActual /*never &pHardDisk!*/);
    934934        if (SUCCEEDED(rc))
    935935        {
    936             uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pHardDisk->i_getId(), DeviceType_HardDisk));
     936            /** @todo r=bird: should we really do notifications for duplicates?
     937             *  ((Medium *)pHardDisk != (Medium *)pHardDiskActual)
     938             * The problem with that, though, is that for the children we don't quite know
     939             * which are duplicates and which aren't.   The above initFromSettings is
     940             * essentially a merge operation now, so in the duplicate case, we may just
     941             * have added a new (grand)child. */
     942
     943            uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pHardDiskActual->i_getId(), DeviceType_HardDisk));
    937944            // Add children IDs to notification using non-recursive children enumeration.
    938945            std::vector<std::pair<MediaList::const_iterator, ComObjPtr<Medium> > > llEnumStack;
    939             const MediaList& mediaList = pHardDisk->i_getChildren();
    940             llEnumStack.push_back(std::pair<MediaList::const_iterator, ComObjPtr<Medium> >(mediaList.begin(), pHardDisk));
     946            const MediaList& mediaList = pHardDiskActual->i_getChildren();
     947            llEnumStack.push_back(std::pair<MediaList::const_iterator, ComObjPtr<Medium> >(mediaList.begin(), pHardDiskActual));
    941948            while (!llEnumStack.empty())
    942949            {
    … …  
    962969        treeLock.release();
    963970        pHardDisk.setNull();
     971        pHardDiskActual.setNull();
     972        if (FAILED(rc)) return rc;
    964973        treeLock.acquire();
    965         if (FAILED(rc)) return rc;
    966974    }
    967975
    … …  
    973981
    974982        ComObjPtr<Medium> pImage;
    975         if (SUCCEEDED(pImage.createObject()))
    976             rc = pImage->init(this,
    977                               NULL,
    978                               DeviceType_DVD,
    979                               uuidRegistry,
    980                               xmlDvd,
    981                               strMachineFolder,
    982                               treeLock);
     983        rc = pImage.createObject();
    983984        if (FAILED(rc)) return rc;
    984985
    985         rc = i_registerMedium(pImage, &pImage, treeLock);
     986        ComObjPtr<Medium> pImageActually = pImage;
     987        rc = pImage->initFromSettings(this,
     988                                      NULL,
     989                                      DeviceType_DVD,
     990                                      uuidRegistry,
     991                                      xmlDvd,
     992                                      strMachineFolder,
     993                                      treeLock,
     994                                      &pImageActually);
    986995        if (SUCCEEDED(rc))
    987             uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pImage->i_getId(), DeviceType_DVD));
     996            uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pImageActually->i_getId(), DeviceType_DVD));
     997
    988998        // Avoid trouble with lock/refcount, before returning or not.
    989999        treeLock.release();
    9901000        pImage.setNull();
     1001        pImageActually.setNull();
     1002        if (FAILED(rc)) return rc;
    9911003        treeLock.acquire();
    992         if (FAILED(rc)) return rc;
    9931004    }
    9941005
    … …  
    10001011
    10011012        ComObjPtr<Medium> pImage;
    1002         if (SUCCEEDED(pImage.createObject()))
    1003             rc = pImage->init(this,
    1004                               NULL,
    1005                               DeviceType_Floppy,
    1006                               uuidRegistry,
    1007                               xmlFloppy,
    1008                               strMachineFolder,
    1009                               treeLock);
     1013        rc = pImage.createObject();
    10101014        if (FAILED(rc)) return rc;
    10111015
    1012         rc = i_registerMedium(pImage, &pImage, treeLock);
     1016        ComObjPtr<Medium> pImageActually = pImage;
     1017        rc = pImage->initFromSettings(this,
     1018                                      NULL,
     1019                                      DeviceType_Floppy,
     1020                                      uuidRegistry,
     1021                                      xmlFloppy,
     1022                                      strMachineFolder,
     1023                                      treeLock,
     1024                                      &pImageActually);
    10131025        if (SUCCEEDED(rc))
    10141026            uIdsForNotify.push_back(std::pair<Guid, DeviceType_T>(pImage->i_getId(), DeviceType_Floppy));
     1027
    10151028        // Avoid trouble with lock/refcount, before returning or not.
    10161029        treeLock.release();
    10171030        pImage.setNull();
     1031        pImageActually.setNull();
     1032        if (FAILED(rc)) return rc;
    10181033        treeLock.acquire();
    1019         if (FAILED(rc)) return rc;
    10201034    }
    10211035
    … …  
    51095123HRESULT VirtualBox::i_registerMedium(const ComObjPtr<Medium> &pMedium,
    51105124                                     ComObjPtr<Medium> *ppMedium,
    5111                                      AutoWriteLock &mediaTreeLock)
     5125                                     AutoWriteLock &mediaTreeLock,
     5126                                     bool fCalledFromMediumInit)
    51125127{
    51135128    AssertReturn(pMedium != NULL, E_INVALIDARG);
    … …  
    52135228    {
    52145229        AutoWriteLock mediumLock(pMedium COMMA_LOCKVAL_SRC_POS);
    5215         if (pMedium->i_addRegistry(m->uuidMediaRegistry))
     5230        if (  fCalledFromMediumInit
     5231            ? (*ppMedium)->i_addRegistryNoCallerCheck(m->uuidMediaRegistry)
     5232            : (*ppMedium)->i_addRegistry(m->uuidMediaRegistry))
    52165233            i_markRegistryModified(m->uuidMediaRegistry);
    52175234    }
Note: See TracChangeset for help on using the changeset viewer.

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