← Back to team overview

kicad-developers team mailing list archive

[PATCH]v3 Fix memory leaks with improper wxBaseConfig* usage (model ownership and ownership-transfer with std::unique_ptr)

 

Finally I catched up. Attached v3 of the patch, the auiManager part is removed. Sorry for misinterpreting the comments.

Michael
>From 46886bcccbbd4d99faaec9f374a1a5d1d5413c95 Mon Sep 17 00:00:00 2001
From: decimad <michsteinb@xxxxxxxxx>
Date: Tue, 5 Jul 2016 02:31:04 +0200
Subject: [PATCH] Fix memory leaks with improper wxBaseConfig* usage (model
 ownership and ownership-transfer with std::unique_ptr)

---
 bitmap2component/bitmap2cmp_gui.cpp     |  9 ++++++---
 common/bin_mod.cpp                      | 12 ++++--------
 common/common.cpp                       |  9 +++++----
 common/hotkeys_basic.cpp                |  6 ++----
 common/pgm_base.cpp                     | 11 +++++++----
 include/bin_mod.h                       | 13 +++++++------
 include/common.h                        |  7 ++++---
 include/kiface_i.h                      |  4 ++--
 include/pgm_base.h                      | 11 +++++++----
 include/wxstruct.h                      |  4 ++--
 kicad/pgm_kicad.h                       |  4 ++--
 pcb_calculator/pcb_calculator_frame.cpp |  8 ++++----
 12 files changed, 52 insertions(+), 46 deletions(-)

diff --git a/bitmap2component/bitmap2cmp_gui.cpp b/bitmap2component/bitmap2cmp_gui.cpp
index 99ad647..1eb3f53 100644
--- a/bitmap2component/bitmap2cmp_gui.cpp
+++ b/bitmap2component/bitmap2cmp_gui.cpp
@@ -2,7 +2,7 @@
  * This program source code file is part of KICAD, a free EDA CAD application.
  *
  * Copyright (C) 1992-2010 jean-pierre.charras
- * Copyright (C) 1992-2015 Kicad Developers, see change_log.txt for contributors.
+ * Copyright (C) 1992-2016 Kicad Developers, see authors.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -80,7 +80,8 @@ private:
     wxString        m_ConvertedFileName;
     wxSize          m_frameSize;
     wxPoint         m_framePos;
-    wxConfigBase*   m_config;
+
+    std::unique_ptr< wxConfigBase > m_config;
 
 public:
     BM2CMP_FRAME( KIWAY* aKiway, wxWindow* aParent );
