Fix buffer overflow in DrawingDisplayItem::Equals()

We should use IntRect instead of FloatRect for the bounds of bitmap.
Previously the loop variables in BitmapEquals() exceeded the
actual bounds of the bitmap if the given FloatRect was not integral.

Bug: 779949
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Ib814cc8d3dcdcd9404654cd1f6352fb98d1e15f9
Reviewed-on: https://chromium-review.googlesource.com/746988Reviewed-by: Walter Korman <wkorman@chromium.org>
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512988}

third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.cpp

@@ -54,7 +54,7 @@ static bool RecordsEqual(sk_sp<const PaintRecord> record1,
 }
 
 static SkBitmap RecordToBitmap(sk_sp<const PaintRecord> record,
-                               const FloatRect& bounds) {
+                               const IntRect& bounds) {
   SkBitmap bitmap;
   bitmap.allocPixels(
       SkImageInfo::MakeN32Premul(bounds.Width(), bounds.Height()));
@@ -67,20 +67,22 @@ static SkBitmap RecordToBitmap(sk_sp<const PaintRecord> record,
 
 static bool BitmapsEqual(sk_sp<const PaintRecord> record1,
                          sk_sp<const PaintRecord> record2,
-                         const FloatRect& bounds) {
+                         const IntRect& bounds) {
   SkBitmap bitmap1 = RecordToBitmap(record1, bounds);
   SkBitmap bitmap2 = RecordToBitmap(record2, bounds);
   int mismatch_count = 0;
-  const int kMaxMismatches = 10;
-  for (int y = 0; y < bounds.Height() && mismatch_count < kMaxMismatches; ++y) {
-    for (int x = 0; x < bounds.Width() && mismatch_count < kMaxMismatches;
-         ++x) {
+  constexpr int kMaxMismatches = 10;
+  for (int y = 0; y < bounds.Height(); ++y) {
+    for (int x = 0; x < bounds.Width(); ++x) {
       SkColor pixel1 = bitmap1.getColor(x, y);
       SkColor pixel2 = bitmap2.getColor(x, y);
       if (pixel1 != pixel2) {
+        if (!RuntimeEnabledFeatures::PaintUnderInvalidationCheckingEnabled())
+          return false;
         LOG(ERROR) << "x=" << x << " y=" << y << " " << std::hex << pixel1
                    << " vs " << std::hex << pixel2;
-        ++mismatch_count;
+        if (++mismatch_count >= kMaxMismatches)
+          return false;
       }
     }
   }
@@ -91,28 +93,28 @@ bool DrawingDisplayItem::Equals(const DisplayItem& other) const {
   if (!DisplayItem::Equals(other))
     return false;
 
-  const sk_sp<const PaintRecord>& record = this->GetPaintRecord();
-  const FloatRect bounds(this->VisualRect());
-  const sk_sp<const PaintRecord>& other_record =
+  const auto& record = GetPaintRecord();
+  const auto& other_record =
       static_cast<const DrawingDisplayItem&>(other).GetPaintRecord();
-  const FloatRect other_bounds(other.VisualRect());
-
   if (!record && !other_record)
     return true;
   if (!record || !other_record)
     return false;
+
+  const auto& bounds = this->VisualRect();
+  const auto& other_bounds = other.VisualRect();
   if (bounds != other_bounds)
     return false;
 
-  if (RecordsEqual(record, other_record, bounds))
+  if (RecordsEqual(record, other_record, FloatRect(bounds)))
     return true;
 
   // Sometimes the client may produce different records for the same visual
   // result, which should be treated as equal.
-  return BitmapsEqual(
-      std::move(record), std::move(other_record),
-      // Limit the bounds to prevent OOM.
-      Intersection(bounds, FloatRect(bounds.X(), bounds.Y(), 6000, 6000)));
+  IntRect int_bounds = EnclosingIntRect(bounds);
+  // Limit the bounds to prevent OOM.
+  int_bounds.Intersect(IntRect(int_bounds.X(), int_bounds.Y(), 6000, 6000));
+  return BitmapsEqual(std::move(record), std::move(other_record), int_bounds);
 }
 
 }  // namespace blink

third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItem.h

@@ -7,7 +7,6 @@
 
 #include "base/compiler_specific.h"
 #include "platform/PlatformExport.h"
-#include "platform/geometry/FloatRect.h"
 #include "platform/graphics/paint/DisplayItem.h"
 #include "platform/graphics/paint/PaintRecord.h"
 #include "platform/runtime_enabled_features.h"
@@ -51,11 +50,12 @@ class PLATFORM_EXPORT DrawingDisplayItem final : public DisplayItem {
     return known_to_be_opaque_;
   }
 
+  bool Equals(const DisplayItem& other) const final;
+
  private:
 #ifndef NDEBUG
   void PropertiesAsJSON(JSONObject&) const override;
 #endif
-  bool Equals(const DisplayItem& other) const final;
 
   sk_sp<const PaintRecord> record_;
 

third_party/WebKit/Source/platform/graphics/paint/DrawingDisplayItemTest.cpp

@@ -36,6 +36,20 @@ static sk_sp<PaintRecord> CreateRectRecord(const FloatRect& record_bounds) {
   return recorder.finishRecordingAsPicture();
 }
 
+static sk_sp<PaintRecord> CreateRectRecordWithTranslate(
+    const FloatRect& record_bounds,
+    float dx,
+    float dy) {
+  PaintRecorder recorder;
+  PaintCanvas* canvas =
+      recorder.beginRecording(record_bounds.Width(), record_bounds.Height());
+  canvas->save();
+  canvas->translate(dx, dy);
+  canvas->drawRect(record_bounds, PaintFlags());
+  canvas->restore();
+  return recorder.finishRecordingAsPicture();
+}
+
 TEST_F(DrawingDisplayItemTest, VisualRectAndDrawingBounds) {
   FloatRect record_bounds(5.5, 6.6, 7.7, 8.8);
   LayoutRect drawing_bounds(record_bounds);
@@ -59,5 +73,57 @@ TEST_F(DrawingDisplayItemTest, VisualRectAndDrawingBounds) {
   item.AppendToWebDisplayItemList(offset, &list2);
 }
 
+TEST_F(DrawingDisplayItemTest, Equals) {
+  FloatRect bounds1(100.1, 100.2, 100.3, 100.4);
+  client_.SetVisualRect(LayoutRect(bounds1));
+  DrawingDisplayItem item1(client_, DisplayItem::kDocumentBackground,
+                           CreateRectRecord(bounds1));
+  DrawingDisplayItem translated(client_, DisplayItem::kDocumentBackground,
+                                CreateRectRecordWithTranslate(bounds1, 10, 20));
+  // This item contains a DrawingRecord that is different from but visually
+  // equivalent to item1's.
+  DrawingDisplayItem zero_translated(
+      client_, DisplayItem::kDocumentBackground,
+      CreateRectRecordWithTranslate(bounds1, 0, 0));
+
+  FloatRect bounds2(100.5, 100.6, 100.7, 100.8);
+  client_.SetVisualRect(LayoutRect(bounds2));
+  DrawingDisplayItem item2(client_, DisplayItem::kDocumentBackground,
+                           CreateRectRecord(bounds2));
+
+  DrawingDisplayItem empty_item(client_, DisplayItem::kDocumentBackground,
+                                nullptr);
+
+  EXPECT_TRUE(item1.Equals(item1));
+  EXPECT_FALSE(item1.Equals(item2));
+  EXPECT_FALSE(item1.Equals(translated));
+  EXPECT_TRUE(item1.Equals(zero_translated));
+  EXPECT_FALSE(item1.Equals(empty_item));
+
+  EXPECT_FALSE(item2.Equals(item1));
+  EXPECT_TRUE(item2.Equals(item2));
+  EXPECT_FALSE(item2.Equals(translated));
+  EXPECT_FALSE(item2.Equals(zero_translated));
+  EXPECT_FALSE(item2.Equals(empty_item));
+
+  EXPECT_FALSE(translated.Equals(item1));
+  EXPECT_FALSE(translated.Equals(item2));
+  EXPECT_TRUE(translated.Equals(translated));
+  EXPECT_FALSE(translated.Equals(zero_translated));
+  EXPECT_FALSE(translated.Equals(empty_item));
+
+  EXPECT_TRUE(zero_translated.Equals(item1));
+  EXPECT_FALSE(zero_translated.Equals(item2));
+  EXPECT_FALSE(zero_translated.Equals(translated));
+  EXPECT_TRUE(zero_translated.Equals(zero_translated));
+  EXPECT_FALSE(zero_translated.Equals(empty_item));
+
+  EXPECT_FALSE(empty_item.Equals(item1));
+  EXPECT_FALSE(empty_item.Equals(item2));
+  EXPECT_FALSE(empty_item.Equals(translated));
+  EXPECT_FALSE(empty_item.Equals(zero_translated));
+  EXPECT_TRUE(empty_item.Equals(empty_item));
+}
+
 }  // namespace
 }  // namespace blink