From e89964ebb3ba3bd7d694695c004c5f976d8d9616 Mon Sep 17 00:00:00 2001 From: Caolán McNamara Date: Tue, 6 Feb 2018 15:36:07 +0000 Subject: ofz: Pos2Page returns true on same value that returned false previously MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a failed position returns false, but stays at the failed position, so next time its called without moving it then it returns true store what we return for a given position for reuse if the position doesn't change Change-Id: I404c65ac89eb6f5c867f62a62028b87effdbcbf8 Reviewed-on: https://gerrit.libreoffice.org/49308 Tested-by: Jenkins Reviewed-by: Caolán McNamara Tested-by: Caolán McNamara --- sot/qa/cppunit/data/pass/badchain-1.compound | Bin 0 -> 1041 bytes sot/qa/cppunit/test_sot.cxx | 1 - sot/source/sdstor/stgdir.cxx | 3 +- sot/source/sdstor/stgstrms.cxx | 94 +++++++++++++++------------ sot/source/sdstor/stgstrms.hxx | 12 ++-- 5 files changed, 63 insertions(+), 47 deletions(-) create mode 100644 sot/qa/cppunit/data/pass/badchain-1.compound (limited to 'sot') diff --git a/sot/qa/cppunit/data/pass/badchain-1.compound b/sot/qa/cppunit/data/pass/badchain-1.compound new file mode 100644 index 000000000000..4c70faf207fd Binary files /dev/null and b/sot/qa/cppunit/data/pass/badchain-1.compound differ diff --git a/sot/qa/cppunit/test_sot.cxx b/sot/qa/cppunit/test_sot.cxx index c4e961271801..0ffbe758a1ec 100644 --- a/sot/qa/cppunit/test_sot.cxx +++ b/sot/qa/cppunit/test_sot.cxx @@ -63,7 +63,6 @@ namespace // Read as much as we can, a corrupted FAT chain can cause real grief here nReadableSize = xStream->ReadBytes(static_cast(pData), nSize); -// fprintf(stderr, "readable size %d vs size %d remaining %d\n", nReadableSize, nSize, nReadableSize); } { // Read the data backwards as well tools::SvRef xStream( xObjStor->OpenSotStream( rStreamName ) ); diff --git a/sot/source/sdstor/stgdir.cxx b/sot/source/sdstor/stgdir.cxx index 253fb4399d05..b2e64967c436 100644 --- a/sot/source/sdstor/stgdir.cxx +++ b/sot/source/sdstor/stgdir.cxx @@ -839,7 +839,8 @@ bool StgDirStrm::Store() sal_Int32 nOldStart = m_nStart; // save for later deletion sal_Int32 nOldSize = m_nSize; m_nStart = m_nPage = STG_EOF; - m_nSize = m_nPos = 0; + m_nSize = 0; + SetPos(0, true); m_nOffset = 0; // Delete all temporary entries m_pRoot->DelTemp( false ); diff --git a/sot/source/sdstor/stgstrms.cxx b/sot/source/sdstor/stgstrms.cxx index 8e5093fe9a91..a9e7217a3a21 100644 --- a/sot/source/sdstor/stgstrms.cxx +++ b/sot/source/sdstor/stgstrms.cxx @@ -322,11 +322,12 @@ bool StgFAT::FreePages( sal_Int32 nStart, bool bAll ) // FAT class for the page allocations. StgStrm::StgStrm( StgIo& r ) - : m_rIo(r), + : m_nPos(0), + m_bBytePosValid(true), + m_rIo(r), m_pEntry(nullptr), m_nStart(STG_EOF), m_nSize(0), - m_nPos(0), m_nPage(STG_EOF), m_nOffset(0), m_nPageSize(m_rIo.GetPhysPageSize()) @@ -356,7 +357,10 @@ void StgStrm::SetEntry( StgDirEntry& r ) sal_Int32 StgStrm::scanBuildPageChainCache() { if (m_nSize > 0) + { m_aPagesCache.reserve(m_nSize/m_nPageSize); + m_aUsedPageNumbers.reserve(m_nSize/m_nPageSize); + } bool bError = false; sal_Int32 nBgn = m_nStart; @@ -364,8 +368,6 @@ sal_Int32 StgStrm::scanBuildPageChainCache() // Track already scanned PageNumbers here and use them to // see if an already counted page is re-visited - std::set< sal_Int32 > nUsedPageNumbers; - while( nBgn >= 0 && !bError ) { if( nBgn >= 0 ) @@ -373,7 +375,7 @@ sal_Int32 StgStrm::scanBuildPageChainCache() nBgn = m_pFat->GetNextPage( nBgn ); //returned second is false if it already exists - if (!nUsedPageNumbers.insert(nBgn).second) + if (!m_aUsedPageNumbers.insert(nBgn).second) { SAL_WARN ("sot", "Error: page number " << nBgn << " already in chain for stream"); bError = true; @@ -386,6 +388,7 @@ sal_Int32 StgStrm::scanBuildPageChainCache() SAL_WARN("sot", "returning wrong format error"); m_rIo.SetError( ERRCODE_IO_WRONGFORMAT ); m_aPagesCache.clear(); + m_aUsedPageNumbers.clear(); } return nOptSize; } @@ -408,8 +411,8 @@ bool StgStrm::Pos2Page( sal_Int32 nBytePos ) sal_Int32 nNew = nBytePos & nMask; m_nOffset = static_cast( nBytePos & ~nMask ); m_nPos = nBytePos; - if( nOld == nNew ) - return true; + if (nOld == nNew) + return m_bBytePosValid; // See fdo#47644 for a .doc with a vast amount of pages where seeking around the // document takes a colossal amount of time @@ -423,7 +426,11 @@ bool StgStrm::Pos2Page( sal_Int32 nBytePos ) size_t nToAdd = nIdx + 1; if (m_aPagesCache.empty()) + { m_aPagesCache.push_back( m_nStart ); + assert(m_aUsedPageNumbers.empty()); + m_aUsedPageNumbers.insert(m_nStart); + } nToAdd -= m_aPagesCache.size(); @@ -436,21 +443,16 @@ bool StgStrm::Pos2Page( sal_Int32 nBytePos ) nBgn = m_pFat->GetNextPage(nOldBgn); if( nBgn >= 0 ) { - if (nOldBgn != nBgn) + //returned second is false if it already exists + if (!m_aUsedPageNumbers.insert(nBgn).second) { - //very much the normal case - m_aPagesCache.push_back(nBgn); - --nToAdd; - } - else - { - //unclear if this is something we should just immediately - //reject, or allow, for the moment support it but - //optimize that all the pages are the same - SAL_WARN("sot", "fat next page is the same as current page, autofilling " << nToAdd << " pages"); - m_aPagesCache.insert(m_aPagesCache.end(), nToAdd, nBgn); - nToAdd = 0; + SAL_WARN ("sot", "Error: page number " << nBgn << " already in chain for stream"); + break; } + + //very much the normal case + m_aPagesCache.push_back(nBgn); + --nToAdd; } } } @@ -467,6 +469,7 @@ bool StgStrm::Pos2Page( sal_Int32 nBytePos ) // nIdx = m_aPagesCache.size(); // nPos = nPageSize * nIdx; // so retain this behavior for now. + m_bBytePosValid = false; return false; } @@ -480,12 +483,14 @@ bool StgStrm::Pos2Page( sal_Int32 nBytePos ) else if ( nIdx == m_aPagesCache.size() ) { m_nPage = STG_EOF; + m_bBytePosValid = false; return false; } m_nPage = m_aPagesCache[ nIdx ]; - return m_nPage >= 0; + m_bBytePosValid = m_nPage >= 0; + return m_bBytePosValid; } // Copy an entire stream. Both streams are allocated in the FAT. @@ -497,6 +502,7 @@ bool StgStrm::Copy( sal_Int32 nFrom, sal_Int32 nBytes ) return false; m_aPagesCache.clear(); + m_aUsedPageNumbers.clear(); sal_Int32 nTo = m_nStart; sal_Int32 nPgs = ( nBytes + m_nPageSize - 1 ) / m_nPageSize; @@ -528,6 +534,7 @@ bool StgStrm::SetSize( sal_Int32 nBytes ) return false; m_aPagesCache.clear(); + m_aUsedPageNumbers.clear(); // round up to page size sal_Int32 nOld = ( ( m_nSize + m_nPageSize - 1 ) / m_nPageSize ) * m_nPageSize; @@ -585,9 +592,10 @@ bool StgFATStrm::Pos2Page( sal_Int32 nBytePos ) nBytePos = m_nSize ? m_nSize - 1 : 0; m_nPage = nBytePos / m_nPageSize; m_nOffset = static_cast( nBytePos % m_nPageSize ); - m_nPos = nBytePos; m_nPage = GetPage(m_nPage, false); - return m_nPage >= 0; + bool bValid = m_nPage >= 0; + SetPos(nBytePos, bValid); + return bValid; } // Get the page number entry for the given page offset. @@ -616,6 +624,7 @@ sal_Int32 StgFATStrm::GetPage(sal_Int32 nOff, bool bMake, sal_uInt16 *pnMasterAl if( bMake ) { m_aPagesCache.clear(); + m_aUsedPageNumbers.clear(); // create a new master page nFAT = nMaxPage++; @@ -673,6 +682,7 @@ bool StgFATStrm::SetPage( short nOff, sal_Int32 nNewPage ) { OSL_ENSURE( nOff >= 0, "The offset may not be negative!" ); m_aPagesCache.clear(); + m_aUsedPageNumbers.clear(); bool bRes = true; if( nOff < StgHeader::GetFAT1Size() ) @@ -727,6 +737,7 @@ bool StgFATStrm::SetSize( sal_Int32 nBytes ) return false; m_aPagesCache.clear(); + m_aUsedPageNumbers.clear(); // Set the number of entries to a multiple of the page size short nOld = static_cast( ( m_nSize + ( m_nPageSize - 1 ) ) / m_nPageSize ); @@ -911,7 +922,7 @@ sal_Int32 StgDataStrm::Read( void* pBuf, sal_Int32 n ) if ( n < 0 ) return 0; - const auto nAvailable = m_nSize - m_nPos; + const auto nAvailable = m_nSize - GetPos(); if (n > nAvailable) n = nAvailable; sal_Int32 nDone = 0; @@ -948,14 +959,14 @@ sal_Int32 StgDataStrm::Read( void* pBuf, sal_Int32 n ) nRes = nBytes; } nDone += nRes; - m_nPos += nRes; + SetPos(GetPos() + nRes, true); n -= nRes; m_nOffset = m_nOffset + nRes; if( nRes != nBytes ) break; // read error or EOF } // Switch to next page if necessary - if( m_nOffset >= m_nPageSize && !Pos2Page( m_nPos ) ) + if (m_nOffset >= m_nPageSize && !Pos2Page(GetPos())) break; } return nDone; @@ -967,10 +978,10 @@ sal_Int32 StgDataStrm::Write( const void* pBuf, sal_Int32 n ) return 0; sal_Int32 nDone = 0; - if( ( m_nPos + n ) > m_nSize ) + if( ( GetPos() + n ) > m_nSize ) { - sal_Int32 nOld = m_nPos; - if( !SetSize( m_nPos + n ) ) + sal_Int32 nOld = GetPos(); + if( !SetSize( nOld + n ) ) return 0; Pos2Page( nOld ); } @@ -1009,14 +1020,14 @@ sal_Int32 StgDataStrm::Write( const void* pBuf, sal_Int32 n ) nRes = nBytes; } nDone += nRes; - m_nPos += nRes; + SetPos(GetPos() + nRes, true); n -= nRes; m_nOffset = m_nOffset + nRes; if( nRes != nBytes ) break; // read error } // Switch to next page if necessary - if( m_nOffset >= m_nPageSize && !Pos2Page( m_nPos ) ) + if( m_nOffset >= m_nPageSize && !Pos2Page(GetPos()) ) break; } return nDone; @@ -1062,8 +1073,9 @@ sal_Int32 StgSmallStrm::Read( void* pBuf, sal_Int32 n ) { // We can safely assume that reads are not huge, since the // small stream is likely to be < 64 KBytes. - if( ( m_nPos + n ) > m_nSize ) - n = m_nSize - m_nPos; + sal_Int32 nBytePos = GetPos(); + if( ( nBytePos + n ) > m_nSize ) + n = m_nSize - nBytePos; sal_Int32 nDone = 0; while( n ) { @@ -1082,7 +1094,7 @@ sal_Int32 StgSmallStrm::Read( void* pBuf, sal_Int32 n ) // all reading through the stream short nRes = static_cast(m_pData->Read( static_cast(pBuf) + nDone, nBytes )); nDone += nRes; - m_nPos += nRes; + SetPos(GetPos() + nRes, true); n -= nRes; m_nOffset = m_nOffset + nRes; // read problem? @@ -1090,7 +1102,7 @@ sal_Int32 StgSmallStrm::Read( void* pBuf, sal_Int32 n ) break; } // Switch to next page if necessary - if( m_nOffset >= m_nPageSize && !Pos2Page( m_nPos ) ) + if (m_nOffset >= m_nPageSize && !Pos2Page(GetPos())) break; } return nDone; @@ -1101,12 +1113,12 @@ sal_Int32 StgSmallStrm::Write( const void* pBuf, sal_Int32 n ) // you can safely assume that reads are not huge, since the // small stream is likely to be < 64 KBytes. sal_Int32 nDone = 0; - if( ( m_nPos + n ) > m_nSize ) + sal_Int32 nOldPos = GetPos(); + if( ( nOldPos + n ) > m_nSize ) { - sal_Int32 nOld = m_nPos; - if( !SetSize( m_nPos + n ) ) + if (!SetSize(nOldPos + n)) return 0; - Pos2Page( nOld ); + Pos2Page(nOldPos); } while( n ) { @@ -1125,7 +1137,7 @@ sal_Int32 StgSmallStrm::Write( const void* pBuf, sal_Int32 n ) break; short nRes = static_cast(m_pData->Write( static_cast(pBuf) + nDone, nBytes )); nDone += nRes; - m_nPos += nRes; + SetPos(GetPos() + nRes, true); n -= nRes; m_nOffset = m_nOffset + nRes; // write problem? @@ -1133,7 +1145,7 @@ sal_Int32 StgSmallStrm::Write( const void* pBuf, sal_Int32 n ) break; } // Switch to next page if necessary - if( m_nOffset >= m_nPageSize && !Pos2Page( m_nPos ) ) + if( m_nOffset >= m_nPageSize && !Pos2Page(GetPos()) ) break; } return nDone; diff --git a/sot/source/sdstor/stgstrms.hxx b/sot/source/sdstor/stgstrms.hxx index a2c8ca6de383..51c08faf5312 100644 --- a/sot/source/sdstor/stgstrms.hxx +++ b/sot/source/sdstor/stgstrms.hxx @@ -21,8 +21,8 @@ #define INCLUDED_SOT_SOURCE_SDSTOR_STGSTRMS_HXX #include +#include #include - #include #include @@ -61,25 +61,29 @@ public: // and accessing the data on a physical basis. It uses the built-in // FAT class for the page allocations. -class StgStrm { // base class for all streams +class StgStrm { // base class for all streams +private: + sal_Int32 m_nPos; // current byte position + bool m_bBytePosValid; // what Pos2Page returns for m_nPos protected: StgIo& m_rIo; // I/O system std::unique_ptr m_pFat; // FAT stream for allocations StgDirEntry* m_pEntry; // dir entry (for ownership) sal_Int32 m_nStart; // 1st data page sal_Int32 m_nSize; // stream size in bytes - sal_Int32 m_nPos; // current byte position sal_Int32 m_nPage; // current logical page short m_nOffset; // offset into current page short m_nPageSize; // logical page size std::vector m_aPagesCache; + o3tl::sorted_vector m_aUsedPageNumbers; sal_Int32 scanBuildPageChainCache(); bool Copy( sal_Int32 nFrom, sal_Int32 nBytes ); + void SetPos(sal_Int32 nPos, bool bValid) { m_nPos = nPos; m_bBytePosValid = bValid; } explicit StgStrm( StgIo& ); public: virtual ~StgStrm(); StgIo& GetIo() { return m_rIo; } - sal_Int32 GetPos() const { return m_nPos; } + sal_Int32 GetPos() const { return m_nPos; } sal_Int32 GetStart() const { return m_nStart; } sal_Int32 GetSize() const { return m_nSize; } sal_Int32 GetPage() const { return m_nPage; } -- cgit v1.2.3