Move SoftwareFrameData overflow checks to the IPC code.

Instead of doing this check in SoftwareFrameManager and silently
dropping the frame, if we have an overflow, drop the IPC from the
renderer (and cause a renderer crash which we can see).

Also move computation code for the frame size in bytes to
SoftwareFrameData so the computation and the check can be beside
each other.

Also add unit tests for SoftwareFrameData IPC.

R=ccameron@chromium.org, jschuh@chromium.org, piman@chromium.org, ccameron, piman
BUG=348332

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

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

6 files changed, 145 insertions, 16 deletions

diff --git a/cc/output/software_frame_data.cc b/cc/output/software_frame_data.cc
index 0b43032..4bc0980 100644
--- a/cc/output/software_frame_data.cc
+++ b/cc/output/software_frame_data.cc
@@ -12,4 +12,17 @@ SoftwareFrameData::SoftwareFrameData()
 
 SoftwareFrameData::~SoftwareFrameData() {}
 
+size_t SoftwareFrameData::SizeInBytes() const {
+  size_t bytes_per_pixel = 4;
+  size_t width = size.width();
+  size_t height = size.height();
+  return bytes_per_pixel * width * height;
+}
+
+base::CheckedNumeric<size_t> SoftwareFrameData::CheckedSizeInBytes() const {
+  return base::CheckedNumeric<size_t>(4) *
+         base::CheckedNumeric<size_t>(size.width()) *
+         base::CheckedNumeric<size_t>(size.height());
+}
+
 }  // namespace cc
diff --git a/cc/output/software_frame_data.h b/cc/output/software_frame_data.h
index b7fb44a..8463f80 100644
--- a/cc/output/software_frame_data.h
+++ b/cc/output/software_frame_data.h
@@ -6,6 +6,7 @@
 #define CC_OUTPUT_SOFTWARE_FRAME_DATA_H_
 
 #include "base/memory/shared_memory.h"
+#include "base/numerics/safe_math.h"
 #include "cc/base/cc_export.h"
 #include "ui/gfx/rect.h"
 
@@ -20,6 +21,9 @@ class CC_EXPORT SoftwareFrameData {
   gfx::Size size;
   gfx::Rect damage_rect;
   base::SharedMemoryHandle handle;
+
+  size_t SizeInBytes() const;
+  base::CheckedNumeric<size_t> CheckedSizeInBytes() const;
 };
 
 }  // namespace cc
