From 673bf5912068218539f524b6dfe74dfb38fe67b8 Mon Sep 17 00:00:00 2001 From: Calder Kitagawa Date: Fri, 27 Apr 2018 22:37:45 +0000 Subject: [Zucchini] Validate equivalences on load A follow-up to https://chromium-review.googlesource.com/c/chromium/src/+/1028575 This moves patch_apply logic to check bounds of an equivalences from the call site of GetNext() to an internal function in the patch_reader. This means the equivalence consumer can use the equivalences without checking anything to do with bounds. I have manually tested that this doesn't appear to break any existing valid patches and it appears to catch all the same errors change 1028575 fixed so I can safely reverse that change. BUG: 837096 Change-Id: I84ccd9e1493f32d16eace4dd8e67586f559220d3 Reviewed-on: https://chromium-review.googlesource.com/1028836 Commit-Queue: Calder Kitagawa Reviewed-by: Samuel Huang Cr-Commit-Position: refs/heads/master@{#554536} NOKEYCHECK=True GitOrigin-RevId: ce5642400b37f5ff2b0a1213522f984bca8a080a --- patch_reader.cc | 44 ++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 42 insertions(+), 2 deletions(-) (limited to 'patch_reader.cc') diff --git a/patch_reader.cc b/patch_reader.cc index 8215405..90f35f4 100644 --- a/patch_reader.cc +++ b/patch_reader.cc @@ -13,6 +13,19 @@ namespace zucchini { +namespace { + +bool RangeIsInRegion(offset_t begin, uint32_t size, BufferRegion region) { + // Return false if |begin + size > kOffsetBound|, without overflowing. + if (begin >= kOffsetBound || kOffsetBound - begin < size) + return false; + offset_t end = begin + size; + // Return false if |[begin, end]| leaks outside |region|. + return begin >= region.lo() && end <= region.hi(); +} + +} // namespace + namespace patch { bool ParseElementMatch(BufferSource* source, ElementMatch* element_match) { @@ -217,7 +230,7 @@ PatchElementReader::~PatchElementReader() = default; bool PatchElementReader::Initialize(BufferSource* source) { bool ok = patch::ParseElementMatch(source, &element_match_) && - equivalences_.Initialize(source) && + equivalences_.Initialize(source) && ValidateEquivalences() && extra_data_.Initialize(source) && raw_delta_.Initialize(source) && reference_delta_.Initialize(source); if (!ok) @@ -240,7 +253,7 @@ bool PatchElementReader::Initialize(BufferSource* source) { } auto insert_result = extra_targets_.insert({pool_tag, {}}); if (!insert_result.second) { // Element already present. - LOG(ERROR) << "Multiple ExtraTargetList found for the same pool_tag"; + LOG(ERROR) << "Multiple ExtraTargetList found for the same pool_tag."; return false; } if (!insert_result.first->second.Initialize(source)) @@ -249,6 +262,33 @@ bool PatchElementReader::Initialize(BufferSource* source) { return true; } +bool PatchElementReader::ValidateEquivalences() { + EquivalenceSource equivalences_copy = equivalences_; + + BufferRegion old_region = element_match_.old_element.region(); + BufferRegion new_region = element_match_.new_element.region(); + + // Validate that each |equivalence| falls within the bounds of the + // |element_match_| and are in order. + offset_t prev_dst_end = 0; + for (auto equivalence = equivalences_copy.GetNext(); equivalence.has_value(); + equivalence = equivalences_copy.GetNext()) { + if (!RangeIsInRegion(equivalence->src_offset, equivalence->length, + old_region) || + !RangeIsInRegion(equivalence->dst_offset, equivalence->length, + new_region)) { + LOG(ERROR) << "Out of bounds equivalence detected."; + return false; + } + if (prev_dst_end > equivalence->dst_end()) { + LOG(ERROR) << "Out of order equivalence detected."; + return false; + } + prev_dst_end = equivalence->dst_end(); + } + return true; +} + /******** EnsemblePatchReader ********/ base::Optional EnsemblePatchReader::Create( -- cgit v1.2.3