Bug 141036 - memory leak of src/mediumpluginmanager.cpp
Summary: memory leak of src/mediumpluginmanager.cpp
Status: RESOLVED FIXED
Alias: None
Product: amarok
Classification: Applications
Component: general (show other bugs)
Version: 1.4-SVN
Platform: Mandriva RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: Amarok Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-02-02 01:05 UTC by Erik Hovland
Modified: 2007-02-02 04:12 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:


Attachments
Fixes the medium object memory leak (1.08 KB, patch)
2007-02-02 01:10 UTC, Erik Hovland
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Hovland 2007-02-02 01:05:47 UTC
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.
Comment 1 Erik Hovland 2007-02-02 01:10:14 UTC
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.
Comment 2 Jeff Mitchell 2007-02-02 04:10:06 UTC
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;
Comment 3 Jeff Mitchell 2007-02-02 04:12:46 UTC
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;