From b03366c9e8801bbb394295c2bd9b0bac324cef38 Mon Sep 17 00:00:00 2001 From: Roberto Fernandez Bautista Date: Mon, 27 Feb 2023 23:55:20 +0100 Subject: [PATCH] Fix CADSTAR importer memory leaks We were leaking the xml tree when throwing exceptions --- .../cadstar/cadstar_archive_parser.cpp | 27 ++++++++++++++++--- .../plugins/cadstar/cadstar_archive_parser.h | 2 ++ .../cadstar/cadstar_sch_archive_parser.cpp | 9 ++++--- .../cadstar/cadstar_sch_archive_parser.h | 14 ++++++++-- .../cadstar/cadstar_pcb_archive_parser.cpp | 8 +++--- .../cadstar/cadstar_pcb_archive_parser.h | 15 +++++++++-- 6 files changed, 60 insertions(+), 15 deletions(-) diff --git a/common/plugins/cadstar/cadstar_archive_parser.cpp b/common/plugins/cadstar/cadstar_archive_parser.cpp index 2f6439503c..32012423da 100644 --- a/common/plugins/cadstar/cadstar_archive_parser.cpp +++ b/common/plugins/cadstar/cadstar_archive_parser.cpp @@ -2428,7 +2428,9 @@ XNODE* CADSTAR_ARCHIVE_PARSER::LoadArchiveFile( const wxString& aFileName, const wxString& aFileTypeIdentifier, PROGRESS_REPORTER* aProgressReporter ) { KEYWORD emptyKeywords[1] = {}; - XNODE * iNode = nullptr, *cNode = nullptr; + XNODE* rootNode = nullptr; + XNODE* cNode = nullptr; + XNODE* iNode = nullptr; int tok; bool cadstarFileCheckDone = false; wxString str; @@ -2460,7 +2462,10 @@ XNODE* CADSTAR_ARCHIVE_PARSER::LoadArchiveFile( const wxString& aFileName, if( aProgressReporter && ( currentProgress() - previousReportedProgress ) > 0.01 ) { if( !aProgressReporter->KeepRefreshing() ) + { + delete rootNode; THROW_IO_ERROR( _( "File import cancelled by user." ) ); + } aProgressReporter->SetCurrentProgress( currentProgress() ); previousReportedProgress = currentProgress(); @@ -2476,6 +2481,7 @@ XNODE* CADSTAR_ARCHIVE_PARSER::LoadArchiveFile( const wxString& aFileName, else { //too many closing brackets + delete rootNode; THROW_IO_ERROR( _( "The selected file is not valid or might be corrupt!" ) ); } } @@ -2485,6 +2491,9 @@ XNODE* CADSTAR_ARCHIVE_PARSER::LoadArchiveFile( const wxString& aFileName, str = wxString( lexer.CurText(), *conv ); cNode = new XNODE( wxXML_ELEMENT_NODE, str ); + if( !rootNode ) + rootNode = cNode; + if( iNode ) { //we will add it as attribute as well as child node @@ -2494,7 +2503,10 @@ XNODE* CADSTAR_ARCHIVE_PARSER::LoadArchiveFile( const wxString& aFileName, else if( !cadstarFileCheckDone ) { if( cNode->GetName() != aFileTypeIdentifier ) + { + delete rootNode; THROW_IO_ERROR( _( "The selected file is not valid or might be corrupt!" ) ); + } cadstarFileCheckDone = true; } @@ -2510,19 +2522,28 @@ XNODE* CADSTAR_ARCHIVE_PARSER::LoadArchiveFile( const wxString& aFileName, else { //not enough closing brackets + delete rootNode; THROW_IO_ERROR( _( "The selected file is not valid or might be corrupt!" ) ); } } // Not enough closing brackets if( iNode != nullptr ) + { + delete rootNode; THROW_IO_ERROR( _( "The selected file is not valid or might be corrupt!" ) ); + } // Throw if no data was parsed - if( cNode ) - return cNode; + if( rootNode ) + { + return rootNode; + } else + { + delete rootNode; THROW_IO_ERROR( _( "The selected file is not valid or might be corrupt!" ) ); + } return nullptr; } diff --git a/common/plugins/cadstar/cadstar_archive_parser.h b/common/plugins/cadstar/cadstar_archive_parser.h index 040f4780fb..efb17f6156 100644 --- a/common/plugins/cadstar/cadstar_archive_parser.h +++ b/common/plugins/cadstar/cadstar_archive_parser.h @@ -93,6 +93,8 @@ class CADSTAR_ARCHIVE_PARSER public: CADSTAR_ARCHIVE_PARSER() { m_progressReporter = nullptr; } + virtual ~CADSTAR_ARCHIVE_PARSER() {} + typedef wxString LINECODE_ID; typedef wxString HATCHCODE_ID; diff --git a/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.cpp b/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.cpp index cc8f5a05bb..524363cbb9 100644 --- a/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.cpp +++ b/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.cpp @@ -35,7 +35,7 @@ void CADSTAR_SCH_ARCHIVE_PARSER::Parse() if( m_progressReporter ) m_progressReporter->BeginPhase( 0 ); // Read file - XNODE* fileRootNode = LoadArchiveFile( Filename, wxT( "CADSTARSCM" ), m_progressReporter ); + m_rootNode = LoadArchiveFile( Filename, wxT( "CADSTARSCM" ), m_progressReporter ); if( m_progressReporter ) { @@ -44,13 +44,13 @@ void CADSTAR_SCH_ARCHIVE_PARSER::Parse() std::vector subNodeChildrenToCount = { wxT( "LIBRARY" ), wxT( "PARTS" ), wxT( "SCHEMATIC" ) }; - long numOfSteps = GetNumberOfStepsForReporting( fileRootNode, subNodeChildrenToCount ); + long numOfSteps = GetNumberOfStepsForReporting( m_rootNode, subNodeChildrenToCount ); m_progressReporter->SetMaxProgress( numOfSteps ); } m_context.CheckPointCallback = [&](){ checkPoint(); }; - XNODE* cNode = fileRootNode->GetChildren(); + XNODE* cNode = m_rootNode->GetChildren(); if( !cNode ) THROW_MISSING_NODE_IO_ERROR( wxT( "HEADER" ), wxT( "CADSTARSCM" ) ); @@ -145,7 +145,8 @@ void CADSTAR_SCH_ARCHIVE_PARSER::Parse() checkPoint(); } - delete fileRootNode; + delete m_rootNode; + m_rootNode = nullptr; } diff --git a/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.h b/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.h index b88e1085e1..b0ce23cefa 100644 --- a/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.h +++ b/eeschema/sch_plugins/cadstar/cadstar_sch_archive_parser.h @@ -35,11 +35,18 @@ class CADSTAR_SCH_ARCHIVE_PARSER : public CADSTAR_ARCHIVE_PARSER { public: - explicit CADSTAR_SCH_ARCHIVE_PARSER( wxString aFilename ) - : CADSTAR_ARCHIVE_PARSER(), Filename( aFilename ), Header(), Assignments(), KiCadUnitDivider( 10 ) + explicit CADSTAR_SCH_ARCHIVE_PARSER( wxString aFilename ) : + CADSTAR_ARCHIVE_PARSER(), Filename( aFilename ), Header(), Assignments(), + KiCadUnitDivider( 10 ), m_rootNode( nullptr ) { } + virtual ~CADSTAR_SCH_ARCHIVE_PARSER() + { + if( m_rootNode ) + delete m_rootNode; + } + /** * @brief Parses the file * @throw IO_ERROR if file could not be opened or there was @@ -454,6 +461,9 @@ public: int KiCadUnitDivider; ///BeginPhase( 0 ); // Read file - XNODE* fileRootNode = LoadArchiveFile( Filename, wxT( "CADSTARPCB" ), m_progressReporter ); + m_rootNode = LoadArchiveFile( Filename, wxT( "CADSTARPCB" ), m_progressReporter ); if( m_progressReporter ) { @@ -44,13 +44,13 @@ void CADSTAR_PCB_ARCHIVE_PARSER::Parse() std::vector subNodeChildrenToCount = { wxT( "LIBRARY" ), wxT( "PARTS" ), wxT( "LAYOUT" ) }; - long numOfSteps = GetNumberOfStepsForReporting( fileRootNode, subNodeChildrenToCount ); + long numOfSteps = GetNumberOfStepsForReporting( m_rootNode, subNodeChildrenToCount ); m_progressReporter->SetMaxProgress( numOfSteps ); } m_context.CheckPointCallback = [&](){ checkPoint(); }; - XNODE* cNode = fileRootNode->GetChildren(); + XNODE* cNode = m_rootNode->GetChildren(); if( !cNode ) THROW_MISSING_NODE_IO_ERROR( wxT( "HEADER" ), wxT( "CADSTARPCB" ) ); @@ -123,7 +123,7 @@ void CADSTAR_PCB_ARCHIVE_PARSER::Parse() checkPoint(); } - delete fileRootNode; + delete m_rootNode; } diff --git a/pcbnew/plugins/cadstar/cadstar_pcb_archive_parser.h b/pcbnew/plugins/cadstar/cadstar_pcb_archive_parser.h index d69ffde87c..3f8ce62cdb 100644 --- a/pcbnew/plugins/cadstar/cadstar_pcb_archive_parser.h +++ b/pcbnew/plugins/cadstar/cadstar_pcb_archive_parser.h @@ -42,12 +42,20 @@ class CADSTAR_PCB_ARCHIVE_PARSER : public CADSTAR_ARCHIVE_PARSER { public: - explicit CADSTAR_PCB_ARCHIVE_PARSER( const wxString& aFilename ) - : Filename( aFilename ), Header(), Assignments(), CADSTAR_ARCHIVE_PARSER() + explicit CADSTAR_PCB_ARCHIVE_PARSER( const wxString& aFilename ) : + Filename( aFilename ), Header(), Assignments(), + m_rootNode( nullptr ), CADSTAR_ARCHIVE_PARSER() { KiCadUnitMultiplier = 10; // assume hundredth micron } + + virtual ~CADSTAR_PCB_ARCHIVE_PARSER() + { + if( m_rootNode ) + delete m_rootNode; + } + /** * @brief Parses the file * @throw IO_ERROR if file could not be opened or there was @@ -1215,6 +1223,9 @@ public: int KiCadUnitMultiplier; ///