Fix bug with ReaderSource::Read for invalid pos.

Add some assertions to readers.

6 files changed, 83 insertions, 25 deletions

diff --git a/coding/file_reader.cpp b/coding/file_reader.cpp
index e652fba58b..a993ccb34a 100644
--- a/coding/file_reader.cpp
+++ b/coding/file_reader.cpp
@@ -86,25 +86,38 @@ uint64_t FileReader::Size() const
 
 void FileReader::Read(uint64_t pos, void * p, size_t size) const
 {
-  ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
+  ASSERT ( AssertPosAndSize(pos, size), () );
   m_pFileData->Read(m_Offset + pos, p, size);
 }
 
 FileReader FileReader::SubReader(uint64_t pos, uint64_t size) const
 {
-  ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
+  ASSERT ( AssertPosAndSize(pos, size), () );
   return FileReader(*this, m_Offset + pos, size);
 }
 
 FileReader * FileReader::CreateSubReader(uint64_t pos, uint64_t size) const
 {
-  ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
+  ASSERT ( AssertPosAndSize(pos, size), () );
   return new FileReader(*this, m_Offset + pos, size);
 }
 
+bool FileReader::AssertPosAndSize(uint64_t pos, uint64_t size) const
+{
+  uint64_t const allSize1 = Size();
+  bool const ret1 = (pos + size <= allSize1);
+  ASSERT ( ret1, (pos, size, allSize1) );
+
+  uint64_t const allSize2 = m_pFileData->Size();
+  bool const ret2 = (m_Offset + pos + size <= allSize2);
+  ASSERT ( ret2, (m_Offset, pos, size, allSize2) );
+
+  return (ret1 && ret2);
+}
+
 void FileReader::SetOffsetAndSize(uint64_t offset, uint64_t size)
 {
-  ASSERT_LESS_OR_EQUAL(offset + size, Size(), (offset, size));
+  ASSERT ( AssertPosAndSize(offset, size), () );
   m_Offset = offset;
   m_Size = size;
 }
diff --git a/coding/file_reader.hpp b/coding/file_reader.hpp
index 76cfa881f8..cb8e080ce1 100644
--- a/coding/file_reader.hpp
+++ b/coding/file_reader.hpp
@@ -23,7 +23,9 @@ public:
   FileReader * CreateSubReader(uint64_t pos, uint64_t size) const;
 
 protected:
-  // Used in special derived readers.
+  /// Make assertion that pos + size in FileReader bounds.
+  bool AssertPosAndSize(uint64_t pos, uint64_t size) const;
+  /// Used in special derived readers.
   void SetOffsetAndSize(uint64_t offset, uint64_t size);
 
 private:
diff --git a/coding/parse_xml.hpp b/coding/parse_xml.hpp
index 60b4921554..b5ca32b904 100644
--- a/coding/parse_xml.hpp
+++ b/coding/parse_xml.hpp
@@ -29,7 +29,9 @@ bool ParseXML(SourceT & source, XMLDispatcherT & dispatcher, bool useCharData =
   }
   catch (SourceOutOfBoundsException & e)
   {
-    if (!parser.ParseBuffer(e.BytesRead(), true))
+    size_t const toRead = e.BytesRead();
+    // 0 - means Reader overflow (see ReaderSource::Read)
+    if (toRead == 0 || !parser.ParseBuffer(toRead, true))
       return false;
   }
 
diff --git a/coding/reader.cpp b/coding/reader.cpp
index 4119bb2e0b..f2ae3b2f68 100644
--- a/coding/reader.cpp
+++ b/coding/reader.cpp
@@ -19,3 +19,24 @@ bool Reader::IsEqual(string const & name1, string const & name2)
   return (name1 == name2);
 #endif
 }
+
+namespace
+{
+  bool AssertPosAndSizeImpl(uint64_t pos, uint64_t size, uint64_t readerSize)
+  {
+    bool const ret1 = (pos + size <= readerSize);
+    bool const ret2 = (size <= static_cast<size_t>(-1));
+    ASSERT ( ret1 && ret2, (pos, size, readerSize) );
+    return (ret1 && ret2);
+  }
+}
+
+bool MemReader::AssertPosAndSize(uint64_t pos, uint64_t size) const
+{
+  return AssertPosAndSizeImpl(pos, size, Size());
+}
+
+bool SharedMemReader::AssertPosAndSize(uint64_t pos, uint64_t size) const
+{
+  return AssertPosAndSizeImpl(pos, size, Size());
+}
diff --git a/coding/reader.hpp b/coding/reader.hpp
index e0605e541b..df33f77176 100644
--- a/coding/reader.hpp
+++ b/coding/reader.hpp
@@ -3,9 +3,8 @@
 #include "source.hpp"
 
 #include "../base/assert.hpp"
-#include "../base/base.hpp"
+#include "../base/logging.hpp"
 #include "../base/exception.hpp"
-#include "../base/macros.hpp"
 
 #include "../std/shared_array.hpp"
 #include "../std/shared_ptr.hpp"
