Convert ReadData...() to use the new user pointer handling (see r285350).

R=darin@chromium.org

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

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

mojo/system/core.cc

@@ -395,8 +395,7 @@ MojoResult Core::ReadData(MojoHandle data_pipe_consumer_handle,
   if (!dispatcher)
     return MOJO_RESULT_INVALID_ARGUMENT;
 
-  return dispatcher->ReadData(elements.GetPointerUnsafe(),
-                              num_bytes.GetPointerUnsafe(), flags);
+  return dispatcher->ReadData(elements, num_bytes, flags);
 }
 
 MojoResult Core::BeginReadData(MojoHandle data_pipe_consumer_handle,

mojo/system/core_test_base.cc

@@ -108,8 +108,8 @@ class MockDispatcher : public Dispatcher {
     return MOJO_RESULT_UNIMPLEMENTED;
   }
 
-  virtual MojoResult ReadDataImplNoLock(void* /*elements*/,
-                                        uint32_t* /*num_bytes*/,
+  virtual MojoResult ReadDataImplNoLock(UserPointer<void> /*elements*/,
+                                        UserPointer<uint32_t> /*num_bytes*/,
                                         MojoReadDataFlags /*flags*/) OVERRIDE {
     info_->IncrementReadDataCallCount();
     lock().AssertAcquired();

mojo/system/data_pipe.cc

@@ -230,8 +230,8 @@ void DataPipe::ConsumerClose() {
       ProducerGetHandleSignalsStateNoLock());
 }
 
-MojoResult DataPipe::ConsumerReadData(void* elements,
-                                      uint32_t* num_bytes,
+MojoResult DataPipe::ConsumerReadData(UserPointer<void> elements,
+                                      UserPointer<uint32_t> num_bytes,
                                       bool all_or_none) {
   base::AutoLock locker(lock_);
   DCHECK(has_local_consumer_no_lock());
@@ -239,21 +239,26 @@ MojoResult DataPipe::ConsumerReadData(void* elements,
   if (consumer_in_two_phase_read_no_lock())
     return MOJO_RESULT_BUSY;
 
-  if (*num_bytes % element_num_bytes_ != 0)
+  uint32_t max_num_bytes_to_read = num_bytes.Get();
+  if (max_num_bytes_to_read % element_num_bytes_ != 0)
     return MOJO_RESULT_INVALID_ARGUMENT;
 
-  if (*num_bytes == 0)
+  if (max_num_bytes_to_read == 0)
     return MOJO_RESULT_OK;  // Nothing to do.
 
+  uint32_t min_num_bytes_to_read = all_or_none ? max_num_bytes_to_read : 0;
+
   HandleSignalsState old_producer_state = ProducerGetHandleSignalsStateNoLock();
-  MojoResult rv = ConsumerReadDataImplNoLock(elements, num_bytes, all_or_none);
+  MojoResult rv = ConsumerReadDataImplNoLock(elements, num_bytes,
+                                             max_num_bytes_to_read,
+                                             min_num_bytes_to_read);
   HandleSignalsState new_producer_state = ProducerGetHandleSignalsStateNoLock();
   if (!new_producer_state.equals(old_producer_state))
     AwakeProducerWaitersForStateChangeNoLock(new_producer_state);
   return rv;
 }
 
-MojoResult DataPipe::ConsumerDiscardData(uint32_t* num_bytes,
+MojoResult DataPipe::ConsumerDiscardData(UserPointer<uint32_t> num_bytes,
                                          bool all_or_none) {
   base::AutoLock locker(lock_);
   DCHECK(has_local_consumer_no_lock());
@@ -261,21 +266,27 @@ MojoResult DataPipe::ConsumerDiscardData(uint32_t* num_bytes,
   if (consumer_in_two_phase_read_no_lock())
     return MOJO_RESULT_BUSY;
 
-  if (*num_bytes % element_num_bytes_ != 0)
+  uint32_t max_num_bytes_to_discard = num_bytes.Get();
+  if (max_num_bytes_to_discard % element_num_bytes_ != 0)
     return MOJO_RESULT_INVALID_ARGUMENT;
 
-  if (*num_bytes == 0)
+  if (max_num_bytes_to_discard == 0)
     return MOJO_RESULT_OK;  // Nothing to do.
 
+  uint32_t min_num_bytes_to_discard = all_or_none ? max_num_bytes_to_discard :
+                                                    0;
+
   HandleSignalsState old_producer_state = ProducerGetHandleSignalsStateNoLock();
-  MojoResult rv = ConsumerDiscardDataImplNoLock(num_bytes, all_or_none);
+  MojoResult rv = ConsumerDiscardDataImplNoLock(num_bytes,
+                                                max_num_bytes_to_discard,
+                                                min_num_bytes_to_discard);
   HandleSignalsState new_producer_state = ProducerGetHandleSignalsStateNoLock();
   if (!new_producer_state.equals(old_producer_state))
     AwakeProducerWaitersForStateChangeNoLock(new_producer_state);
   return rv;
 }
 
-MojoResult DataPipe::ConsumerQueryData(uint32_t* num_bytes) {
+MojoResult DataPipe::ConsumerQueryData(UserPointer<uint32_t> num_bytes) {
   base::AutoLock locker(lock_);
   DCHECK(has_local_consumer_no_lock());
 

mojo/system/data_pipe.h

@@ -71,12 +71,12 @@ class MOJO_SYSTEM_IMPL_EXPORT DataPipe :
   void ConsumerClose();
   // This does not validate its arguments, except to check that |*num_bytes| is
   // a multiple of |element_num_bytes_|.
-  MojoResult ConsumerReadData(void* elements,
-                              uint32_t* num_bytes,
+  MojoResult ConsumerReadData(UserPointer<void> elements,
+                              UserPointer<uint32_t> num_bytes,
                               bool all_or_none);
-  MojoResult ConsumerDiscardData(uint32_t* num_bytes,
+  MojoResult ConsumerDiscardData(UserPointer<uint32_t> num_bytes,
                                  bool all_or_none);
-  MojoResult ConsumerQueryData(uint32_t* num_bytes);
+  MojoResult ConsumerQueryData(UserPointer<uint32_t> num_bytes);
   MojoResult ConsumerBeginReadData(UserPointer<const void*> buffer,
                                    UserPointer<uint32_t> buffer_num_bytes,
                                    bool all_or_none);
@@ -111,13 +111,18 @@ class MOJO_SYSTEM_IMPL_EXPORT DataPipe :
 
   virtual void ConsumerCloseImplNoLock() = 0;
   // |*num_bytes| will be a nonzero multiple of |element_num_bytes_|.
-  virtual MojoResult ConsumerReadDataImplNoLock(void* elements,
-                                                uint32_t* num_bytes,
-                                                bool all_or_none) = 0;
-  virtual MojoResult ConsumerDiscardDataImplNoLock(uint32_t* num_bytes,
-                                                   bool all_or_none) = 0;
+  virtual MojoResult ConsumerReadDataImplNoLock(
+      UserPointer<void> elements,
+      UserPointer<uint32_t> num_bytes,
+      uint32_t max_num_bytes_to_read,
+      uint32_t min_num_bytes_to_read) = 0;
+  virtual MojoResult ConsumerDiscardDataImplNoLock(
+      UserPointer<uint32_t> num_bytes,
+      uint32_t max_num_bytes_to_discard,
+      uint32_t min_num_bytes_to_discard) = 0;
   // |*num_bytes| will be a nonzero multiple of |element_num_bytes_|.
-  virtual MojoResult ConsumerQueryDataImplNoLock(uint32_t* num_bytes) = 0;
+  virtual MojoResult ConsumerQueryDataImplNoLock(
+      UserPointer<uint32_t> num_bytes) = 0;
   virtual MojoResult ConsumerBeginReadDataImplNoLock(
       UserPointer<const void*> buffer,
       UserPointer<uint32_t> buffer_num_bytes,

mojo/system/data_pipe_consumer_dispatcher.cc

@@ -51,33 +51,28 @@ DataPipeConsumerDispatcher::CreateEquivalentDispatcherAndCloseImplNoLock() {
 }
 
 MojoResult DataPipeConsumerDispatcher::ReadDataImplNoLock(
-    void* elements,
-    uint32_t* num_bytes,
+    UserPointer<void> elements,
+    UserPointer<uint32_t> num_bytes,
     MojoReadDataFlags flags) {
   lock().AssertAcquired();
 
-  if (!VerifyUserPointer<uint32_t>(num_bytes))
-    return MOJO_RESULT_INVALID_ARGUMENT;
-
   if ((flags & MOJO_READ_DATA_FLAG_DISCARD)) {
     // These flags are mutally exclusive.
     if ((flags & MOJO_READ_DATA_FLAG_QUERY))
       return MOJO_RESULT_INVALID_ARGUMENT;
-    DVLOG_IF(2, elements) << "Discard mode: ignoring non-null |elements|";
+    DVLOG_IF(2, !elements.IsNull())
+        << "Discard mode: ignoring non-null |elements|";
     return data_pipe_->ConsumerDiscardData(
         num_bytes, (flags & MOJO_READ_DATA_FLAG_ALL_OR_NONE));
   }
 
   if ((flags & MOJO_READ_DATA_FLAG_QUERY)) {
     DCHECK(!(flags & MOJO_READ_DATA_FLAG_DISCARD));  // Handled above.
-    DVLOG_IF(2, elements) << "Query mode: ignoring non-null |elements|";
+    DVLOG_IF(2, !elements.IsNull())
+        << "Query mode: ignoring non-null |elements|";
     return data_pipe_->ConsumerQueryData(num_bytes);
   }
 
-  // Only verify |elements| if we're neither discarding nor querying.
-  if (!VerifyUserPointerWithSize<1>(elements, *num_bytes))
-    return MOJO_RESULT_INVALID_ARGUMENT;
-
   return data_pipe_->ConsumerReadData(
       elements, num_bytes, (flags & MOJO_READ_DATA_FLAG_ALL_OR_NONE));
 }

mojo/system/data_pipe_consumer_dispatcher.h

@@ -37,8 +37,8 @@ class MOJO_SYSTEM_IMPL_EXPORT DataPipeConsumerDispatcher : public Dispatcher {
   virtual void CloseImplNoLock() OVERRIDE;
   virtual scoped_refptr<Dispatcher>
       CreateEquivalentDispatcherAndCloseImplNoLock() OVERRIDE;
-  virtual MojoResult ReadDataImplNoLock(void* elements,
-                                        uint32_t* num_bytes,
+  virtual MojoResult ReadDataImplNoLock(UserPointer<void> elements,
+                                        UserPointer<uint32_t> num_bytes,
                                         MojoReadDataFlags flags) OVERRIDE;
   virtual MojoResult BeginReadDataImplNoLock(
       UserPointer<const void*> buffer,

mojo/system/dispatcher.cc

@@ -164,8 +164,8 @@ MojoResult Dispatcher::EndWriteData(uint32_t num_bytes_written) {
   return EndWriteDataImplNoLock(num_bytes_written);
 }
 
-MojoResult Dispatcher::ReadData(void* elements,
-                                uint32_t* num_bytes,
+MojoResult Dispatcher::ReadData(UserPointer<void> elements,
+                                UserPointer<uint32_t> num_bytes,
                                 MojoReadDataFlags flags) {
   base::AutoLock locker(lock_);
   if (is_closed_)
@@ -303,8 +303,8 @@ MojoResult Dispatcher::EndWriteDataImplNoLock(uint32_t /*num_bytes_written*/) {
   return MOJO_RESULT_INVALID_ARGUMENT;
 }
 
-MojoResult Dispatcher::ReadDataImplNoLock(void* /*elements*/,
-                                          uint32_t* /*num_bytes*/,
+MojoResult Dispatcher::ReadDataImplNoLock(UserPointer<void> /*elements*/,
+                                          UserPointer<uint32_t> /*num_bytes*/,
                                           MojoReadDataFlags /*flags*/) {
   lock_.AssertAcquired();
   DCHECK(!is_closed_);

mojo/system/dispatcher.h

@@ -99,8 +99,8 @@ class MOJO_SYSTEM_IMPL_EXPORT Dispatcher :
                             UserPointer<uint32_t> buffer_num_bytes,
                             MojoWriteDataFlags flags);
   MojoResult EndWriteData(uint32_t num_bytes_written);
-  MojoResult ReadData(void* elements,
-                      uint32_t* num_bytes,
+  MojoResult ReadData(UserPointer<void> elements,
+                      UserPointer<uint32_t> num_bytes,
                       MojoReadDataFlags flags);
   MojoResult BeginReadData(UserPointer<const void*> buffer,
                            UserPointer<uint32_t> buffer_num_bytes,
@@ -230,8 +230,8 @@ class MOJO_SYSTEM_IMPL_EXPORT Dispatcher :
       UserPointer<uint32_t> buffer_num_bytes,
       MojoWriteDataFlags flags);
   virtual MojoResult EndWriteDataImplNoLock(uint32_t num_bytes_written);
-  virtual MojoResult ReadDataImplNoLock(void* elements,
-                                        uint32_t* num_bytes,
+  virtual MojoResult ReadDataImplNoLock(UserPointer<void> elements,
+                                        UserPointer<uint32_t> num_bytes,
                                         MojoReadDataFlags flags);
   virtual MojoResult BeginReadDataImplNoLock(
       UserPointer<const void*> buffer,

mojo/system/dispatcher_unittest.cc

@@ -59,7 +59,8 @@ TEST(DispatcherTest, Basic) {
   EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
             d->EndWriteData(0));
   EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
-            d->ReadData(NULL, NULL, MOJO_READ_DATA_FLAG_NONE));
+            d->ReadData(NullUserPointer(), NullUserPointer(),
+                        MOJO_READ_DATA_FLAG_NONE));
   EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
             d->BeginReadData(NullUserPointer(), NullUserPointer(),
                              MOJO_READ_DATA_FLAG_NONE));
@@ -89,7 +90,8 @@ TEST(DispatcherTest, Basic) {
   EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
             d->EndWriteData(0));
   EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
-            d->ReadData(NULL, NULL, MOJO_READ_DATA_FLAG_NONE));
+            d->ReadData(NullUserPointer(), NullUserPointer(),
+                        MOJO_READ_DATA_FLAG_NONE));
   EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
             d->BeginReadData(NullUserPointer(), NullUserPointer(),
                              MOJO_READ_DATA_FLAG_NONE));
@@ -175,7 +177,8 @@ class ThreadSafetyStressThread : public base::SimpleThread {
         break;
       case READ_DATA:
         EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,
-                  dispatcher_->ReadData(NULL, NULL, MOJO_READ_DATA_FLAG_NONE));
+                  dispatcher_->ReadData(NullUserPointer(), NullUserPointer(),
+                                        MOJO_READ_DATA_FLAG_NONE));
         break;
       case BEGIN_READ_DATA:
         EXPECT_EQ(MOJO_RESULT_INVALID_ARGUMENT,

mojo/system/local_data_pipe.cc

@@ -6,7 +6,7 @@
 // E.g., |start_index_| and |current_num_bytes_| fit into a |uint32_t|, but
 // their sum may not. This is bad and poses a security risk. (We're currently
 // saved by the limit on capacity -- the maximum size of the buffer, checked in
-// |DataPipe::ValidateOptions()|, is currently sufficiently small.
+// |DataPipe::ValidateOptions()|, is currently sufficiently small.)
 
 #include "mojo/system/local_data_pipe.h"
 
@@ -171,13 +171,16 @@ void LocalDataPipe::ConsumerCloseImplNoLock() {
   current_num_bytes_ = 0;
 }
 
-MojoResult LocalDataPipe::ConsumerReadDataImplNoLock(void* elements,
-                                                     uint32_t* num_bytes,
-                                                     bool all_or_none) {
-  DCHECK_EQ(*num_bytes % element_num_bytes(), 0u);
-  DCHECK_GT(*num_bytes, 0u);
+MojoResult LocalDataPipe::ConsumerReadDataImplNoLock(
+    UserPointer<void> elements,
+    UserPointer<uint32_t> num_bytes,
+    uint32_t max_num_bytes_to_read,
+    uint32_t min_num_bytes_to_read) {
+  DCHECK_EQ(max_num_bytes_to_read % element_num_bytes(), 0u);
+  DCHECK_EQ(min_num_bytes_to_read % element_num_bytes(), 0u);
+  DCHECK_GT(max_num_bytes_to_read, 0u);
 
-  if (all_or_none && *num_bytes > current_num_bytes_) {
+  if (min_num_bytes_to_read > current_num_bytes_) {
     // Don't return "should wait" since you can't wait for a specified amount of
     // data.
     return producer_open_no_lock() ? MOJO_RESULT_OUT_OF_RANGE :
@@ -185,7 +188,7 @@ MojoResult LocalDataPipe::ConsumerReadDataImplNoLock(void* elements,
   }
 
   size_t num_bytes_to_read =
-      std::min(static_cast<size_t>(*num_bytes), current_num_bytes_);
+      std::min(static_cast<size_t>(max_num_bytes_to_read), current_num_bytes_);
   if (num_bytes_to_read == 0) {
     return producer_open_no_lock() ? MOJO_RESULT_SHOULD_WAIT :
                                      MOJO_RESULT_FAILED_PRECONDITION;
@@ -194,26 +197,28 @@ MojoResult LocalDataPipe::ConsumerReadDataImplNoLock(void* elements,
   // The amount we can read in our first |memcpy()|.
   size_t num_bytes_to_read_first =
       std::min(num_bytes_to_read, GetMaxNumBytesToReadNoLock());
-  memcpy(elements, buffer_.get() + start_index_, num_bytes_to_read_first);
+  elements.PutArray(buffer_.get() + start_index_, num_bytes_to_read_first);
 
   if (num_bytes_to_read_first < num_bytes_to_read) {
     // The "second read index" is zero.
-    memcpy(static_cast<char*>(elements) + num_bytes_to_read_first,
-           buffer_.get(),
-           num_bytes_to_read - num_bytes_to_read_first);
+    elements.At(num_bytes_to_read_first).PutArray(
+        buffer_.get(), num_bytes_to_read - num_bytes_to_read_first);
   }
 
   MarkDataAsConsumedNoLock(num_bytes_to_read);
-  *num_bytes = static_cast<uint32_t>(num_bytes_to_read);
+  num_bytes.Put(static_cast<uint32_t>(num_bytes_to_read));
   return MOJO_RESULT_OK;
 }
 
-MojoResult LocalDataPipe::ConsumerDiscardDataImplNoLock(uint32_t* num_bytes,
-                                                        bool all_or_none) {
-  DCHECK_EQ(*num_bytes % element_num_bytes(), 0u);
-  DCHECK_GT(*num_bytes, 0u);
+MojoResult LocalDataPipe::ConsumerDiscardDataImplNoLock(
+    UserPointer<uint32_t> num_bytes,
+    uint32_t max_num_bytes_to_discard,
+    uint32_t min_num_bytes_to_discard) {
+  DCHECK_EQ(max_num_bytes_to_discard % element_num_bytes(), 0u);
+  DCHECK_EQ(min_num_bytes_to_discard % element_num_bytes(), 0u);
+  DCHECK_GT(max_num_bytes_to_discard, 0u);
 
-  if (all_or_none && *num_bytes > current_num_bytes_) {
+  if (min_num_bytes_to_discard > current_num_bytes_) {
     // Don't return "should wait" since you can't wait for a specified amount of
     // data.
     return producer_open_no_lock() ? MOJO_RESULT_OUT_OF_RANGE :
@@ -227,15 +232,17 @@ MojoResult LocalDataPipe::ConsumerDiscardDataImplNoLock(uint32_t* num_bytes,
   }
 
   size_t num_bytes_to_discard =
-      std::min(static_cast<size_t>(*num_bytes), current_num_bytes_);
+      std::min(static_cast<size_t>(max_num_bytes_to_discard),
+                                   current_num_bytes_);
   MarkDataAsConsumedNoLock(num_bytes_to_discard);
-  *num_bytes = static_cast<uint32_t>(num_bytes_to_discard);
+  num_bytes.Put(static_cast<uint32_t>(num_bytes_to_discard));
   return MOJO_RESULT_OK;
 }
 
-MojoResult LocalDataPipe::ConsumerQueryDataImplNoLock(uint32_t* num_bytes) {
+MojoResult LocalDataPipe::ConsumerQueryDataImplNoLock(
+    UserPointer<uint32_t> num_bytes) {
   // Note: This cast is safe, since the capacity fits into a |uint32_t|.
-  *num_bytes = static_cast<uint32_t>(current_num_bytes_);
+  num_bytes.Put(static_cast<uint32_t>(current_num_bytes_));
   return MOJO_RESULT_OK;
 }
 

mojo/system/local_data_pipe.h

@@ -43,12 +43,17 @@ class MOJO_SYSTEM_IMPL_EXPORT LocalDataPipe : public DataPipe {
   virtual HandleSignalsState
       ProducerGetHandleSignalsStateNoLock() const OVERRIDE;
   virtual void ConsumerCloseImplNoLock() OVERRIDE;
-  virtual MojoResult ConsumerReadDataImplNoLock(void* elements,
-                                                uint32_t* num_bytes,
-                                                bool all_or_none) OVERRIDE;
-  virtual MojoResult ConsumerDiscardDataImplNoLock(uint32_t* num_bytes,
-                                                   bool all_or_none) OVERRIDE;
-  virtual MojoResult ConsumerQueryDataImplNoLock(uint32_t* num_bytes) OVERRIDE;
+  virtual MojoResult ConsumerReadDataImplNoLock(
+      UserPointer<void> elements,
+      UserPointer<uint32_t> num_bytes,
+      uint32_t max_num_bytes_to_read,
+      uint32_t min_num_bytes_to_read) OVERRIDE;
+  virtual MojoResult ConsumerDiscardDataImplNoLock(
+      UserPointer<uint32_t> num_bytes,
+      uint32_t max_num_bytes_to_discard,
+      uint32_t min_num_bytes_to_discard) OVERRIDE;
+  virtual MojoResult ConsumerQueryDataImplNoLock(
+      UserPointer<uint32_t> num_bytes) OVERRIDE;
   virtual MojoResult ConsumerBeginReadDataImplNoLock(
       UserPointer<const void*> buffer,
       UserPointer<uint32_t> buffer_num_bytes,

mojo/system/memory.h

@@ -181,11 +181,10 @@ class UserPointer {
     memcpy(pointer_, source, count * sizeof(NonVoidType));
   }
 
-  // Gets a |UserPointer| at offset |i| (in |Type|s) relative to this. This
-  // method is not valid if |Type| is |void| (TODO(vtl): Maybe I should make it
-  // valid, with the offset in bytes?).
+  // Gets a |UserPointer| at offset |i| (in |Type|s) relative to this.
   UserPointer At(size_t i) const {
-    return UserPointer(pointer_ + i);
+    return UserPointer(static_cast<Type*>(
+        static_cast<NonVoidType*>(pointer_) + i));
   }
 
   // TODO(vtl): This is temporary. Get rid of this. (We should pass