Commit 520aebd1 authored by Calder Kitagawa's avatar Calder Kitagawa Committed by Commit Bot

[Zucchini] Fix bugs found by fuzzer in Apply

There was an outstanding TODO to validate that during Apply the writes
to the |new_image| and copies from the |old_image| were within valid
ranges. During raw apply fuzzing a number of memory violations occurred
this is a fix for those.

Bug: 835341
Change-Id: I669304e93e51ba7cd2b862189fbc0a6f3cea1748
Reviewed-on: https://chromium-review.googlesource.com/1028575
Commit-Queue: Calder Kitagawa <ckitagawa@google.com>
Reviewed-by: default avatarSamuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553718}
parent 7e8aa6ab
...@@ -26,19 +26,33 @@ bool ApplyEquivalenceAndExtraData(ConstBufferView old_image, ...@@ -26,19 +26,33 @@ bool ApplyEquivalenceAndExtraData(ConstBufferView old_image,
for (auto equivalence = equiv_source.GetNext(); equivalence.has_value(); for (auto equivalence = equiv_source.GetNext(); equivalence.has_value();
equivalence = equiv_source.GetNext()) { equivalence = equiv_source.GetNext()) {
// TODO(etiennep): Guard against out of range errors and return false // TODO(ckitagawa): Ensure guards don't overflow. Move these validation
// instead. // check to the patch reader.
// Validate that the |equivalence| is within the |new_image|.
if (equivalence->dst_end() > new_image.size()) {
LOG(ERROR) << "Out of bounds equivalence";
return false;
}
MutableBufferView::iterator next_dst_it = MutableBufferView::iterator next_dst_it =
new_image.begin() + equivalence->dst_offset; new_image.begin() + equivalence->dst_offset;
CHECK(next_dst_it >= dst_it); CHECK(next_dst_it >= dst_it);
offset_t gap = static_cast<offset_t>(next_dst_it - dst_it); offset_t gap = static_cast<offset_t>(next_dst_it - dst_it);
base::Optional<ConstBufferView> extra_data = extra_data_source.GetNext(gap); base::Optional<ConstBufferView> extra_data = extra_data_source.GetNext(gap);
if (!extra_data) { if (!extra_data) {
LOG(ERROR) << "Error reading extra_data"; LOG(ERROR) << "Error reading extra_data";
return false; return false;
} }
// |extra_data| length is based on what was parsed from the patch so this
// copy should be valid.
dst_it = std::copy(extra_data->begin(), extra_data->end(), dst_it); dst_it = std::copy(extra_data->begin(), extra_data->end(), dst_it);
CHECK_EQ(dst_it, next_dst_it); CHECK_EQ(dst_it, next_dst_it);
// Validate that the |equivalence| is within the |old_image|.
if (equivalence->src_end() > old_image.size()) {
LOG(ERROR) << "Out of bounds equivalence";
return false;
}
dst_it = std::copy_n(old_image.begin() + equivalence->src_offset, dst_it = std::copy_n(old_image.begin() + equivalence->src_offset,
equivalence->length, dst_it); equivalence->length, dst_it);
CHECK_EQ(dst_it, next_dst_it + equivalence->length); CHECK_EQ(dst_it, next_dst_it + equivalence->length);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment