From 6798315327690fdbe93add15159be5b925779bfe Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Sat, 4 Nov 2017 00:08:08 -0700 Subject: otautil: Remove the aborts in RangeSet::Parse(). We used to CHECK and abort on parsing errors. While it works fine for the updater use case (because recovery starts updater in a forked process and collects the process exit code), it's difficult for other clients to use RangeSet as a library (e.g. update_verifier). This CL switches the aborts to returning empty RangeSet instead. Callers need to check the parsing results explicitly. The CL also separates RangeSet::PushBack() into a function, and moves SortedRangeSet::Clear() into RangeSet. Test: recovery_unit_test Test: Sideload an OTA package with the new updater on angler. Test: Sideload an OTA package with injected range string errors. The updater aborts from the explicit checks. Change-Id: If2b7f6f41dc93af917a21c7877a83e98dc3fd016 --- otautil/include/otautil/rangeset.h | 42 ++++++++++++++----- otautil/rangeset.cpp | 85 +++++++++++++++++++++++++------------- tests/unit/rangeset_test.cpp | 78 +++++++++++++++++++++++++++------- updater/blockimg.cpp | 20 ++++++++- 4 files changed, 170 insertions(+), 55 deletions(-) diff --git a/otautil/include/otautil/rangeset.h b/otautil/include/otautil/rangeset.h index c4234d181..af8ae2dee 100644 --- a/otautil/include/otautil/rangeset.h +++ b/otautil/include/otautil/rangeset.h @@ -30,28 +30,35 @@ class RangeSet { explicit RangeSet(std::vector&& pairs); + // Parses the given string into a RangeSet. Returns the parsed RangeSet, or an empty RangeSet on + // errors. static RangeSet Parse(const std::string& range_text); + // Appends the given Range to the current RangeSet. + bool PushBack(Range range); + + // Clears all the ranges from the RangeSet. + void Clear(); + std::string ToString() const; - // Get the block number for the i-th (starting from 0) block in the RangeSet. + // Gets the block number for the i-th (starting from 0) block in the RangeSet. size_t GetBlockNumber(size_t idx) const; - // RangeSet has half-closed half-open bounds. For example, "3,5" contains blocks 3 and 4. So "3,5" - // and "5,7" are not overlapped. + // Returns whether the current RangeSet overlaps with other. RangeSet has half-closed half-open + // bounds. For example, "3,5" contains blocks 3 and 4. So "3,5" and "5,7" are not overlapped. bool Overlaps(const RangeSet& other) const; - // size() gives the number of Range's in this RangeSet. + // Returns the number of Range's in this RangeSet. size_t size() const { return ranges_.size(); } - // blocks() gives the number of all blocks in this RangeSet. + // Returns the total number of blocks in this RangeSet. size_t blocks() const { return blocks_; } - // We provide const iterators only. std::vector::const_iterator cbegin() const { return ranges_.cbegin(); } @@ -60,13 +67,20 @@ class RangeSet { return ranges_.cend(); } - // Need to provide begin()/end() since range-based loop expects begin()/end(). + std::vector::iterator begin() { + return ranges_.begin(); + } + + std::vector::iterator end() { + return ranges_.end(); + } + std::vector::const_iterator begin() const { - return ranges_.cbegin(); + return ranges_.begin(); } std::vector::const_iterator end() const { - return ranges_.cend(); + return ranges_.end(); } // Reverse const iterators for MoveRange(). @@ -78,6 +92,11 @@ class RangeSet { return ranges_.crend(); } + // Returns whether the RangeSet is valid (i.e. non-empty). + explicit operator bool() const { + return !ranges_.empty(); + } + const Range& operator[](size_t i) const { return ranges_[i]; } @@ -109,6 +128,9 @@ class RangeSet { // every block in the original source. class SortedRangeSet : public RangeSet { public: + // The block size when working with offset and file length. + static constexpr size_t kBlockSize = 4096; + SortedRangeSet() {} // Ranges in the the set should be mutually exclusive; and they're sorted by the start block. @@ -122,8 +144,6 @@ class SortedRangeSet : public RangeSet { // Compute the block range the file occupies, and insert that range. void Insert(size_t start, size_t len); - void Clear(); - using RangeSet::Overlaps; bool Overlaps(size_t start, size_t len) const; diff --git a/otautil/rangeset.cpp b/otautil/rangeset.cpp index a121a4efc..532cba4a8 100644 --- a/otautil/rangeset.cpp +++ b/otautil/rangeset.cpp @@ -16,8 +16,10 @@ #include "otautil/rangeset.h" +#include #include +#include #include #include #include @@ -28,47 +30,79 @@ #include RangeSet::RangeSet(std::vector&& pairs) { - CHECK_NE(pairs.size(), static_cast(0)) << "Invalid number of tokens"; + blocks_ = 0; + if (pairs.empty()) { + LOG(ERROR) << "Invalid number of tokens"; + return; + } - // Sanity check the input. - size_t result = 0; for (const auto& range : pairs) { - CHECK_LT(range.first, range.second) << "Empty or negative range: " << range.first << ", " - << range.second; - size_t sz = range.second - range.first; - CHECK_LE(result, SIZE_MAX - sz) << "RangeSet size overflow"; - result += sz; + if (!PushBack(range)) { + Clear(); + return; + } } - - ranges_ = pairs; - blocks_ = result; } RangeSet RangeSet::Parse(const std::string& range_text) { std::vector pieces = android::base::Split(range_text, ","); - CHECK_GE(pieces.size(), static_cast(3)) << "Invalid range text: " << range_text; + if (pieces.size() < 3) { + LOG(ERROR) << "Invalid range text: " << range_text; + return {}; + } size_t num; - CHECK(android::base::ParseUint(pieces[0], &num, static_cast(INT_MAX))) - << "Failed to parse the number of tokens: " << range_text; - - CHECK_NE(num, static_cast(0)) << "Invalid number of tokens: " << range_text; - CHECK_EQ(num % 2, static_cast(0)) << "Number of tokens must be even: " << range_text; - CHECK_EQ(num, pieces.size() - 1) << "Mismatching number of tokens: " << range_text; + if (!android::base::ParseUint(pieces[0], &num, static_cast(INT_MAX))) { + LOG(ERROR) << "Failed to parse the number of tokens: " << range_text; + return {}; + } + if (num == 0) { + LOG(ERROR) << "Invalid number of tokens: " << range_text; + return {}; + } + if (num % 2 != 0) { + LOG(ERROR) << "Number of tokens must be even: " << range_text; + return {}; + } + if (num != pieces.size() - 1) { + LOG(ERROR) << "Mismatching number of tokens: " << range_text; + return {}; + } std::vector pairs; for (size_t i = 0; i < num; i += 2) { size_t first; - CHECK(android::base::ParseUint(pieces[i + 1], &first, static_cast(INT_MAX))); size_t second; - CHECK(android::base::ParseUint(pieces[i + 2], &second, static_cast(INT_MAX))); - + if (!android::base::ParseUint(pieces[i + 1], &first, static_cast(INT_MAX)) || + !android::base::ParseUint(pieces[i + 2], &second, static_cast(INT_MAX))) { + return {}; + } pairs.emplace_back(first, second); } - return RangeSet(std::move(pairs)); } +bool RangeSet::PushBack(Range range) { + if (range.first >= range.second) { + LOG(ERROR) << "Empty or negative range: " << range.first << ", " << range.second; + return false; + } + size_t sz = range.second - range.first; + if (blocks_ >= SIZE_MAX - sz) { + LOG(ERROR) << "RangeSet size overflow"; + return false; + } + + ranges_.push_back(std::move(range)); + blocks_ += sz; + return true; +} + +void RangeSet::Clear() { + ranges_.clear(); + blocks_ = 0; +} + std::string RangeSet::ToString() const { if (ranges_.empty()) { return ""; @@ -114,8 +148,6 @@ bool RangeSet::Overlaps(const RangeSet& other) const { return false; } -static constexpr size_t kBlockSize = 4096; - // Ranges in the the set should be mutually exclusive; and they're sorted by the start block. SortedRangeSet::SortedRangeSet(std::vector&& pairs) : RangeSet(std::move(pairs)) { std::sort(ranges_.begin(), ranges_.end()); @@ -158,11 +190,6 @@ void SortedRangeSet::Insert(size_t start, size_t len) { Insert(to_insert); } -void SortedRangeSet::Clear() { - blocks_ = 0; - ranges_.clear(); -} - bool SortedRangeSet::Overlaps(size_t start, size_t len) const { RangeSet rs({ { start / kBlockSize, (start + len - 1) / kBlockSize + 1 } }); return Overlaps(rs); diff --git a/tests/unit/rangeset_test.cpp b/tests/unit/rangeset_test.cpp index b3ed99215..5141bb67f 100644 --- a/tests/unit/rangeset_test.cpp +++ b/tests/unit/rangeset_test.cpp @@ -17,12 +17,24 @@ #include #include +#include #include #include #include "otautil/rangeset.h" +TEST(RangeSetTest, ctor) { + RangeSet rs(std::vector{ Range{ 8, 10 }, Range{ 1, 5 } }); + ASSERT_TRUE(rs); + + RangeSet rs2(std::vector{}); + ASSERT_FALSE(rs2); + + RangeSet rs3(std::vector{ Range{ 8, 10 }, Range{ 5, 1 } }); + ASSERT_FALSE(rs3); +} + TEST(RangeSetTest, Parse_smoke) { RangeSet rs = RangeSet::Parse("2,1,10"); ASSERT_EQ(static_cast(1), rs.size()); @@ -37,27 +49,64 @@ TEST(RangeSetTest, Parse_smoke) { // Leading zeros are fine. But android::base::ParseUint() doesn't like trailing zeros like "10 ". ASSERT_EQ(rs, RangeSet::Parse(" 2, 1, 10")); - ASSERT_EXIT(RangeSet::Parse("2,1,10 "), ::testing::KilledBySignal(SIGABRT), ""); + ASSERT_FALSE(RangeSet::Parse("2,1,10 ")); } TEST(RangeSetTest, Parse_InvalidCases) { // Insufficient number of tokens. - ASSERT_EXIT(RangeSet::Parse(""), ::testing::KilledBySignal(SIGABRT), ""); - ASSERT_EXIT(RangeSet::Parse("2,1"), ::testing::KilledBySignal(SIGABRT), ""); + ASSERT_FALSE(RangeSet::Parse("")); + ASSERT_FALSE(RangeSet::Parse("2,1")); // The first token (i.e. the number of following tokens) is invalid. - ASSERT_EXIT(RangeSet::Parse("a,1,1"), ::testing::KilledBySignal(SIGABRT), ""); - ASSERT_EXIT(RangeSet::Parse("3,1,1"), ::testing::KilledBySignal(SIGABRT), ""); - ASSERT_EXIT(RangeSet::Parse("-3,1,1"), ::testing::KilledBySignal(SIGABRT), ""); - ASSERT_EXIT(RangeSet::Parse("2,1,2,3"), ::testing::KilledBySignal(SIGABRT), ""); + ASSERT_FALSE(RangeSet::Parse("a,1,1")); + ASSERT_FALSE(RangeSet::Parse("3,1,1")); + ASSERT_FALSE(RangeSet::Parse("-3,1,1")); + ASSERT_FALSE(RangeSet::Parse("2,1,2,3")); // Invalid tokens. - ASSERT_EXIT(RangeSet::Parse("2,1,10a"), ::testing::KilledBySignal(SIGABRT), ""); - ASSERT_EXIT(RangeSet::Parse("2,,10"), ::testing::KilledBySignal(SIGABRT), ""); + ASSERT_FALSE(RangeSet::Parse("2,1,10a")); + ASSERT_FALSE(RangeSet::Parse("2,,10")); // Empty or negative range. - ASSERT_EXIT(RangeSet::Parse("2,2,2"), ::testing::KilledBySignal(SIGABRT), ""); - ASSERT_EXIT(RangeSet::Parse("2,2,1"), ::testing::KilledBySignal(SIGABRT), ""); + ASSERT_FALSE(RangeSet::Parse("2,2,2")); + ASSERT_FALSE(RangeSet::Parse("2,2,1")); +} + +TEST(RangeSetTest, Clear) { + RangeSet rs = RangeSet::Parse("2,1,6"); + ASSERT_TRUE(rs); + rs.Clear(); + ASSERT_FALSE(rs); + + // No-op to clear an empty RangeSet. + rs.Clear(); + ASSERT_FALSE(rs); +} + +TEST(RangeSetTest, PushBack) { + RangeSet rs; + ASSERT_FALSE(rs); + + ASSERT_TRUE(rs.PushBack({ 3, 5 })); + ASSERT_EQ(RangeSet::Parse("2,3,5"), rs); + + ASSERT_TRUE(rs.PushBack({ 5, 15 })); + ASSERT_EQ(RangeSet::Parse("4,3,5,5,15"), rs); + ASSERT_EQ(static_cast(2), rs.size()); + ASSERT_EQ(static_cast(12), rs.blocks()); +} + +TEST(RangeSetTest, PushBack_InvalidInput) { + RangeSet rs; + ASSERT_FALSE(rs); + ASSERT_FALSE(rs.PushBack({ 5, 3 })); + ASSERT_FALSE(rs); + ASSERT_FALSE(rs.PushBack({ 15, 15 })); + ASSERT_FALSE(rs); + + ASSERT_TRUE(rs.PushBack({ 5, 15 })); + ASSERT_FALSE(rs.PushBack({ 5, std::numeric_limits::max() - 2 })); + ASSERT_EQ(RangeSet::Parse("2,5,15"), rs); } TEST(RangeSetTest, Overlaps) { @@ -90,7 +139,7 @@ TEST(RangeSetTest, equality) { ASSERT_NE(RangeSet::Parse("2,1,6"), RangeSet::Parse("2,1,7")); ASSERT_NE(RangeSet::Parse("2,1,6"), RangeSet::Parse("2,2,7")); - // The orders of Range's matter. "4,1,5,8,10" != "4,8,10,1,5". + // The orders of Range's matter, e.g. "4,1,5,8,10" != "4,8,10,1,5". ASSERT_NE(RangeSet::Parse("4,1,5,8,10"), RangeSet::Parse("4,8,10,1,5")); } @@ -111,13 +160,14 @@ TEST(RangeSetTest, iterators) { ASSERT_EQ((std::vector{ Range{ 8, 10 }, Range{ 1, 5 } }), ranges); } -TEST(RangeSetTest, tostring) { +TEST(RangeSetTest, ToString) { + ASSERT_EQ("", RangeSet::Parse("").ToString()); ASSERT_EQ("2,1,6", RangeSet::Parse("2,1,6").ToString()); ASSERT_EQ("4,1,5,8,10", RangeSet::Parse("4,1,5,8,10").ToString()); ASSERT_EQ("6,1,3,4,6,15,22", RangeSet::Parse("6,1,3,4,6,15,22").ToString()); } -TEST(SortedRangeSetTest, insertion) { +TEST(SortedRangeSetTest, Insert) { SortedRangeSet rs({ { 2, 3 }, { 4, 6 }, { 8, 14 } }); rs.Insert({ 1, 2 }); ASSERT_EQ(SortedRangeSet({ { 1, 3 }, { 4, 6 }, { 8, 14 } }), rs); diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp index 6c7b3efcf..1c931afef 100644 --- a/updater/blockimg.cpp +++ b/updater/blockimg.cpp @@ -492,6 +492,10 @@ static void PrintHashForCorruptedSourceBlocks(const CommandParameters& params, } RangeSet src = RangeSet::Parse(params.tokens[pos++]); + if (!src) { + LOG(ERROR) << "Failed to parse range in " << params.cmdline; + return; + } RangeSet locs; // If there's no stashed blocks, content in the buffer is consecutive and has the same @@ -936,6 +940,7 @@ static int LoadSourceBlocks(CommandParameters& params, const RangeSet& tgt, size params.cpos++; } else { RangeSet src = RangeSet::Parse(params.tokens[params.cpos++]); + CHECK(static_cast(src)); *overlap = src.Overlaps(tgt); if (ReadBlocks(src, params.buffer, params.fd) == -1) { @@ -948,6 +953,7 @@ static int LoadSourceBlocks(CommandParameters& params, const RangeSet& tgt, size } RangeSet locs = RangeSet::Parse(params.tokens[params.cpos++]); + CHECK(static_cast(locs)); MoveRange(params.buffer, locs, params.buffer); } @@ -970,6 +976,7 @@ static int LoadSourceBlocks(CommandParameters& params, const RangeSet& tgt, size } RangeSet locs = RangeSet::Parse(tokens[1]); + CHECK(static_cast(locs)); MoveRange(params.buffer, locs, stash); } @@ -1034,6 +1041,7 @@ static int LoadSrcTgtVersion3(CommandParameters& params, RangeSet& tgt, size_t* // tgt = RangeSet::Parse(params.tokens[params.cpos++]); + CHECK(static_cast(tgt)); std::vector tgtbuffer(tgt.blocks() * BLOCKSIZE); if (ReadBlocks(tgt, tgtbuffer, params.fd) == -1) { @@ -1146,6 +1154,7 @@ static int PerformCommandStash(CommandParameters& params) { } RangeSet src = RangeSet::Parse(params.tokens[params.cpos++]); + CHECK(static_cast(src)); allocate(src.blocks() * BLOCKSIZE, params.buffer); if (ReadBlocks(src, params.buffer, params.fd) == -1) { @@ -1196,6 +1205,7 @@ static int PerformCommandZero(CommandParameters& params) { } RangeSet tgt = RangeSet::Parse(params.tokens[params.cpos++]); + CHECK(static_cast(tgt)); LOG(INFO) << " zeroing " << tgt.blocks() << " blocks"; @@ -1238,6 +1248,7 @@ static int PerformCommandNew(CommandParameters& params) { } RangeSet tgt = RangeSet::Parse(params.tokens[params.cpos++]); + CHECK(static_cast(tgt)); if (params.canwrite) { LOG(INFO) << " writing " << tgt.blocks() << " blocks of new data"; @@ -1368,6 +1379,7 @@ static int PerformCommandErase(CommandParameters& params) { } RangeSet tgt = RangeSet::Parse(params.tokens[params.cpos++]); + CHECK(static_cast(tgt)); if (params.canwrite) { LOG(INFO) << " erasing " << tgt.blocks() << " blocks"; @@ -1773,6 +1785,7 @@ Value* RangeSha1Fn(const char* name, State* state, const std::vectordata); + CHECK(static_cast(rs)); SHA_CTX ctx; SHA1_Init(&ctx); @@ -1884,6 +1897,11 @@ Value* BlockImageRecoverFn(const char* name, State* state, ErrorAbort(state, kArgsParsingFailure, "ranges argument to %s must be string", name); return StringValue(""); } + RangeSet rs = RangeSet::Parse(ranges->data); + if (!rs) { + ErrorAbort(state, kArgsParsingFailure, "failed to parse ranges: %s", ranges->data.c_str()); + return StringValue(""); + } // Output notice to log when recover is attempted LOG(INFO) << filename->data << " image corrupted, attempting to recover..."; @@ -1909,7 +1927,7 @@ Value* BlockImageRecoverFn(const char* name, State* state, } uint8_t buffer[BLOCKSIZE]; - for (const auto& range : RangeSet::Parse(ranges->data)) { + for (const auto& range : rs) { for (size_t j = range.first; j < range.second; ++j) { // Stay within the data area, libfec validates and corrects metadata if (status.data_size <= static_cast(j) * BLOCKSIZE) { -- cgit v1.2.3