ofz: Pos2Page returns true on same value that returned false previously

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/49308Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
Tested-by: Caolán McNamara <caolanm@redhat.com>

include/o3tl/sorted_vector.hxx

@@ -86,6 +86,11 @@ public:
         m_vector.clear();
     }
 
+    void reserve(size_type amount)
+    {
+        m_vector.reserve(amount);
+    }
+
     // ACCESSORS
 
     size_type size() const

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<void *>(pData), nSize);
-//            fprintf(stderr, "readable size %d vs size %d remaining %d\n", nReadableSize, nSize, nReadableSize);
         }
         {   // Read the data backwards as well
             tools::SvRef<SotStorageStream> xStream( xObjStor->OpenSotStream( rStreamName ) );

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 );

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<short>( 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<short>( 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<short>( ( 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<short>(m_pData->Read( static_cast<sal_uInt8*>(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<short>(m_pData->Write( static_cast<sal_uInt8 const *>(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;

sot/source/sdstor/stgstrms.hxx

@@ -21,8 +21,8 @@
 #define INCLUDED_SOT_SOURCE_SDSTOR_STGSTRMS_HXX
 
 #include <tools/stream.hxx>
+#include <o3tl/sorted_vector.hxx>
 #include <rtl/ref.hxx>
-
 #include <vector>
 #include <memory>
 
@@ -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<StgFAT> 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<sal_Int32> m_aPagesCache;
+    o3tl::sorted_vector<sal_Int32> 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;  }