@@ -206,6 +207,7 @@ BM2CMP_FRAME::BM2CMP_FRAME( KIWAY* aKiway, wxWindow* aParent ) :
 
 BM2CMP_FRAME::~BM2CMP_FRAME()
 {
+    /// @todo Should the config really not be written if BM2CMP is iconized?
     if( !m_config || IsIconized() )
         return;
 
@@ -223,7 +225,8 @@ BM2CMP_FRAME::~BM2CMP_FRAME()
     m_config->Write( KEYWORD_LAST_FORMAT,  m_radioBoxFormat->GetSelection() );
     m_config->Write( KEYWORD_LAST_MODLAYER,  m_radio_PCBLayer->GetSelection() );
 
-    delete m_config;
+    m_config->Flush();
+    m_config.reset();
 
     /* This needed for OSX: avoids further OnDraw processing after this
      * destructor and before the native window is destroyed
diff --git a/common/bin_mod.cpp b/common/bin_mod.cpp
index 116bc20..55edb35 100644
--- a/common/bin_mod.cpp
+++ b/common/bin_mod.cpp
@@ -2,7 +2,7 @@
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
  * Copyright (C) 2014 CERN
- * Copyright (C) 2014 KiCad Developers, see CHANGELOG.TXT for contributors.
+ * Copyright (C) 2014-2016 KiCad Developers, see CHANGELOG.TXT for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -27,8 +27,7 @@
 
 
 BIN_MOD::BIN_MOD( const char* aName ) :
-    m_name( aName ),
-    m_config( 0 )
+    m_name( aName )
 {
 }
 
@@ -55,11 +54,8 @@ void BIN_MOD::End()
     if( m_config )
     {
         m_history.Save( *m_config );
-
-        // Deleting a wxConfigBase writes its contents to disk if changed.
-        // Might be NULL if called twice, in which case nothing happens.
-        delete m_config;
-        m_config = 0;
+        m_config->Flush();
+        m_config.reset();
     }
 }
 
diff --git a/common/common.cpp b/common/common.cpp
index 976e419..5f6cac0 100644
--- a/common/common.cpp
+++ b/common/common.cpp
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2014-2015 Jean-Pierre Charras, jp.charras at wanadoo.fr
  * Copyright (C) 2008-2015 Wayne Stambaugh <stambaughw@xxxxxxxxxxx>
- * Copyright (C) 1992-2015 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2016 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -300,15 +300,16 @@ double RoundTo0( double x, double precision )
 }
 
 
-wxConfigBase* GetNewConfig( const wxString& aProgName )
+std::unique_ptr< wxConfigBase > GetNewConfig( const wxString& aProgName )
 {
     wxConfigBase* cfg = 0;
     wxFileName configname;
     configname.AssignDir( GetKicadConfigPath() );
     configname.SetFullName( aProgName );
 
-    cfg = new wxFileConfig( wxT( "" ), wxT( "" ), configname.GetFullPath() );
-    return cfg;
+    return std::unique_ptr< wxConfigBase >(
+        new wxFileConfig( wxT(""), wxT(""), configname.GetFullPath() )
+    );
 }
 
 wxString GetKicadLockFilePath()
diff --git a/common/hotkeys_basic.cpp b/common/hotkeys_basic.cpp
index c2f775e..74f0b91 100644
--- a/common/hotkeys_basic.cpp
+++ b/common/hotkeys_basic.cpp
@@ -589,9 +589,8 @@ int EDA_BASE_FRAME::WriteHotkeyConfig( struct EDA_HOTKEY_CONFIG* aDescList,
     {
         wxFileName fn( GetName() );
         fn.SetExt( DEFAULT_HOTKEY_FILENAME_EXT );
-        wxConfigBase* config = GetNewConfig( fn.GetFullPath() );
+        std::unique_ptr< wxConfigBase > config = GetNewConfig( fn.GetFullPath() );
         config->Write( HOTKEYS_CONFIG_KEY, msg );
-        delete config;
     }
 
     return 1;
@@ -635,7 +634,7 @@ void ReadHotkeyConfig( const wxString& Appname, struct EDA_HOTKEY_CONFIG* aDescL
     wxFileName fn( Appname );
     fn.SetExt( DEFAULT_HOTKEY_FILENAME_EXT );
 
-    wxConfigBase* config = GetNewConfig( fn.GetFullPath() );
+    std::unique_ptr< wxConfigBase > config = GetNewConfig( fn.GetFullPath() );
 
     if( !config->HasEntry( HOTKEYS_CONFIG_KEY ) )
     {
@@ -645,7 +644,6 @@ void ReadHotkeyConfig( const wxString& Appname, struct EDA_HOTKEY_CONFIG* aDescL
 
     wxString data;
     config->Read( HOTKEYS_CONFIG_KEY, &data );
-    delete config;
 
     ParseHotkeyConfig( data, aDescList );
 }
diff --git a/common/pgm_base.cpp b/common/pgm_base.cpp
index 9368534..0644513 100644
--- a/common/pgm_base.cpp
+++ b/common/pgm_base.cpp
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2004-2015 Jean-Pierre Charras, jaen-pierre.charras@xxxxxxxxxxxxxxxxxx
  * Copyright (C) 2008-2015 Wayne Stambaugh <stambaughw@xxxxxxxxxxx>
- * Copyright (C) 1992-2015 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2016 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -283,7 +283,6 @@ PGM_BASE::PGM_BASE()
 {
     m_pgm_checker = NULL;
     m_locale = NULL;
-    m_common_settings = NULL;
 
     m_wx_app = NULL;
     m_show_env_var_dialog = true;
@@ -304,9 +303,13 @@ void PGM_BASE::destroy()
 {
     // unlike a normal destructor, this is designed to be called more than once safely:
 
-    delete m_common_settings;
-    m_common_settings = 0;
+    if( m_common_settings )
+    {
+        m_common_settings->Flush();
+        m_common_settings.reset();
+    }
 
+    /// @todo Use smart pointers for these too.
     delete m_pgm_checker;
     m_pgm_checker = 0;
 
diff --git a/include/bin_mod.h b/include/bin_mod.h
index cbf360d..dd9f706 100644
--- a/include/bin_mod.h
+++ b/include/bin_mod.h
@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 2014 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 2014-2016 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -34,6 +34,7 @@
 
 #include <search_stack.h>
 
+#include <memory>
 class wxConfigBase;
 
 /**
@@ -52,13 +53,13 @@ struct BIN_MOD
     void Init();
     void End();
 
-    const char*         m_name;             ///< name of this binary module, static C string.
+    const char*                     m_name;   ///< name of this binary module, static C string.
+    std::unique_ptr< wxConfigBase > m_config; ///< maybe from $HOME/.${m_name}
 
-    wxConfigBase*       m_config;           ///< maybe from $HOME/.${m_name}
-    wxFileHistory       m_history;
-    wxString            m_help_file;
+    wxFileHistory   m_history;
+    wxString        m_help_file;
 
-    SEARCH_STACK        m_search;
+    SEARCH_STACK    m_search;
 
 
 };
diff --git a/include/common.h b/include/common.h
index 70e7379..a80c677 100644
--- a/include/common.h
+++ b/include/common.h
@@ -43,6 +43,7 @@
 #include <colors.h>
 
 #include <atomic>
+#include <memory>
 
 
 class wxAboutDialogInfo;
@@ -379,10 +380,10 @@ const wxString PrePendPath( const wxString& aEnvVar, const wxString& aPriorityPa
  *
  * @param aProgName is the name of the program calling this function - can be obtained by
  *  calling Pgm().App().GetAppName().  This will be the actual file name of the config file.
- * @return A pointer to a new wxConfigBase derived object is returned.  The caller is in charge
- *  of deleting it.
+ * @return A std::unique_pointer to a new wxConfigBase derived object is returned. Caller
+ *  assumes ownership.
  */
-wxConfigBase* GetNewConfig( const wxString& aProgName );
+std::unique_ptr< wxConfigBase > GetNewConfig( const wxString& aProgName );
 
 /**
  * Function GetKicadLockFilePath
diff --git a/include/kiface_i.h b/include/kiface_i.h
index 5cb41b8..153df13 100644
--- a/include/kiface_i.h
+++ b/include/kiface_i.h
@@ -1,7 +1,7 @@
 /*
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
- * Copyright (C) 1992-2014 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2016 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -98,7 +98,7 @@ public:
 
     const wxString Name()                               { return wxString::FromUTF8( m_bm.m_name ); }
 
-    wxConfigBase* KifaceSettings() const                { return m_bm.m_config; }
+    wxConfigBase* KifaceSettings() const                { return m_bm.m_config.get(); }
 
     /**
      * Function StartFlags
diff --git a/include/pgm_base.h b/include/pgm_base.h
index fae237a..32dd306 100644
--- a/include/pgm_base.h
+++ b/include/pgm_base.h
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2004-2015 Jean-Pierre Charras, jaen-pierre.charras@xxxxxxxxxxxxxxxxxx
  * Copyright (C) 2008-2015 Wayne Stambaugh <stambaughw@xxxxxxxxxxx>
- * Copyright (C) 1992-2015 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2016 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -31,12 +31,12 @@
 #ifndef  PGM_BASE_H_
 #define  PGM_BASE_H_
 
+#include <memory>
 #include <map>
 #include <wx/filename.h>
 #include <search_stack.h>
 #include <wx/gdicmn.h>
 
-
 class wxConfigBase;
 class wxSingleInstanceChecker;
 class wxApp;
@@ -106,6 +106,9 @@ class PGM_BASE
 {
 public:
     PGM_BASE();
+
+	// @todo: Add virtual or comment that it will never be destroyed through a base
+	//  class reference.
     ~PGM_BASE();
 
     /**
@@ -127,7 +130,7 @@ public:
 
     //----<Cross Module API>-----------------------------------------------------
 
-    VTBL_ENTRY wxConfigBase* CommonSettings() const                 { return m_common_settings; }
+    VTBL_ENTRY wxConfigBase* CommonSettings() const                 { return m_common_settings.get(); }
 
     VTBL_ENTRY void SetEditorName( const wxString& aFileName );
 
@@ -317,7 +320,7 @@ protected:
 
     /// Configuration settings common to all KiCad program modules,
     /// like as in $HOME/.kicad_common
-    wxConfigBase*   m_common_settings;
+    std::unique_ptr< wxConfigBase > m_common_settings;
 
     /// full path to this program
     wxString        m_bin_dir;
diff --git a/include/wxstruct.h b/include/wxstruct.h
index a6eb79c..8ae9946 100644
--- a/include/wxstruct.h
+++ b/include/wxstruct.h
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2009-2015 Jean-Pierre Charras, jp.charras wanadoo.fr
  * Copyright (C) 2011-2015 Wayne Stambaugh <stambaughw@xxxxxxxxxxx>
- * Copyright (C) 1992-2015 KiCad Developers, see AUTHORS.txt for contributors.
+ * Copyright (C) 1992-2016 KiCad Developers, see AUTHORS.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -196,7 +196,7 @@ public:
         const wxString& aTitle, const wxPoint& aPos, const wxSize& aSize,
         long aStyle, const wxString& aFrameName );
 
-    ~EDA_BASE_FRAME();
+    virtual ~EDA_BASE_FRAME();
 
     /**
      * Function ProcessEvent
diff --git a/kicad/pgm_kicad.h b/kicad/pgm_kicad.h
index 61990d9..4ca9a03 100644
--- a/kicad/pgm_kicad.h
+++ b/kicad/pgm_kicad.h
@@ -2,7 +2,7 @@
  * This program source code file is part of KiCad, a free EDA CAD application.
  *
  * Copyright (C) 2014 SoftPLC Corporation, Dick Hollenbeck <dick@xxxxxxxxxxx>
- * Copyright (C) 2014 KiCad Developers, see CHANGELOG.TXT for contributors.
+ * Copyright (C) 2014-2016 KiCad Developers, see CHANGELOG.TXT for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -55,7 +55,7 @@ public:
 
     wxFileHistory&  GetFileHistory()            { return m_bm.m_history; }
 
-    wxConfigBase*   PgmSettings()               { return m_bm.m_config; }
+    wxConfigBase*   PgmSettings()               { return m_bm.m_config.get(); }
 
     SEARCH_STACK&   SysSearch()                 { return m_bm.m_search; }
 
diff --git a/pcb_calculator/pcb_calculator_frame.cpp b/pcb_calculator/pcb_calculator_frame.cpp
index 476dee9..dd96e64 100644
--- a/pcb_calculator/pcb_calculator_frame.cpp
+++ b/pcb_calculator/pcb_calculator_frame.cpp
@@ -2,7 +2,7 @@
  * This program source code file is part of KICAD, a free EDA CAD application.
  *
  * Copyright (C) 1992-2015 jean-pierre.charras
- * Copyright (C) 1992-2015 Kicad Developers, see change_log.txt for contributors.
+ * Copyright (C) 1992-2016 Kicad Developers, see change_log.txt for contributors.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -83,15 +83,15 @@ PCB_CALCULATOR_FRAME::PCB_CALCULATOR_FRAME( KIWAY* aKiway, wxWindow* aParent ) :
     m_attenuator_list.push_back( new ATTENUATOR_SPLITTER() );
     m_currAttenuator = m_attenuator_list[0];
 
-    wxConfigBase* config = GetNewConfig( Pgm().App().GetAppName() );
-    LoadSettings( config );
+    std::unique_ptr< wxConfigBase > config = GetNewConfig( Pgm().App().GetAppName() );
+    LoadSettings( config.get() );
 
     ReadDataFile();
 
     TranslineTypeSelection( m_currTransLineType );
     m_TranslineSelection->SetSelection( m_currTransLineType );
 
-    TW_Init( config );
+    TW_Init( config.get() );
 
     SetAttenuator( m_AttenuatorsSelection->GetSelection() );
 
-- 
2.9.0.windows.1