Commit 1b1153fc authored by Samuel Huang's avatar Samuel Huang Committed by Commit Bot

[Zucchini] Fix BufferViewBase::covers_array() to allow 0-sized array at end of buffer.

BufferViewBase::covers_array(offset, num, elt_size) decides whether a
buffer at |offset| can fit an array with |num| elements, each with
|elt_size|. A special case is covers_array(size(), 0, elt_size), i.e.,
can we fit a empty array at end of the buffer?

Previously this was considered to be a pathological case, so the result
is "false". However, recently it's revealed that this causes some valid
DEX files to rejected!

What happens is that ParseAnnotationDirectoryItem() parses data that
look like (in regex) "(AF*M*P*)*", where "AF*M*P*" is a block with
header "A" with counts for structs "F", "M", "P", followed by the
specified number of these structs. The parsing code uses covers_array()
to check for buffer overrun. However, for the case where the last
"AF*M*P*" block has 0 "P" blocks, we'd encounter the special case
covers_array(size(), 0, elt_size), and the resulting "false"
invalidates the DEX file.

The fix is to make the special case return "true". Note that this only
affects DEX (which is currently the only user of covers_array()).

Change-Id: I2939194f7e91739193e1558361aeb9617bf9c023
Reviewed-on: https://chromium-review.googlesource.com/1133688Reviewed-by: default avatarSamuel Huang <huangs@chromium.org>
Reviewed-by: default avataragrieve <agrieve@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574279}
parent c6971f08
......@@ -110,7 +110,7 @@ class BufferViewBase {
bool covers_array(size_t offset, size_t num, size_t elt_size) {
DCHECK_GT(elt_size, 0U);
// Use subtraction and division to avoid overflow.
return offset < size() && (size() - offset) / elt_size >= num;
return offset <= size() && (size() - offset) / elt_size >= num;
}
// Element access
......
......@@ -194,10 +194,10 @@ TEST_F(BufferViewTest, CoversArray) {
EXPECT_TRUE(view.covers_array(0, 0, bytes_.size()));
EXPECT_TRUE(view.covers_array(bytes_.size() - 1, 0, bytes_.size()));
EXPECT_FALSE(view.covers_array(bytes_.size(), 0, bytes_.size()));
EXPECT_TRUE(view.covers_array(bytes_.size(), 0, bytes_.size()));
EXPECT_TRUE(view.covers_array(0, 0, 0x10000));
EXPECT_TRUE(view.covers_array(bytes_.size() - 1, 0, 0x10000));
EXPECT_FALSE(view.covers_array(bytes_.size(), 0, 0x10000));
EXPECT_TRUE(view.covers_array(bytes_.size(), 0, 0x10000));
EXPECT_FALSE(view.covers_array(0, 1, bytes_.size() + 1));
EXPECT_FALSE(view.covers_array(0, 2, bytes_.size()));
......@@ -206,7 +206,7 @@ TEST_F(BufferViewTest, CoversArray) {
EXPECT_FALSE(view.covers_array(1, bytes_.size(), 1));
EXPECT_FALSE(view.covers_array(bytes_.size(), 1, 1));
EXPECT_FALSE(view.covers_array(bytes_.size(), 0, 1));
EXPECT_TRUE(view.covers_array(bytes_.size(), 0, 1));
EXPECT_FALSE(view.covers_array(0, 0x10000, 0x10000));
}
......
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