From a724c0e417e7ef553f464f04a94db60cb68fa751 Mon Sep 17 00:00:00 2001 From: Maxim Pimenov Date: Wed, 19 Dec 2018 15:38:36 +0300 Subject: [PATCH] [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 | 23 ++++++---- coding/reader.cpp | 9 +++- coding/reader.hpp | 6 +++ generator/mwm_diff/diff.cpp | 9 +++- .../mwm_diff/mwm_diff_tests/diff_test.cpp | 43 +++++++++++++++++++ 5 files changed, 80 insertions(+), 10 deletions(-) diff --git a/base/logging.hpp b/base/logging.hpp index c33dd7d299..de74773a21 100644 --- a/base/logging.hpp +++ b/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 diff --git a/coding/reader.cpp b/coding/reader.cpp index 6b94af3bc7..b7f4a1c59d 100644 --- a/coding/reader.cpp +++ b/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 Reader::Contents() const +{ + vector 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) diff --git a/coding/reader.hpp b/coding/reader.hpp index fb3546a717..5ff276ccc1 100644 --- a/coding/reader.hpp +++ b/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 Contents() const; + static bool IsEqual(string const & name1, string const & name2); }; diff --git a/generator/mwm_diff/diff.cpp b/generator/mwm_diff/diff.cpp index ca9ecc8bdc..38b60a69d9 100644 --- a/generator/mwm_diff/diff.cpp +++ b/generator/mwm_diff/diff.cpp @@ -64,7 +64,14 @@ bool ApplyDiffVersion0(FileReader & oldReader, FileWriter & newWriter, vector 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); diff --git a/generator/mwm_diff/mwm_diff_tests/diff_test.cpp b/generator/mwm_diff/mwm_diff_tests/diff_test.cpp index b16354d3b2..7011163a81 100644 --- a/generator/mwm_diff/mwm_diff_tests/diff_test.cpp +++ b/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 +#include +#include + 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 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(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 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