Revert of https://codereview.chromium.org/80403002/

Reason for revert: Breaks Aura

TBR=jln@chromium.org,aedla@chromium.org
NOTREECHECKS=true
NOTRY=true

Review URL: https://codereview.chromium.org/87533003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237293 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 72 insertions, 127 deletions

diff --git a/ui/base/clipboard/clipboard.cc b/ui/base/clipboard/clipboard.cc
index a77efd6..0e25342 100644
--- a/ui/base/clipboard/clipboard.cc
+++ b/ui/base/clipboard/clipboard.cc
@@ -18,12 +18,61 @@ namespace ui {
 
 namespace {
 
+// Extracts the bitmap size from the passed in params. Since the renderer could
+// send us bad data, explicitly copy the parameters to make sure we go through
+// gfx::Size's sanity checks.
+bool GetBitmapSizeFromParams(const Clipboard::ObjectMapParams& params,
+                             gfx::Size* size) {
+  DCHECK(params.size() == 2);
+  if (params[1].size() != sizeof(gfx::Size))
+    return false;
+  const gfx::Size* size_from_renderer =
+      reinterpret_cast<const gfx::Size*>(&(params[1].front()));
+  size->set_width(size_from_renderer->width());
+  size->set_height(size_from_renderer->height());
+  return true;
+}
+
+// A compromised renderer could send us bad size data, so validate it to verify
+// that calculating the number of bytes in the bitmap won't overflow a uint32.
+//
+// |size| - Clipboard bitmap size to validate.
+// |bitmap_bytes| - On return contains the number of bytes needed to store
+// the bitmap data or -1 if the data is invalid.
+// returns: true if the bitmap size is valid, false otherwise.
+bool IsBitmapSizeSane(const gfx::Size& size, uint32* bitmap_bytes) {
+  DCHECK_GE(size.width(), 0);
+  DCHECK_GE(size.height(), 0);
+
+  *bitmap_bytes = -1;
+  uint32 total_size = size.width();
+  // Limit size to max int so SkBitmap::getSize() calculation won't overflow.
+  // TODO(dcheng): Instead of all this custom validation, we should just use a
+  // combination of SkBitmap's setConfig() and getSize64() to detect this, e.g.:
+  //   SkBitmap bitmap;
+  //   if (!bitmap.setConfig(...))
+  //      return false;
+  //   if (bitmap.getSize64().is64())
+  //      return false;
+  if (std::numeric_limits<int>::max() / size.width() <= size.height())
+    return false;
+  total_size *= size.height();
+  if (std::numeric_limits<int>::max() / total_size <= 4)
+    return false;
+  total_size *= 4;
+  *bitmap_bytes = total_size;
+  return true;
+}
+
 // Valides a shared bitmap on the clipboard.
 // Returns true if the clipboard data makes sense and it's safe to access the
 // bitmap.
-bool ValidateAndMapSharedBitmap(size_t bitmap_bytes,
+bool ValidateAndMapSharedBitmap(const gfx::Size& size,
                                 base::SharedMemory* bitmap_data) {
   using base::SharedMemory;
+  uint32 bitmap_bytes = -1;
+  if (!IsBitmapSizeSane(size, &bitmap_bytes))
+    return false;
 
   if (!bitmap_data || !SharedMemory::IsHandleValid(bitmap_data->handle()))
     return false;
@@ -35,6 +84,19 @@ bool ValidateAndMapSharedBitmap(size_t bitmap_bytes,
   return true;
 }
 
+// Adopts a blob of bytes (assumed to be ARGB pixels) into a SkBitmap. Since the
+// pixel data is not copied, the caller must ensure that |data| is valid as long
+// as the SkBitmap is in use. Note that on little endian machines, each pixel is
+// actually BGRA in memory.
+SkBitmap AdoptBytesIntoSkBitmap(const void* data, const gfx::Size& size) {
+  SkBitmap bitmap;
+  bitmap.setConfig(SkBitmap::kARGB_8888_Config, size.width(), size.height());
+  // Guaranteed not to overflow since it's been validated by IsBitmapSizeSane().
+  DCHECK_EQ(4U * size.width(), bitmap.rowBytes());
+  bitmap.setPixels(const_cast<void*>(data));
+  return bitmap;
+}
+
 // A list of allowed threads. By default, this is empty and no thread checking
 // is done (in the unit test case), but a user (like content) can set which
 // threads are allowed to call this method.
@@ -146,38 +208,26 @@ void Clipboard::DispatchObject(ObjectType type, const ObjectMapParams& params) {
       using base::SharedMemory;
       using base::SharedMemoryHandle;
 
-      if (params[0].size() != sizeof(SharedMemory*) ||
-          params[1].size() != sizeof(gfx::Size)) {
-        return;
-      }
-
-      SkBitmap bitmap;
-      const gfx::Size* unvalidated_size =
-          reinterpret_cast<const gfx::Size*>(&params[1].front());
-      // Let Skia do some sanity checking for us (no negative widths/heights, no
-      // overflows while calculating bytes per row, etc).
-      if (!bitmap.setConfig(SkBitmap::kARGB_8888_Config,
-                            unvalidated_size->width(),
-                            unvalidated_size->height())) {
-        return;
-      }
-      // Make sure the size is representable as a signed 32-bit int, so
-      // SkBitmap::getSize() won't be truncated.
-      if (bitmap.getSize64().is64())
+      if (params[0].size() != sizeof(SharedMemory*))
         return;
 
       // It's OK to cast away constness here since we map the handle as
       // read-only.
       const char* raw_bitmap_data_const =
-          reinterpret_cast<const char*>(&params[0].front());
+          reinterpret_cast<const char*>(&(params[0].front()));
       char* raw_bitmap_data = const_cast<char*>(raw_bitmap_data_const);
       scoped_ptr<SharedMemory> bitmap_data(
           *reinterpret_cast<SharedMemory**>(raw_bitmap_data));
 
-      if (!ValidateAndMapSharedBitmap(bitmap.getSize(), bitmap_data.get()))
+      gfx::Size size;
+      if (!GetBitmapSizeFromParams(params, &size))
+        return;
+
+      if (!ValidateAndMapSharedBitmap(size, bitmap_data.get()))
         return;
-      bitmap.setPixels(bitmap_data->memory());
 
+      const SkBitmap& bitmap = AdoptBytesIntoSkBitmap(
+          bitmap_data->memory(), size);
       WriteBitmap(bitmap);
       break;
     }
diff --git a/ui/base/clipboard/clipboard_unittest.cc b/ui/base/clipboard/clipboard_unittest.cc
index 52181de..c423e41 100644
--- a/ui/base/clipboard/clipboard_unittest.cc
+++ b/ui/base/clipboard/clipboard_unittest.cc
@@ -423,111 +423,6 @@ TEST_F(ClipboardTest, SharedBitmapTest) {
   }
 }
 
-namespace {
-// A size class that just happens to have the same layout as gfx::Size!
-struct UnsafeSize {
-  int width;
-  int height;
-};
-COMPILE_ASSERT(sizeof(UnsafeSize) == sizeof(gfx::Size),
-               UnsafeSize_must_be_same_size_as_gfx_Size);
-}  // namespace
-
-TEST_F(ClipboardTest, SharedBitmapWithTwoNegativeSizes) {
-  Clipboard::ObjectMapParam placeholder_param;
-  void* crash_me = reinterpret_cast<void*>(57);
-  placeholder_param.resize(sizeof(crash_me));
-  memcpy(&placeholder_param.front(), &crash_me, sizeof(crash_me));
-
-  Clipboard::ObjectMapParam size_param;
-  UnsafeSize size = {-100, -100};
-  size_param.resize(sizeof(size));
-  memcpy(&size_param.front(), &size, sizeof(size));
-
-  Clipboard::ObjectMapParams params;
-  params.push_back(placeholder_param);
-  params.push_back(size_param);
-
-  Clipboard::ObjectMap objects;
-  objects[Clipboard::CBF_SMBITMAP] = params;
-
-  clipboard().WriteObjects(CLIPBOARD_TYPE_COPY_PASTE, objects);
-  EXPECT_FALSE(clipboard().IsFormatAvailable(Clipboard::GetBitmapFormatType(),
-                                             CLIPBOARD_TYPE_COPY_PASTE));
-}
-
-TEST_F(ClipboardTest, SharedBitmapWithOneNegativeSize) {
-  Clipboard::ObjectMapParam placeholder_param;
-  void* crash_me = reinterpret_cast<void*>(57);
-  placeholder_param.resize(sizeof(crash_me));
-  memcpy(&placeholder_param.front(), &crash_me, sizeof(crash_me));
-
-  Clipboard::ObjectMapParam size_param;
-  UnsafeSize size = {-100, 100};
-  size_param.resize(sizeof(size));
-  memcpy(&size_param.front(), &size, sizeof(size));
-
-  Clipboard::ObjectMapParams params;
-  params.push_back(placeholder_param);
-  params.push_back(size_param);
-
-  Clipboard::ObjectMap objects;
-  objects[Clipboard::CBF_SMBITMAP] = params;
-
-  clipboard().WriteObjects(CLIPBOARD_TYPE_COPY_PASTE, objects);
-  EXPECT_FALSE(clipboard().IsFormatAvailable(Clipboard::GetBitmapFormatType(),
-                                             CLIPBOARD_TYPE_COPY_PASTE));
-}
-
-TEST_F(ClipboardTest, BitmapWithSuperSize) {
-  Clipboard::ObjectMapParam placeholder_param;
-  void* crash_me = reinterpret_cast<void*>(57);
-  placeholder_param.resize(sizeof(crash_me));
-  memcpy(&placeholder_param.front(), &crash_me, sizeof(crash_me));
-
-  Clipboard::ObjectMapParam size_param;
-  // Width just big enough that bytes per row won't fit in a 32-bit
-  // representation.
-  gfx::Size size(0x20000000, 1);
-  size_param.resize(sizeof(size));
-  memcpy(&size_param.front(), &size, sizeof(size));
-
-  Clipboard::ObjectMapParams params;
-  params.push_back(placeholder_param);
-  params.push_back(size_param);
-
-  Clipboard::ObjectMap objects;
-  objects[Clipboard::CBF_SMBITMAP] = params;
-
-  clipboard().WriteObjects(CLIPBOARD_TYPE_COPY_PASTE, objects);
-  EXPECT_FALSE(clipboard().IsFormatAvailable(Clipboard::GetBitmapFormatType(),
-                                             CLIPBOARD_TYPE_COPY_PASTE));
-}
-
-TEST_F(ClipboardTest, BitmapWithSuperSize2) {
-  Clipboard::ObjectMapParam placeholder_param;
-  void* crash_me = reinterpret_cast<void*>(57);
-  placeholder_param.resize(sizeof(crash_me));
-  memcpy(&placeholder_param.front(), &crash_me, sizeof(crash_me));
-
-  Clipboard::ObjectMapParam size_param;
-  // Width and height large enough that SkBitmap::getSize() will be truncated.
-  gfx::Size size(0x0fffffff, 0x0fffffff);
-  size_param.resize(sizeof(size));
-  memcpy(&size_param.front(), &size, sizeof(size));
-
-  Clipboard::ObjectMapParams params;
-  params.push_back(placeholder_param);
-  params.push_back(size_param);
-
-  Clipboard::ObjectMap objects;
-  objects[Clipboard::CBF_SMBITMAP] = params;
-
-  clipboard().WriteObjects(CLIPBOARD_TYPE_COPY_PASTE, objects);
-  EXPECT_FALSE(clipboard().IsFormatAvailable(Clipboard::GetBitmapFormatType(),
-                                             CLIPBOARD_TYPE_COPY_PASTE));
-}
-
 TEST_F(ClipboardTest, DataTest) {
   const ui::Clipboard::FormatType kFormat =
       ui::Clipboard::GetFormatType("chromium/x-test-format");