commit d758cb7bfc3f9394f8e96a8e14d01af7b0c388b4
author Samuel Huang <huangs@chromium.org> Thu Jul 19 14:32:28 2018 +0000
committer Copybara-Service <copybara-worker@google.com> Sun Jul 25 20:31:52 2021 -0700
tree 4ec9b0ff5476eef415550672ddd80675d03a9d4f
parent 06a9be1c70878cf651b70192275187bf9a8662a9

[Zucchini] Fix BufferRegion::FitsIn() so empty region fits at end of buffer.

This CL is similar to:
https://chromium-review.googlesource.com/1133688

BufferRegion::FitsIn() (and BufferViewBase::covers()) decides whether
a BufferRegion fits inside a buffer. A special case is whether an empty
region fits at the end of a buffer?

Previously this was considered to be a pathological case, so the result
is "false". However, this led to a DCHECK failure found by the DEX
fuzzer: a CodeItem with insns_size = 0 is checked against an empty
buffer.

It may seem straightforward to change the DCHECK to a handled failure.
However, the failing code (in CodeItemParser::GetCodeItemInsns())
occurs after CodeItem have been supposedly validated, so the DCHECK
is correctly placed! Two causes are:
(1) Technically insns_size should be > 0, as dictated by constraint A1
    ("The insns array mus tnot be empty") in Dalvik spec.
(2) The FitsIn() check is too stringent.

This CL focuses on relaxing (2). This makes checking slightly more
permissive elsewhere in code (patch_reader.cc and Win32 disassembler),
but this looks like the right thing to do.

As for (1), we plan to visit
https://source.android.com/devices/tech/dalvik/constraints
and implement more rigorous checks. So we simply add a TODO for now.

Bug: 863478
Change-Id: Iacbb2bb9bf26701db960192c7b727351ea5afdec
Reviewed-on: https://chromium-review.googlesource.com/1142517
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Commit-Queue: Samuel Huang <huangs@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576482}
NOKEYCHECK=True
GitOrigin-RevId: 2b31de169e783260c9e2fbaea295b39ae808fbf9

buffer_view.h

diff --git a/buffer_view.h b/buffer_view.h
index 7bdd8ec..66925c4 100644
--- a/buffer_view.h
+++ b/buffer_view.h
@@ -24,9 +24,9 @@
   size_t hi() const { return offset + size; }
 
   // Returns whether the Region fits in |[0, container_size)|. Special case:
-  // a size-0 region starting at |container_size| does not fit.
+  // a size-0 region starting at |container_size| fits.
   bool FitsIn(size_t container_size) const {
-    return offset < container_size && container_size - offset >= size;
+    return offset <= container_size && container_size - offset >= size;
   }
 
   // Returns |v| clipped to the inclusive range |[lo(), hi()]|.

buffer_view_unittest.cc

diff --git a/buffer_view_unittest.cc b/buffer_view_unittest.cc
index b048747..7df34b2 100644
--- a/buffer_view_unittest.cc
+++ b/buffer_view_unittest.cc
@@ -154,7 +154,7 @@
 }
 
 TEST_F(BufferViewTest, Covers) {
-  EXPECT_FALSE(ConstBufferView().covers({0, 0}));
+  EXPECT_TRUE(ConstBufferView().covers({0, 0}));
   EXPECT_FALSE(ConstBufferView().covers({0, 1}));
 
   ConstBufferView view(bytes_.data(), bytes_.size());
@@ -168,8 +168,10 @@
   EXPECT_TRUE(view.covers({bytes_.size() - 1, 0}));
   EXPECT_TRUE(view.covers({bytes_.size() - 1, 1}));
   EXPECT_FALSE(view.covers({bytes_.size() - 1, 2}));
-  EXPECT_FALSE(view.covers({bytes_.size(), 0}));
+  EXPECT_TRUE(view.covers({bytes_.size(), 0}));
   EXPECT_FALSE(view.covers({bytes_.size(), 1}));
+  EXPECT_FALSE(view.covers({bytes_.size() + 1, 0}));
+  EXPECT_FALSE(view.covers({bytes_.size() + 1, 1}));
 
   EXPECT_FALSE(view.covers({1, size_t(-1)}));
   EXPECT_FALSE(view.covers({size_t(-1), 1}));

disassembler_dex.cc

diff --git a/disassembler_dex.cc b/disassembler_dex.cc
index 6ec67d1..373d645 100644
--- a/disassembler_dex.cc
+++ b/disassembler_dex.cc
@@ -162,6 +162,7 @@
       return kInvalidOffset;
     DCHECK(Is32BitAligned(code_item_offset));
 
+    // TODO(huangs): Fail if |code_item->insns_size == 0| (Constraint A1).
     // Skip instruction bytes.
     if (!source_.GetArray<uint16_t>(code_item->insns_size))
       return kInvalidOffset;