From 2acdcb2374e448371b173feb03650d8d6db8aba2 Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Wed, 12 Mar 2014 16:05:37 +0100 Subject: [PATCH] coverity#1158232 Fix ownership of NamedDBs::insert argument f70d03436b7b501e0ed1d745935a204b9b97b855 "coverity#1158232 have a stab at silencing warning with function markup" claimed that NamedDBs::insert always takes ownerhip of its argument, but boost::ptr_set::insert(std::auto_ptr x) simply calls insert(x.release()), so only takes ownership when it returns true. ScDBDocFunc::AddDBRange (sc/source/ui/docshell/dbdocfun.cxx) relies on this behavior, deleting the argument when insert failed. ScDBDocFunc::RenameDBRange (sc/source/ui/docshell/dbdocfun.cxx) relied on this behavior, deleting the argument when insert failed, until f55cc330dec0dec60c755e2ce28a840c7fca1956 "Fixed the fallout of the changes in ScDBCollection" removed the delete (presumably in error?). I put it back in now. All other uses of NamedDBs::insert ignored the return value (witnessed with SAL_WARN_UNUSED_RESULT). Some are insert-if-not-found cases, where I added asserts now (Sc10Import::LoadDataBaseCollection, sc/source/filter/starcalc/scflt.cxx, is not entirely clear to me, so I added a TODO), while others would have potentially leaked the argument, in which cases I fixed the code. Change-Id: Iad40fbeb625c8ce6b0a61cbf16298d71cdc7de80 --- sc/inc/dbdata.hxx | 3 ++- sc/qa/unit/ucalc_formula.cxx | 6 +++++- sc/source/core/data/formulacell.cxx | 7 ++++++- sc/source/core/tool/dbdata.cxx | 1 - sc/source/filter/starcalc/scflt.cxx | 5 ++++- sc/source/filter/xml/xmldrani.cxx | 5 ++++- sc/source/ui/dbgui/dbnamdlg.cxx | 7 ++++++- sc/source/ui/docshell/dbdocfun.cxx | 3 +++ sc/source/ui/docshell/docsh5.cxx | 7 ++++++- 9 files changed, 36 insertions(+), 8 deletions(-) diff --git a/sc/inc/dbdata.hxx b/sc/inc/dbdata.hxx index 78525b78d821..50ec53c486b1 100644 --- a/sc/inc/dbdata.hxx +++ b/sc/inc/dbdata.hxx @@ -177,7 +177,8 @@ public: const_iterator end() const; ScDBData* findByIndex(sal_uInt16 nIndex); ScDBData* findByUpperName(const OUString& rName); - bool insert(ScDBData* p); + // Takes ownership of p iff it returns true: + SAL_WARN_UNUSED_RESULT bool insert(ScDBData* p); void erase(iterator itr); void erase(const ScDBData& r); bool empty() const; diff --git a/sc/qa/unit/ucalc_formula.cxx b/sc/qa/unit/ucalc_formula.cxx index 69fe42051393..3fcc89686fc8 100644 --- a/sc/qa/unit/ucalc_formula.cxx +++ b/sc/qa/unit/ucalc_formula.cxx @@ -145,7 +145,11 @@ void Test::testFormulaCreateStringFromTokens() ScDBData* pData = new ScDBData( OUString::createFromAscii( aDBs[i].pName), aDBs[i].nTab, aDBs[i].nCol1, aDBs[i].nRow1, aDBs[i].nCol2,aDBs[i].nRow2); - pDBs->getNamedDBs().insert(pData); + bool bInserted = pDBs->getNamedDBs().insert(pData); + CPPUNIT_ASSERT_MESSAGE( + OString( + "Failed to insert \"" + OString(aDBs[i].pName) + "\"").getStr(), + bInserted); } const char* aTests[] = { diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx index 5b23770e4440..c500fa68d031 100644 --- a/sc/source/core/data/formulacell.cxx +++ b/sc/source/core/data/formulacell.cxx @@ -17,6 +17,10 @@ * the License at http://www.apache.org/licenses/LICENSE-2.0 . */ +#include + +#include + #include "formulacell.hxx" #include "grouptokenconverter.hxx" @@ -379,7 +383,8 @@ void adjustDBRange(ScToken* pToken, ScDocument& rNewDoc, const ScDocument* pOldD if (!pNewDBData) { pNewDBData = new ScDBData(*pDBData); - aNewNamedDBs.insert(pNewDBData); + bool ins = aNewNamedDBs.insert(pNewDBData); + assert(ins); (void)ins; } pToken->SetIndex(pNewDBData->GetIndex()); } diff --git a/sc/source/core/tool/dbdata.cxx b/sc/source/core/tool/dbdata.cxx index 62e3a36ccf1b..e8f171fa4ce0 100644 --- a/sc/source/core/tool/dbdata.cxx +++ b/sc/source/core/tool/dbdata.cxx @@ -683,7 +683,6 @@ ScDBData* ScDBCollection::NamedDBs::findByUpperName(const OUString& rName) return itr == maDBs.end() ? NULL : &(*itr); } -// coverity[+free : arg-0] bool ScDBCollection::NamedDBs::insert(ScDBData* p) { SAL_WNODEPRECATED_DECLARATIONS_PUSH diff --git a/sc/source/filter/starcalc/scflt.cxx b/sc/source/filter/starcalc/scflt.cxx index 88616d3979df..91bcf02c4836 100644 --- a/sc/source/filter/starcalc/scflt.cxx +++ b/sc/source/filter/starcalc/scflt.cxx @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -1394,7 +1395,9 @@ void Sc10Import::LoadDataBaseCollection() ( SCROW ) pOldData->DataBaseRec.Block.y2, true, ( sal_Bool) pOldData->DataBaseRec.RowHeader ); - pDoc->GetDBCollection()->getNamedDBs().insert(pNewData); + bool ins = pDoc->GetDBCollection()->getNamedDBs().insert(pNewData); + assert(ins); (void)ins; + //TODO: or can this fail (and need delete pNewData)? } } diff --git a/sc/source/filter/xml/xmldrani.cxx b/sc/source/filter/xml/xmldrani.cxx index ab7e2a642d54..7cb8a551c671 100644 --- a/sc/source/filter/xml/xmldrani.cxx +++ b/sc/source/filter/xml/xmldrani.cxx @@ -491,7 +491,10 @@ void ScXMLDatabaseRangeContext::EndElement() if (pData.get()) { setAutoFilterFlags(*pDoc, *pData); - pDoc->GetDBCollection()->getNamedDBs().insert(pData.release()); + if (pDoc->GetDBCollection()->getNamedDBs().insert(pData.get())) + { + pData.release(); + } } } } diff --git a/sc/source/ui/dbgui/dbnamdlg.cxx b/sc/source/ui/dbgui/dbnamdlg.cxx index b1d01b33f484..ba4e781af7e1 100644 --- a/sc/source/ui/dbgui/dbnamdlg.cxx +++ b/sc/source/ui/dbgui/dbnamdlg.cxx @@ -17,6 +17,10 @@ * the License at http://www.apache.org/licenses/LICENSE-2.0 . */ +#include + +#include + #include #include @@ -469,7 +473,8 @@ IMPL_LINK_NOARG(ScDbNameDlg, AddBtnHdl) pNewEntry->SetKeepFmt( m_pBtnKeepFmt->IsChecked() ); pNewEntry->SetStripData( m_pBtnStripData->IsChecked() ); - aLocalDbCol.getNamedDBs().insert(pNewEntry); + bool ins = aLocalDbCol.getNamedDBs().insert(pNewEntry); + assert(ins); (void)ins; } UpdateNames(); diff --git a/sc/source/ui/docshell/dbdocfun.cxx b/sc/source/ui/docshell/dbdocfun.cxx index 8a590db78f8d..1698b621a1c5 100644 --- a/sc/source/ui/docshell/dbdocfun.cxx +++ b/sc/source/ui/docshell/dbdocfun.cxx @@ -167,7 +167,10 @@ bool ScDBDocFunc::RenameDBRange( const OUString& rOld, const OUString& rNew ) rDBs.erase(*pOld); bool bInserted = rDBs.insert(pNewData); if (!bInserted) // Fehler -> alten Zustand wiederherstellen + { + delete pNewData; pDoc->SetDBCollection(pUndoColl); // gehoert dann dem Dokument + } pDoc->CompileDBFormula( false ); // CompileFormulaString diff --git a/sc/source/ui/docshell/docsh5.cxx b/sc/source/ui/docshell/docsh5.cxx index 0366dccae1cf..f3c9f39ebd26 100644 --- a/sc/source/ui/docshell/docsh5.cxx +++ b/sc/source/ui/docshell/docsh5.cxx @@ -17,6 +17,10 @@ * the License at http://www.apache.org/licenses/LICENSE-2.0 . */ +#include + +#include + #include "scitems.hxx" #include #include @@ -274,7 +278,8 @@ ScDBData* ScDocShell::GetDBData( const ScRange& rMarked, ScGetDBMode eMode, ScGe pNoNameData = new ScDBData( aNewName, nTab, nStartCol,nStartRow, nEndCol,nEndRow, true, bHasHeader ); - rDBs.insert(pNoNameData); + bool ins = rDBs.insert(pNoNameData); + assert(ins); (void)ins; } else {