Mojo: Improve RawSharedBuffer's error reporting, kind of.

* Make Create() just require |num_bytes| to be nonzero (so any failure
  is due to "resource exhaustion".
* Make a |IsValidMap()| to validate arguments to |Map()|, and add a
  |MapNoCheck()| that's like |Map()| but skips argument validation.
* Uncomment the part of SharedBufferDispatcherTest.CreateInvalidNumBytes
  that failed.

R=davemoore@chromium.org

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

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

5 files changed, 61 insertions, 21 deletions

diff --git a/mojo/system/raw_shared_buffer.cc b/mojo/system/raw_shared_buffer.cc
index 3a90317..a909de3 100644
--- a/mojo/system/raw_shared_buffer.cc
+++ b/mojo/system/raw_shared_buffer.cc
@@ -4,13 +4,14 @@
 
 #include "mojo/system/raw_shared_buffer.h"
 
+#include "base/logging.h"
+
 namespace mojo {
 namespace system {
 
 // static
 RawSharedBuffer* RawSharedBuffer::Create(size_t num_bytes) {
-  if (!num_bytes)
-    return NULL;
+  DCHECK_GT(num_bytes, 0u);
 
   RawSharedBuffer* rv = new RawSharedBuffer(num_bytes);
   // No need to take the lock since we haven't given the object to anyone yet.
@@ -22,13 +23,30 @@ RawSharedBuffer* RawSharedBuffer::Create(size_t num_bytes) {
 
 scoped_ptr<RawSharedBuffer::Mapping> RawSharedBuffer::Map(size_t offset,
                                                           size_t length) {
-  if (offset > num_bytes_ || length == 0)
+  if (!IsValidMap(offset, length))
     return scoped_ptr<Mapping>();
 
+  return MapNoCheck(offset, length);
+  base::AutoLock locker(lock_);
+  return MapImplNoLock(offset, length);
+}
+
+bool RawSharedBuffer::IsValidMap(size_t offset, size_t length) {
+  if (offset > num_bytes_ || length == 0)
+    return false;
+
   // Note: This is an overflow-safe check of |offset + length > num_bytes_|
   // (that |num_bytes >= offset| is verified above).
   if (length > num_bytes_ - offset)
-    return scoped_ptr<Mapping>();
+    return false;
+
+  return true;
+}
+
+scoped_ptr<RawSharedBuffer::Mapping> RawSharedBuffer::MapNoCheck(
+    size_t offset,
+    size_t length) {
+  DCHECK(IsValidMap(offset, length));
 
   base::AutoLock locker(lock_);
   return MapImplNoLock(offset, length);
diff --git a/mojo/system/raw_shared_buffer.h b/mojo/system/raw_shared_buffer.h
index 9a4b39c..c7a287c 100644
--- a/mojo/system/raw_shared_buffer.h
+++ b/mojo/system/raw_shared_buffer.h
@@ -65,7 +65,7 @@ class MOJO_SYSTEM_IMPL_EXPORT RawSharedBuffer
   };
 
   // Creates a shared buffer of size |num_bytes| bytes (initially zero-filled).
-  // Returns null on failure.
+  // |num_bytes| must be nonzero. Returns null on failure.
   static RawSharedBuffer* Create(size_t num_bytes);
 
   // Maps (some) of the shared buffer into memory; [|offset|, |offset + length|]
@@ -73,6 +73,13 @@ class MOJO_SYSTEM_IMPL_EXPORT RawSharedBuffer
   // Returns null on failure.
   scoped_ptr<Mapping> Map(size_t offset, size_t length);
 
+  // Checks if |offset| and |length| are valid arguments.
+  bool IsValidMap(size_t offset, size_t length);
+
+  // Like |Map()|, but doesn't check its arguments (which should have been
+  // preflighted using |IsValidMap()|).
+  scoped_ptr<Mapping> MapNoCheck(size_t offset, size_t length);
+
   size_t num_bytes() const { return num_bytes_; }
 
  private:
diff --git a/mojo/system/raw_shared_buffer_unittest.cc b/mojo/system/raw_shared_buffer_unittest.cc
index 9604de1..18f453b 100644
--- a/mojo/system/raw_shared_buffer_unittest.cc
+++ b/mojo/system/raw_shared_buffer_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "mojo/system/raw_shared_buffer.h"
 
+#include <limits>
+
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ -25,6 +27,7 @@ TEST(RawSharedBufferTest, Basic) {
 
   // Map it all, scribble some stuff, and then unmap it.
   {
+    EXPECT_TRUE(buffer->IsValidMap(0, kNumBytes));
     scoped_ptr<RawSharedBuffer::Mapping> mapping(buffer->Map(0, kNumBytes));
     ASSERT_TRUE(mapping);
     ASSERT_TRUE(mapping->base());
@@ -38,7 +41,10 @@ TEST(RawSharedBufferTest, Basic) {
   // unmap the first mapping, scribble on some of the second mapping, and then
   // unmap it.
   {
-    scoped_ptr<RawSharedBuffer::Mapping> mapping1(buffer->Map(0, kNumBytes));
+    ASSERT_TRUE(buffer->IsValidMap(0, kNumBytes));
+    // Use |MapNoCheck()| this time.
+    scoped_ptr<RawSharedBuffer::Mapping> mapping1(
+        buffer->MapNoCheck(0, kNumBytes));
     ASSERT_TRUE(mapping1);
     ASSERT_TRUE(mapping1->base());
     int* stuff1 = static_cast<int*>(mapping1->base());
@@ -68,6 +74,7 @@ TEST(RawSharedBufferTest, Basic) {
   // Do another partial mapping and check that everything is the way we expect
   // it to be.
   {
+    EXPECT_TRUE(buffer->IsValidMap(sizeof(int), kNumBytes - sizeof(int)));
     scoped_ptr<RawSharedBuffer::Mapping> mapping(
         buffer->Map(sizeof(int), kNumBytes - sizeof(int)));
     ASSERT_TRUE(mapping);
@@ -90,8 +97,11 @@ TEST(RawSharedBufferTest, Basic) {
 // TODO(vtl): Bigger buffers.
 
 TEST(RawSharedBufferTest, InvalidArguments) {
-  // Zero length not allowed.
-  EXPECT_FALSE(RawSharedBuffer::Create(0));
+// TODO(vtl): This part of the test fails on Mac. Figure out why!
+/*
+  // Too big.
+  EXPECT_FALSE(RawSharedBuffer::Create(std::numeric_limits<size_t>::max()));
+*/
 
   // Invalid mappings:
   scoped_refptr<RawSharedBuffer> buffer(RawSharedBuffer::Create(100));
@@ -99,18 +109,25 @@ TEST(RawSharedBufferTest, InvalidArguments) {
 
   // Zero length not allowed.
   EXPECT_FALSE(buffer->Map(0, 0));
+  EXPECT_FALSE(buffer->IsValidMap(0, 0));
 
   // Okay:
   EXPECT_TRUE(buffer->Map(0, 100));
+  EXPECT_TRUE(buffer->IsValidMap(0, 100));
   // Offset + length too big.
   EXPECT_FALSE(buffer->Map(0, 101));
+  EXPECT_FALSE(buffer->IsValidMap(0, 101));
   EXPECT_FALSE(buffer->Map(1, 100));
+  EXPECT_FALSE(buffer->IsValidMap(1, 100));
 
   // Okay:
   EXPECT_TRUE(buffer->Map(50, 50));
+  EXPECT_TRUE(buffer->IsValidMap(50, 50));
   // Offset + length too big.
   EXPECT_FALSE(buffer->Map(50, 51));
+  EXPECT_FALSE(buffer->IsValidMap(50, 51));
   EXPECT_FALSE(buffer->Map(51, 50));
+  EXPECT_FALSE(buffer->IsValidMap(51, 50));
 }
 
 // Tests that separate mappings get distinct addresses.
diff --git a/mojo/system/shared_buffer_dispatcher.cc b/mojo/system/shared_buffer_dispatcher.cc
index 9f5cf0e..684f993 100644
--- a/mojo/system/shared_buffer_dispatcher.cc
+++ b/mojo/system/shared_buffer_dispatcher.cc
@@ -42,16 +42,15 @@ MojoResult SharedBufferDispatcher::Create(
     const MojoCreateSharedBufferOptions& /*validated_options*/,
     uint64_t num_bytes,
     scoped_refptr<SharedBufferDispatcher>* result) {
+  if (!num_bytes)
+    return MOJO_RESULT_INVALID_ARGUMENT;
   if (num_bytes > kMaxSharedMemoryNumBytes)
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
 
   scoped_refptr<RawSharedBuffer> shared_buffer(
       RawSharedBuffer::Create(static_cast<size_t>(num_bytes)));
-  if (!shared_buffer) {
-    // TODO(vtl): This error code is a bit of a guess. Maybe we should propagate
-    // error codes more carefully.
+  if (!shared_buffer)
     return MOJO_RESULT_RESOURCE_EXHAUSTED;
-  }
 
   *result = new SharedBufferDispatcher(shared_buffer);
   return MOJO_RESULT_OK;
@@ -115,13 +114,15 @@ MojoResult SharedBufferDispatcher::MapBufferImplNoLock(
   if (num_bytes > static_cast<uint64_t>(std::numeric_limits<size_t>::max()))
     return MOJO_RESULT_INVALID_ARGUMENT;
 
-  DCHECK(mapping);
-  *mapping = shared_buffer_->Map(static_cast<size_t>(offset),
-                                 static_cast<size_t>(num_bytes));
-  if (!*mapping) {
-    // TODO(vtl): This is just a guess; propagate errors more thoroughly.
+  if (!shared_buffer_->IsValidMap(static_cast<size_t>(offset),
+                                  static_cast<size_t>(num_bytes)))
     return MOJO_RESULT_INVALID_ARGUMENT;
-  }
+
+  DCHECK(mapping);
+  *mapping = shared_buffer_->MapNoCheck(static_cast<size_t>(offset),
+                                        static_cast<size_t>(num_bytes));
+  if (!*mapping)
+    return MOJO_RESULT_RESOURCE_EXHAUSTED;
 
   return MOJO_RESULT_OK;
 }
diff --git a/mojo/system/shared_buffer_dispatcher_unittest.cc b/mojo/system/shared_buffer_dispatcher_unittest.cc
index 0af6e8b..ecd055c 100644
--- a/mojo/system/shared_buffer_dispatcher_unittest.cc
+++ b/mojo/system/shared_buffer_dispatcher_unittest.cc
@@ -181,13 +181,10 @@ TEST(SharedBufferDispatcherTest, CreateInvalidNumBytes) {
                                            &dispatcher));
   EXPECT_FALSE(dispatcher);
 
-  // TODO(vtl): currently fails.
-/*
   // Zero size.
   EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
             SharedBufferDispatcher::Create(validated_options, 0, &dispatcher));
   EXPECT_FALSE(dispatcher);
-*/
 }
 
 TEST(SharedBufferDispatcherTest, MapBufferInvalidArguments) {