[generator] A fix for applying corrupted diffs.

It looks like sometimes diff files cannot be unarchived.
The result of inflate() should at least contain the diff
header even for diffs created from two identical files but
for some reason it appears to be empty.
This commit stops assuming that the downloaded diff
file can be read without errors.

base/logging.hpp

@@ -41,27 +41,34 @@ LogMessageFn SetLogMessageFn(LogMessageFn fn);
 void LogMessageDefault(LogLevel level, SrcPoint const & srcPoint, std::string const & msg);
 void LogMessageTests(LogLevel level, SrcPoint const & srcPoint, std::string const & msg);
 
-/// Scope Guard to temporarily suppress specific log level, for example, in unit tests:
-/// ...
-/// {
-///   LogLevelSuppressor onlyLERRORAndLCriticalLogsAreEnabled;
-///   TEST(SomeFunctionWhichHasDebugOrInfoOrWarningLogs(), ());
-/// }
+// Scope guard to temporarily suppress a specific log level and all lower ones.
+//
+// For example, in unit tests:
+// {
+//   // Only LERROR and LCRITICAL log messages will be printed.
+//   ScopedLogLevelChanger defaultChanger;
+//   // Call a function that has LDEBUG/LINFO/LWARNING logs that you want to suppress.
+//   TEST(func(), ());
+// }
 struct ScopedLogLevelChanger
 {
-  LogLevel m_old = g_LogLevel;
   ScopedLogLevelChanger(LogLevel temporaryLogLevel = LERROR) { g_LogLevel = temporaryLogLevel; }
+
   ~ScopedLogLevelChanger() { g_LogLevel = m_old; }
+
+  LogLevel m_old = g_LogLevel;
 };
 
 struct ScopedLogAbortLevelChanger
 {
-  LogLevel m_old = g_LogAbortLevel;
   ScopedLogAbortLevelChanger(LogLevel temporaryLogAbortLevel = LCRITICAL)
   {
     g_LogAbortLevel = temporaryLogAbortLevel;
   }
+
   ~ScopedLogAbortLevelChanger() { g_LogAbortLevel = m_old; }
+
+  LogLevel m_old = g_LogAbortLevel;
 };
 }  // namespace base
 

coding/reader.cpp

@@ -2,7 +2,6 @@
 
 #include "base/string_utils.hpp"
 
-
 void Reader::ReadAsString(string & s) const
 {
   s.clear();
@@ -11,6 +10,14 @@ void Reader::ReadAsString(string & s) const
   Read(0, &s[0], sz);
 }
 
+vector<uint8_t> Reader::Contents() const
+{
+  vector<uint8_t> contents;
+  contents.resize(Size());
+  Read(0 /* pos */, contents.data(), contents.size());
+  return contents;
+}
+
 bool Reader::IsEqual(string const & name1, string const & name2)
 {
 #if defined(OMIM_OS_WINDOWS)

coding/reader.hpp

@@ -1,9 +1,11 @@
 #pragma once
+
 #include "coding/endianness.hpp"
 
 #include "base/assert.hpp"
 #include "base/exception.hpp"
 
+#include "std/cstdint.hpp"
 #include "std/cstring.hpp"
 #include "std/shared_array.hpp"
 #include "std/shared_ptr.hpp"
@@ -28,6 +30,10 @@ public:
 
   void ReadAsString(string & s) const;
 
+  // Reads the contents of this Reader to a vector of 8-bit bytes.
+  // Similar to ReadAsString but makes no assumptions about the char type.
+  vector<uint8_t> Contents() const;
+
   static bool IsEqual(string const & name1, string const & name2);
 };
 

generator/mwm_diff/diff.cpp

@@ -64,7 +64,14 @@ bool ApplyDiffVersion0(FileReader & oldReader, FileWriter & newWriter,
   vector<uint8_t> diffBuf;
   inflate(deflatedDiff.data(), deflatedDiff.size(), back_inserter(diffBuf));
 
-  MemReader diffMemReader(diffBuf.data(), diffBuf.size());
+  // Our bsdiff assumes that both the old mwm and the diff files are correct and
+  // does no checks when using its readers.
+  // Yet sometimes we observe corrupted files in the logs, and to avoid
+  // crashes from such files the exception-throwing version of MemReader is used here.
+  // |oldReader| is a FileReader so it throws exceptions too but we
+  // are more confident in the uncorrupted status of the old file because
+  // its checksum is compared to the one stored in the diff file.
+  MemReaderWithExceptions diffMemReader(diffBuf.data(), diffBuf.size());
 
   auto status = bsdiff::ApplyBinaryPatch(oldReader, newWriter, diffMemReader);
 

generator/mwm_diff/mwm_diff_tests/diff_test.cpp

@@ -5,11 +5,17 @@
 #include "platform/platform.hpp"
 
 #include "coding/file_name_utils.hpp"
+#include "coding/file_reader.hpp"
 #include "coding/file_writer.hpp"
 #include "coding/internal/file_data.hpp"
 
+#include "base/logging.hpp"
 #include "base/scope_guard.hpp"
 
+#include <cstdint>
+#include <string>
+#include <vector>
+
 using namespace std;
 
 namespace generator
@@ -18,6 +24,8 @@ namespace mwm_diff
 {
 UNIT_TEST(IncrementalUpdates_Smoke)
 {
+  base::ScopedLogAbortLevelChanger ignoreLogError(base::LogLevel::LCRITICAL);
+
   string const oldMwmPath = base::JoinFoldersToPath(GetPlatform().WritableDir(), "minsk-pass.mwm");
   string const newMwmPath1 =
       base::JoinFoldersToPath(GetPlatform().WritableDir(), "minsk-pass-new1.mwm");
@@ -39,7 +47,42 @@ UNIT_TEST(IncrementalUpdates_Smoke)
   TEST(MakeDiff(oldMwmPath, newMwmPath1, diffPath), ());
   TEST(ApplyDiff(oldMwmPath, newMwmPath2, diffPath), ());
 
+  {
+    // Alter the old mwm slightly.
+    vector<uint8_t> oldMwmContents = FileReader(oldMwmPath).Contents();
+    size_t const sz = oldMwmContents.size();
+    for (size_t i = 3 * sz / 10; i < 4 * sz / 10; i++)
+      oldMwmContents[i] += static_cast<uint8_t>(i);
+
+    FileWriter writer(newMwmPath1);
+    writer.Write(oldMwmContents.data(), oldMwmContents.size());
+  }
+
+  TEST(MakeDiff(oldMwmPath, newMwmPath1, diffPath), ());
+  TEST(ApplyDiff(oldMwmPath, newMwmPath2, diffPath), ());
+
   TEST(base::IsEqualFiles(newMwmPath1, newMwmPath2), ());
+
+  {
+    // Corrupt the diff file contents.
+    vector<uint8_t> diffContents = FileReader(diffPath).Contents();
+
+    // Leave the version bits intact.
+    for (size_t i = 4; i < diffContents.size(); i += 2)
+      diffContents[i] ^= 255;
+
+    FileWriter writer(diffPath);
+    writer.Write(diffContents.data(), diffContents.size());
+  }
+
+  TEST(!ApplyDiff(oldMwmPath, newMwmPath2, diffPath), ());
+
+  {
+    // Reset the diff file contents.
+    FileWriter writer(diffPath);
+  }
+
+  TEST(!ApplyDiff(oldMwmPath, newMwmPath2, diffPath), ());
 }
 }  // namespace mwm_diff
 }  // namespace generator