From a11ed230844873cef34e4c5c0d1536f4ed14b401 Mon Sep 17 00:00:00 2001 From: Stephan Bergmann Date: Fri, 17 Aug 2018 20:09:19 +0200 Subject: Avoid further downstream overflow ...after 004304eb2fd1703d22dec0abf0170bb2ce493d0c "try to avoid overflows in some compare functions" had changed the return type of just one function, but not its callers. Found with Clang's new -fsanitize=implicit-conversion during CppunitTest_sd_filters_test: > Testing file:///home/sbergman/lo/core/sd/qa/unit/data/ppt/pass/hang-17.ppt: [...] > sot/source/sdstor/stgdir.cxx:101:19: runtime error: implicit conversion from type 'sal_Int32' (aka 'int') of value -57120 (32-bit, signed) to type 'short' changed the value to 8416 (16-bit, signed) > #0 in StgDirEntry::Compare(StgAvlNode const*) const at sot/source/sdstor/stgdir.cxx:101:19 (instdir/program/libsotlo.so +0x217699) > #1 in StgAvlNode::Find(StgAvlNode const*) at sot/source/sdstor/stgavl.cxx:43:29 (instdir/program/libsotlo.so +0x1db72b) > #2 in StgDirStrm::Find(StgDirEntry&, rtl::OUString const&) at sot/source/sdstor/stgdir.cxx:907:57 (instdir/program/libsotlo.so +0x22f2dc) > #3 in Storage::IsStream(rtl::OUString const&) const at sot/source/sdstor/stg.cxx:773:39 (instdir/program/libsotlo.so +0x1d2cdf) > #4 in SotStorage::IsStream(rtl::OUString const&) const at sot/source/sdstor/storage.cxx:654:27 (instdir/program/libsotlo.so +0x29ebdb) > #5 in PropRead::PropRead(SotStorage&, rtl::OUString const&) at sd/source/filter/ppt/propread.cxx:543:19 (instdir/program/libsdfiltlo.so +0x6b72ee) > #6 in ImplSdPPTImport::Import() at sd/source/filter/ppt/pptin.cxx:262:32 (instdir/program/libsdfiltlo.so +0x5d0dc4) > #7 in SdPPTImport::Import() at sd/source/filter/ppt/pptin.cxx:167:21 (instdir/program/libsdfiltlo.so +0x5cf733) > #8 in ImportPPT at sd/source/filter/ppt/pptin.cxx:2761:26 (instdir/program/libsdfiltlo.so +0x618f64) > #9 in SdPPTFilter::Import() at sd/source/filter/sdpptwrp.cxx:106:32 (instdir/program/libsdlo.so +0x2877ed3) > #10 in sd::DrawDocShell::ConvertFrom(SfxMedium&) at sd/source/ui/docshell/docshel4.cxx:474:46 (instdir/program/libsdlo.so +0x2e1607c) > #11 in SfxObjectShell::DoLoad(SfxMedium*) at sfx2/source/doc/objstor.cxx:786:23 (instdir/program/libsfxlo.so +0x2c8c762) > #12 in SdFiltersTest::load(rtl::OUString const&, rtl::OUString const&, rtl::OUString const&, SfxFilterFlags, SotClipboardFormatId, unsigned int) at sd/qa/unit/filters-test.cxx:75:31 (workdir/LinkTarget/CppunitTest/libtest_sd_filters_test.so +0x19771) > #13 in test::FiltersTest::recursiveScan(test::filterStatus, rtl::OUString const&, rtl::OUString const&, rtl::OUString const&, SfxFilterFlags, SotClipboardFormatId, unsigned int, bool) at unotest/source/cpp/filters-test.cxx:130:20 (workdir/LinkTarget/CppunitTest/../Library/libunotest.so +0x5724c) > #14 in test::FiltersTest::testDir(rtl::OUString const&, rtl::OUString const&, rtl::OUString const&, SfxFilterFlags, SotClipboardFormatId, unsigned int, bool) at unotest/source/cpp/filters-test.cxx:155:5 (workdir/LinkTarget/CppunitTest/../Library/libunotest.so +0x57ec9) > #15 in SdFiltersTest::testCVEs() at sd/qa/unit/filters-test.cxx:83:5 (workdir/LinkTarget/CppunitTest/libtest_sd_filters_test.so +0x19d6d) Change-Id: Iaffd35d33f0e1109195e3bd56538104d395af01b Reviewed-on: https://gerrit.libreoffice.org/59274 Tested-by: Jenkins Reviewed-by: Stephan Bergmann (cherry picked from commit 4cb69cf33b5bf17030bcd263fe31258177c76d5e) Reviewed-on: https://gerrit.libreoffice.org/59277 Reviewed-by: Michael Stahl --- sot/source/sdstor/stgavl.cxx | 12 ++++++------ sot/source/sdstor/stgavl.hxx | 4 ++-- sot/source/sdstor/stgdir.cxx | 4 ++-- sot/source/sdstor/stgdir.hxx | 2 +- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/sot/source/sdstor/stgavl.cxx b/sot/source/sdstor/stgavl.cxx index d488eec69130..0c78054045f0 100644 --- a/sot/source/sdstor/stgavl.cxx +++ b/sot/source/sdstor/stgavl.cxx @@ -40,7 +40,7 @@ StgAvlNode* StgAvlNode::Find( StgAvlNode const * pFind ) StgAvlNode* p = this; while( p ) { - short nRes = p->Compare( pFind ); + sal_Int32 nRes = p->Compare( pFind ); if( !nRes ) return p; else p = ( nRes < 0 ) ? p->m_pLeft : p->m_pRight; @@ -52,11 +52,11 @@ StgAvlNode* StgAvlNode::Find( StgAvlNode const * pFind ) // find point to add node to AVL tree and returns // +/0/- for >/=/< previous -short StgAvlNode::Locate +sal_Int32 StgAvlNode::Locate ( StgAvlNode const * pFind, StgAvlNode** pPivot, StgAvlNode **pParent, StgAvlNode** pPrev ) { - short nRes = 0; + sal_Int32 nRes = 0; StgAvlNode* pCur = this; OSL_ENSURE( pPivot && pParent && pPrev, "The pointers may not be NULL!" ); @@ -98,7 +98,7 @@ short StgAvlNode::Adjust( StgAvlNode** pHeavy, StgAvlNode const * pNew ) if( pCur == pNew || !pNew ) return m_nBalance; - short nRes = Compare( pNew ); + sal_Int32 nRes = Compare( pNew ); if( nRes > 0 ) { *pHeavy = pCur = m_pRight; @@ -221,7 +221,7 @@ StgAvlNode* StgAvlNode::Rem( StgAvlNode** p, StgAvlNode* pDel, bool bPtrs ) if( p && *p && pDel ) { StgAvlNode* pCur = *p; - short nRes = bPtrs ? short( pCur == pDel ) : pCur->Compare( pDel ); + sal_Int32 nRes = bPtrs ? sal_Int32( pCur == pDel ) : pCur->Compare( pDel ); if( !nRes ) { // Element found: remove @@ -295,7 +295,7 @@ bool StgAvlNode::Insert( StgAvlNode** pRoot, StgAvlNode* pIns ) return true; } // find insertion point and return if already present - short nRes = (*pRoot)->Locate( pIns, &pPivot, &pParent, &pPrev ); + sal_Int32 nRes = (*pRoot)->Locate( pIns, &pPivot, &pParent, &pPrev ); if( !nRes ) return false; diff --git a/sot/source/sdstor/stgavl.hxx b/sot/source/sdstor/stgavl.hxx index d521eb00b290..5dd29a988f30 100644 --- a/sot/source/sdstor/stgavl.hxx +++ b/sot/source/sdstor/stgavl.hxx @@ -27,7 +27,7 @@ class StgAvlNode { friend class StgAvlIterator; private: - short Locate( StgAvlNode const *, StgAvlNode**, StgAvlNode**, StgAvlNode** ); + sal_Int32 Locate( StgAvlNode const *, StgAvlNode**, StgAvlNode**, StgAvlNode** ); short Adjust( StgAvlNode**, StgAvlNode const * ); StgAvlNode* RotLL(); StgAvlNode* RotLR(); @@ -45,7 +45,7 @@ public: StgAvlNode* Find( StgAvlNode const * ); static bool Insert( StgAvlNode**, StgAvlNode* ); static bool Remove( StgAvlNode**, StgAvlNode*, bool bDel ); - virtual short Compare( const StgAvlNode* ) const = 0; + virtual sal_Int32 Compare( const StgAvlNode* ) const = 0; }; // The iterator class provides single stepping through an AVL tree. diff --git a/sot/source/sdstor/stgdir.cxx b/sot/source/sdstor/stgdir.cxx index b2e64967c436..6ca1d60b61f7 100644 --- a/sot/source/sdstor/stgdir.cxx +++ b/sot/source/sdstor/stgdir.cxx @@ -92,9 +92,9 @@ StgDirEntry::~StgDirEntry() // Comparison function -short StgDirEntry::Compare( const StgAvlNode* p ) const +sal_Int32 StgDirEntry::Compare( const StgAvlNode* p ) const { - short nResult = -1; + sal_Int32 nResult = -1; if ( p ) { const StgDirEntry* pEntry = static_cast(p); diff --git a/sot/source/sdstor/stgdir.hxx b/sot/source/sdstor/stgdir.hxx index 95c624c1205b..b4f1035c0078 100644 --- a/sot/source/sdstor/stgdir.hxx +++ b/sot/source/sdstor/stgdir.hxx @@ -45,7 +45,7 @@ class StgDirEntry : public StgAvlNode bool m_bDirty; // dirty directory entry bool m_bRemoved; // removed per Invalidate() void InitMembers(); // ctor helper - virtual short Compare( const StgAvlNode* ) const override; + virtual sal_Int32 Compare( const StgAvlNode* ) const override; bool StoreStream( StgIo& ); // store the stream bool StoreStreams( StgIo& ); // store all streams void RevertAll(); // revert the whole tree -- cgit v1.2.3