Version: 1.4-SVN (using KDE KDE 3.5.5KDE 1.2) Installed from: Gentoo PackagesMandriva RPMs Compiler: gcc (GCC) 4.0.1 (4.0.1-5mdk for Mandriva Linux release 2006.0) OS: Linux In the class ManualDeviceAdder in member function newDevice() the code attempts to check if there is a valid medium by calling ManualDeviceAdder::getMedium() in the if statement at line 232. This call to getMedium new's a Medium object off of the heap (look in the same file at line 363). Since the if statement in newDevice() does not assign the returned pointer to anything that memory is lost. The same mistake is perpetuated at the very next line in that function, where getMedium() is called again (when I think they just want the same medium they just asked for). Making both of those calls will leak the memory associated to the Medium objects that were new'ed.
Created attachment 19505 [details] Fixes the medium object memory leak This patch adds a pointer var to assign the provided medium to. Since the medium that is gotten is probably the one that the code wants to use again in the next if, the object pointed to with that pointer var is then used to determine if the medium name is a QString null. After that block is finished the medium object is deleted before the scope of the original conditional is finished.
SVN commit 629248 by mitchell: Fix mem leaks in ManualDeviceAdder and do some cleanup. This code compiles cleanly but I haven't been able to test yet...since 1.4.5 is already tagged though, there's plenty of time... :-) For 1.4. CCBUG: 141036 M +19 -9 mediumpluginmanager.cpp M +2 -3 mediumpluginmanager.h --- branches/stable/extragear/multimedia/amarok/src/mediumpluginmanager.cpp #629247:629248 @@ -229,7 +229,7 @@ { DEBUG_BLOCK ManualDeviceAdder* mda = new ManualDeviceAdder( this ); - if( mda->exec() == QDialog::Accepted && mda->successful() && mda->getMedium() != 0 ) + if( mda->exec() == QDialog::Accepted && mda->successful() ) { if( !Amarok::config( "MediaBrowser" )->readEntry( mda->getMedium()->id() ).isNull() ) { @@ -240,10 +240,9 @@ } else { - Medium *newdev = mda->getMedium(); + Medium *newdev = new Medium( mda->getMedium() ); Amarok::config( "MediaBrowser" )->writeEntry( newdev->id(), mda->getPlugin() ); MediaDeviceManager::instance()->addManualDevice( newdev ); - m_newDevMap[newdev->id()] = newdev; detectDevices(); } } @@ -258,6 +257,7 @@ { m_mpm = mpm; m_successful = false; + m_newMed = 0; kapp->setTopWidget( this ); setCaption( kapp->makeStdCaption( i18n( "Add New Device") ) ); @@ -296,6 +296,7 @@ ManualDeviceAdder::~ManualDeviceAdder() { + delete m_newMed; delete m_mdaName; delete m_mdaMountPoint; } @@ -309,7 +310,7 @@ void ManualDeviceAdder::slotOk() { - if( getMedium() && !getMedium()->name().isEmpty() && + if( getMedium( true ) && !getMedium()->name().isEmpty() && MediaDeviceManager::instance()->getDevice( getMedium()->name() ) == NULL ) { m_successful = true; @@ -348,8 +349,17 @@ } Medium* -ManualDeviceAdder::getMedium() +ManualDeviceAdder::getMedium( bool recreate ) { + if( !recreate ) + return m_newMed; + + if( m_newMed && recreate ) + { + delete m_newMed; + m_newMed = 0; + } + if( m_mdaMountPoint->isEnabled() == false && m_mdaName->text().isNull() ) return NULL; @@ -360,10 +370,10 @@ ( m_mdaMountPoint->text().isNull() || m_mdaMountPoint->isEnabled() == false ? "(null)" : m_mdaMountPoint->text() ); - Medium* added = new Medium( id, m_mdaName->text() ); - added->setAutodetected( false ); - added->setMountPoint( m_mdaMountPoint->text() ); - return added; + m_newMed = new Medium( id, m_mdaName->text() ); + m_newMed->setAutodetected( false ); + m_newMed->setMountPoint( m_mdaMountPoint->text() ); + return m_newMed; } MediaDeviceConfig::MediaDeviceConfig( Medium *medium, MediumPluginManager *mgr, const bool nographics, QWidget *parent, const char *name ) --- branches/stable/extragear/multimedia/amarok/src/mediumpluginmanager.h #629247:629248 @@ -35,7 +35,6 @@ class MediumPluginManager; typedef QMap<QString, Medium*> DeletedMap; -typedef QMap<QString, Medium*> NewDeviceMap; /** @author Jeff Mitchell <kde-dev@emailgoeshere.com> @@ -105,7 +104,6 @@ private: bool detectDevices( bool redetect=false, bool nographics=false ); DeletedMap m_deletedMap; - NewDeviceMap m_newDevMap; DeviceList m_deviceList; QWidget *m_widget; bool m_hasChanged; @@ -138,7 +136,7 @@ ManualDeviceAdder( MediumPluginManager* mdm ); ~ManualDeviceAdder(); bool successful() const { return m_successful; } - Medium* getMedium(); + Medium* getMedium( bool recreate = false ); QString getPlugin() const { return m_selectedPlugin; } private slots: @@ -151,6 +149,7 @@ bool m_successful; QString m_comboOldText; QString m_selectedPlugin; + Medium *m_newMed; KComboBox* m_mdaCombo; HintLineEdit* m_mdaName;
SVN commit 629249 by mitchell: Fix mem leaks in ManualDeviceAdder and do some cleanup. For 2.0 BUG: 141036 M +19 -9 mediumpluginmanager.cpp M +2 -3 mediumpluginmanager.h --- trunk/extragear/multimedia/amarok/src/mediumpluginmanager.cpp #629248:629249 @@ -229,7 +229,7 @@ { DEBUG_BLOCK ManualDeviceAdder* mda = new ManualDeviceAdder( this ); - if( mda->exec() == QDialog::Accepted && mda->successful() && mda->getMedium() != 0 ) + if( mda->exec() == QDialog::Accepted && mda->successful() ) { if( !Amarok::config( "MediaBrowser" )->readEntry( mda->getMedium()->id() ).isNull() ) { @@ -240,10 +240,9 @@ } else { - Medium *newdev = mda->getMedium(); + Medium *newdev = new Medium( mda->getMedium() ); Amarok::config( "MediaBrowser" )->writeEntry( newdev->id(), mda->getPlugin() ); MediaDeviceManager::instance()->addManualDevice( newdev ); - m_newDevMap[newdev->id()] = newdev; detectDevices(); } } @@ -258,6 +257,7 @@ { m_mpm = mpm; m_successful = false; + m_newMed = 0; kapp->setTopWidget( this ); setCaption( KInstance::makeStandardCaption( i18n( "Add New Device") ) ); @@ -297,6 +297,7 @@ ManualDeviceAdder::~ManualDeviceAdder() { + delete m_newMed; delete m_mdaName; delete m_mdaMountPoint; } @@ -310,7 +311,7 @@ void ManualDeviceAdder::slotOk() { - if( getMedium() && !getMedium()->name().isEmpty() && + if( getMedium( true ) && !getMedium()->name().isEmpty() && MediaDeviceManager::instance()->getDevice( getMedium()->name() ) == NULL ) { m_successful = true; @@ -349,8 +350,17 @@ } Medium* -ManualDeviceAdder::getMedium() +ManualDeviceAdder::getMedium( bool recreate ) { + if( !recreate ) + return m_newMed; + + if( m_newMed && recreate ) + { + delete m_newMed; + m_newMed = 0; + } + if( m_mdaMountPoint->isEnabled() == false && m_mdaName->text().isNull() ) return NULL; @@ -361,10 +371,10 @@ ( m_mdaMountPoint->text().isNull() || m_mdaMountPoint->isEnabled() == false ? "(null)" : m_mdaMountPoint->text() ); - Medium* added = new Medium( id, m_mdaName->text() ); - added->setAutodetected( false ); - added->setMountPoint( m_mdaMountPoint->text() ); - return added; + m_newMed = new Medium( id, m_mdaName->text() ); + m_newMed->setAutodetected( false ); + m_newMed->setMountPoint( m_mdaMountPoint->text() ); + return m_newMed; } MediaDeviceConfig::MediaDeviceConfig( Medium *medium, MediumPluginManager *mgr, const bool nographics, QWidget *parent, const char *name ) --- trunk/extragear/multimedia/amarok/src/mediumpluginmanager.h #629248:629249 @@ -38,7 +38,6 @@ class MediumPluginManager; typedef QMap<QString, Medium*> DeletedMap; -typedef QMap<QString, Medium*> NewDeviceMap; /** @author Jeff Mitchell <kde-dev@emailgoeshere.com> @@ -108,7 +107,6 @@ private: bool detectDevices( bool redetect=false, bool nographics=false ); DeletedMap m_deletedMap; - NewDeviceMap m_newDevMap; DeviceList m_deviceList; QWidget *m_widget; bool m_hasChanged; @@ -141,7 +139,7 @@ ManualDeviceAdder( MediumPluginManager* mdm ); ~ManualDeviceAdder(); bool successful() const { return m_successful; } - Medium* getMedium(); + Medium* getMedium( bool recreate = false ); QString getPlugin() const { return m_selectedPlugin; } private slots: @@ -154,6 +152,7 @@ bool m_successful; QString m_comboOldText; QString m_selectedPlugin; + Medium *m_newMed; KComboBox* m_mdaCombo; HintLineEdit* m_mdaName;