@@ -34,6 +33,8 @@ public:
 // Reader from memory.
 class MemReader : public Reader
 {
+  bool AssertPosAndSize(uint64_t pos, uint64_t size) const;
+
 public:
   // Construct from block of memory.
   MemReader(void const * pData, size_t size)
@@ -48,21 +49,19 @@ public:
 
   inline void Read(uint64_t pos, void * p, size_t size) const
   {
-    ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
+    ASSERT ( AssertPosAndSize(pos, size), () );
     memcpy(p, m_pData + pos, size);
   }
 
   inline MemReader SubReader(uint64_t pos, uint64_t size) const
   {
-    ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
-    ASSERT_LESS_OR_EQUAL(size, static_cast<size_t>(-1), ());
+    ASSERT ( AssertPosAndSize(pos, size), () );
     return MemReader(m_pData + pos, static_cast<size_t>(size));
   }
 
   inline MemReader * CreateSubReader(uint64_t pos, uint64_t size) const
   {
-    ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
-    ASSERT_LESS_OR_EQUAL(size, static_cast<size_t>(-1), ());
+    ASSERT ( AssertPosAndSize(pos, size), () );
     return new MemReader(m_pData + pos, static_cast<size_t>(size));
   }
 
@@ -73,6 +72,8 @@ private:
 
 class SharedMemReader
 {
+  bool AssertPosAndSize(uint64_t pos, uint64_t size) const;
+
 public:
   explicit SharedMemReader(size_t size) : m_data(new char[size]), m_offset(0), m_size(size) {}
 
@@ -82,21 +83,19 @@ public:
 
   inline void Read(uint64_t pos, void * p, size_t size) const
   {
-    ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
+    ASSERT ( AssertPosAndSize(pos, size), () );
     memcpy(p, Data() + pos, size);
   }
 
   inline SharedMemReader SubReader(uint64_t pos, uint64_t size) const
   {
-    ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
-    ASSERT_LESS_OR_EQUAL(size, static_cast<size_t>(-1), ());
+    ASSERT ( AssertPosAndSize(pos, size), () );
     return SharedMemReader(m_data, static_cast<size_t>(pos), static_cast<size_t>(size));
   }
 
   inline SharedMemReader * CreateSubReader(uint64_t pos, uint64_t size) const
   {
-    ASSERT_LESS_OR_EQUAL(pos + size, Size(), (pos, size));
-    ASSERT_LESS_OR_EQUAL(size, static_cast<size_t>(-1), ());
+    ASSERT ( AssertPosAndSize(pos, size), () );
     return new SharedMemReader(m_data, static_cast<size_t>(pos), static_cast<size_t>(size));
   }
 
@@ -180,11 +179,16 @@ public:
 
   void Read(void * p, size_t size)
   {
-    if (m_pos + size > m_reader.Size())
+    uint64_t const readerSize = m_reader.Size();
+    if (m_pos + size > readerSize)
     {
-      size_t const remainingSize = static_cast<size_t>(Size());
-      m_reader.Read(m_pos, p, remainingSize);
-      m_pos = m_reader.Size();
+      size_t remainingSize = 0;
+      if (readerSize >= m_pos)
+      {
+        remainingSize = static_cast<size_t>(readerSize - m_pos);
+        m_reader.Read(m_pos, p, remainingSize);
+        m_pos = readerSize;
+      }
       MYTHROW1(SourceOutOfBoundsException, remainingSize, ());
     }
     else
@@ -197,6 +201,7 @@ public:
   void Skip(uint64_t size)
   {
     m_pos += size;
+    ASSERT ( AssertPosition(), () );
   }
 
   uint64_t Pos() const
@@ -206,6 +211,7 @@ public:
 
   uint64_t Size() const
   {
+    ASSERT ( AssertPosition(), () );
     return (m_reader.Size() - m_pos);
   }
 
@@ -216,17 +222,24 @@ public:
 
   ReaderT SubReader(uint64_t size)
   {
-    uint64_t pos = m_pos;
-    m_pos += size;
+    uint64_t const pos = m_pos;
+    Skip(size);
     return m_reader.SubReader(pos, size);
   }
 
   ReaderT SubReader()
   {
-    return SubReader(m_reader.Size() - m_pos);
+    return SubReader(Size());
   }
 
 private:
+  bool AssertPosition() const
+  {
+    bool const ret = (m_pos <= m_reader.Size());
+    ASSERT ( ret, (m_pos, m_reader.Size()) );
+    return ret;
+  }
+
   ReaderT m_reader;
   uint64_t m_pos;
 };
diff --git a/coding/var_serial_vector.hpp b/coding/var_serial_vector.hpp
index b2dc66c3f2..366befb120 100644
--- a/coding/var_serial_vector.hpp
+++ b/coding/var_serial_vector.hpp
@@ -78,6 +78,11 @@ class VarSerialVectorReader
 public:
   template <typename SourceT>
   explicit VarSerialVectorReader(SourceT & source) :
+
+  /// Reading this code can blow your mind :)
+  /// Initialization (and declaration below) order of m_offsetsReader and m_dataReader matters!!!
+  /// @see ReaderSource::SubReader - it's not constant.
+  /// @todo Do this stuff in a better way.
   m_size(ReadPrimitiveFromSource<uint32_t>(source)),
   m_offsetsReader(source.SubReader(m_size * sizeof(uint32_t))),
   m_dataReader(source.SubReader())
@@ -111,6 +116,8 @@ public:
   }
 
 private:
+
+  /// @note Do NOT change declaration order.
   uint64_t m_size;
   ReaderT m_offsetsReader;
   ReaderT m_dataReader;