diff --git a/content/browser/renderer_host/software_frame_manager.cc b/content/browser/renderer_host/software_frame_manager.cc
index 387dc00..3630e59 100644
--- a/content/browser/renderer_host/software_frame_manager.cc
+++ b/content/browser/renderer_host/software_frame_manager.cc
@@ -97,15 +97,9 @@ bool SoftwareFrameManager::SwapToNewFrame(
 
   // The NULL handle is used in testing.
   if (base::SharedMemory::IsHandleValid(shared_memory->handle())) {
-    base::CheckedNumeric<size_t> size_in_bytes_checked =
-        base::CheckedNumeric<size_t>(4) *
-        base::CheckedNumeric<size_t>(frame_data->size.width()) *
-        base::CheckedNumeric<size_t>(frame_data->size.height());
-    if (!size_in_bytes_checked.IsValid()) {
-      DLOG(ERROR) << "Integer overflow when computing bytes to map.";
-      return false;
-    }
-    size_t size_in_bytes = size_in_bytes_checked.ValueOrDie();
+    DCHECK(frame_data->CheckedSizeInBytes().IsValid())
+        << "Integer overflow when computing bytes to map.";
+    size_t size_in_bytes = frame_data->SizeInBytes();
 #ifdef OS_WIN
     if (!shared_memory->Map(0)) {
       DLOG(ERROR) << "Unable to map renderer memory.";
diff --git a/content/common/cc_messages.cc b/content/common/cc_messages.cc
index 2c6370ef..712c608 100644
--- a/content/common/cc_messages.cc
+++ b/content/common/cc_messages.cc
@@ -755,4 +755,42 @@ void ParamTraits<cc::DelegatedFrameData>::Log(const param_type& p,
   l->append("])");
 }
 
+void ParamTraits<cc::SoftwareFrameData>::Write(Message* m,
+                                               const param_type& p) {
+  DCHECK(p.CheckedSizeInBytes().IsValid());
+
+  m->Reserve(sizeof(cc::SoftwareFrameData));
+  WriteParam(m, p.id);
+  WriteParam(m, p.size);
+  WriteParam(m, p.damage_rect);
+  WriteParam(m, p.handle);
+}
+
+bool ParamTraits<cc::SoftwareFrameData>::Read(const Message* m,
+                                              PickleIterator* iter,
+                                              param_type* p) {
+  if (!ReadParam(m, iter, &p->id))
+    return false;
+  if (!ReadParam(m, iter, &p->size) || !p->CheckedSizeInBytes().IsValid())
+    return false;
+  if (!ReadParam(m, iter, &p->damage_rect))
+    return false;
+  if (!ReadParam(m, iter, &p->handle))
+    return false;
+  return true;
+}
+
+void ParamTraits<cc::SoftwareFrameData>::Log(const param_type& p,
+                                             std::string* l) {
+  l->append("SoftwareFrameData(");
+  LogParam(p.id, l);
+  l->append(", ");
+  LogParam(p.size, l);
+  l->append(", ");
+  LogParam(p.damage_rect, l);
+  l->append(", ");
+  LogParam(p.handle, l);
+  l->append(")");
+}
+
 }  // namespace IPC
diff --git a/content/common/cc_messages.h b/content/common/cc_messages.h
index f567310..634c56b 100644
--- a/content/common/cc_messages.h
+++ b/content/common/cc_messages.h
@@ -106,6 +106,14 @@ struct CONTENT_EXPORT ParamTraits<cc::DelegatedFrameData> {
   static void Log(const param_type& p, std::string* l);
 };
 
+template <>
+struct CONTENT_EXPORT ParamTraits<cc::SoftwareFrameData> {
+  typedef cc::SoftwareFrameData param_type;
+  static void Write(Message* m, const param_type& p);
+  static bool Read(const Message* m, PickleIterator* iter, param_type* p);
+  static void Log(const param_type& p, std::string* l);
+};
+
 }  // namespace IPC
 
 #endif  // CONTENT_COMMON_CC_MESSAGES_H_
@@ -266,10 +274,3 @@ IPC_STRUCT_TRAITS_BEGIN(cc::GLFrameData)
   IPC_STRUCT_TRAITS_MEMBER(size)
   IPC_STRUCT_TRAITS_MEMBER(sub_buffer_rect)
 IPC_STRUCT_TRAITS_END()
-
-IPC_STRUCT_TRAITS_BEGIN(cc::SoftwareFrameData)
-  IPC_STRUCT_TRAITS_MEMBER(id)
-  IPC_STRUCT_TRAITS_MEMBER(size)
-  IPC_STRUCT_TRAITS_MEMBER(damage_rect)
-  IPC_STRUCT_TRAITS_MEMBER(handle)
-IPC_STRUCT_TRAITS_END()
diff --git a/content/common/cc_messages_unittest.cc b/content/common/cc_messages_unittest.cc
index 8a7f0d8..1e801d5 100644
--- a/content/common/cc_messages_unittest.cc
+++ b/content/common/cc_messages_unittest.cc
@@ -9,11 +9,16 @@
 #include <algorithm>
 
 #include "cc/output/compositor_frame.h"
+#include "content/public/common/common_param_traits.h"
 #include "ipc/ipc_message.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/khronos/GLES2/gl2ext.h"
 #include "third_party/skia/include/effects/SkBlurImageFilter.h"
 
+#if defined(OS_POSIX)
+#include "base/file_descriptor_posix.h"
+#endif
+
 using cc::CheckerboardDrawQuad;
 using cc::DelegatedFrameData;
 using cc::DebugBorderDrawQuad;
@@ -26,6 +31,7 @@ using cc::RenderPass;
 using cc::RenderPassDrawQuad;
 using cc::ResourceProvider;
 using cc::SharedQuadState;
+using cc::SoftwareFrameData;
 using cc::SolidColorDrawQuad;
 using cc::SurfaceDrawQuad;
 using cc::TextureDrawQuad;
@@ -745,5 +751,78 @@ TEST_F(CCMessagesTest, LargestQuadType) {
   EXPECT_EQ(sizeof(RenderPassDrawQuad), largest);
 }
 
+TEST_F(CCMessagesTest, SoftwareFrameData) {
+  cc::SoftwareFrameData frame_in;
+  frame_in.id = 3;
+  frame_in.size = gfx::Size(40, 20);
+  frame_in.damage_rect = gfx::Rect(5, 18, 31, 44);
+#if defined(OS_WIN)
+  frame_in.handle = reinterpret_cast<base::SharedMemoryHandle>(23);
+#elif defined(OS_POSIX)
+  frame_in.handle = base::FileDescriptor(23, true);
+#endif
+
+  // Write the frame.
+  IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
+  IPC::ParamTraits<cc::SoftwareFrameData>::Write(&msg, frame_in);
+
+  // Read the frame.
+  cc::SoftwareFrameData frame_out;
+  PickleIterator iter(msg);
+  EXPECT_TRUE(
+      IPC::ParamTraits<SoftwareFrameData>::Read(&msg, &iter, &frame_out));
+  EXPECT_EQ(frame_in.id, frame_out.id);
+  EXPECT_EQ(frame_in.size.ToString(), frame_out.size.ToString());
+  EXPECT_EQ(frame_in.damage_rect.ToString(), frame_out.damage_rect.ToString());
+  EXPECT_EQ(frame_in.handle, frame_out.handle);
+}
+
+TEST_F(CCMessagesTest, SoftwareFrameDataMaxInt) {
+  SoftwareFrameData frame_in;
+  frame_in.id = 3;
+  frame_in.size = gfx::Size(40, 20);
+  frame_in.damage_rect = gfx::Rect(5, 18, 31, 44);
+#if defined(OS_WIN)
+  frame_in.handle = reinterpret_cast<base::SharedMemoryHandle>(23);
+#elif defined(OS_POSIX)
+  frame_in.handle = base::FileDescriptor(23, true);
+#endif
+
+  // Write the SoftwareFrameData by hand, make sure it works.
+  {
+    IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
+    IPC::WriteParam(&msg, frame_in.id);
+    IPC::WriteParam(&msg, frame_in.size);
+    IPC::WriteParam(&msg, frame_in.damage_rect);
+    IPC::WriteParam(&msg, frame_in.handle);
+    SoftwareFrameData frame_out;
+    PickleIterator iter(msg);
+    EXPECT_TRUE(
+        IPC::ParamTraits<SoftwareFrameData>::Read(&msg, &iter, &frame_out));
+  }
+
+  // The size of the frame may overflow when multiplied together.
+  int max = std::numeric_limits<int>::max();
+  frame_in.size = gfx::Size(max, max);
+
+  // If size_t is larger than int, then int*int*4 can always fit in size_t.
+  bool expect_read = sizeof(size_t) >= sizeof(int) * 2;
+
+  // Write the SoftwareFrameData with the MaxInt size, if it causes overflow it
+  // should fail.
+  {
+    IPC::Message msg(1, 2, IPC::Message::PRIORITY_NORMAL);
+    IPC::WriteParam(&msg, frame_in.id);
+    IPC::WriteParam(&msg, frame_in.size);
+    IPC::WriteParam(&msg, frame_in.damage_rect);
+    IPC::WriteParam(&msg, frame_in.handle);
+    SoftwareFrameData frame_out;
+    PickleIterator iter(msg);
+    EXPECT_EQ(
+        expect_read,
+        IPC::ParamTraits<SoftwareFrameData>::Read(&msg, &iter, &frame_out));
+  }
+}
+
 }  // namespace
 }  // namespace content