Fixing a Dr. Memory hang in the remoting_unittests

remoting/host/security_key/remote_security_key_message_reader_impl.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/security_key/remote_security_key_message_reader_impl.h"
 
 #include <cstdint>
 #include <string>
 #include <utility>
 
@@ skipping 67 matching lines @@
       NotifyError();
       return;
     }
 
     if (!SecurityKeyMessage::IsValidMessageSize(total_message_size_bytes)) {
       LOG(ERROR) << "Message size too large: " << total_message_size_bytes;
       NotifyError();
       return;
     }
 
+    // Read the message type.
     std::string message_data(total_message_size_bytes, '\0');
-    read_result = read_stream_.ReadAtCurrentPos(string_as_array(&message_data),
-                                                total_message_size_bytes);
-    // The static cast is safe as we know the value is smaller than max int.
-    if (read_result != static_cast<int>(total_message_size_bytes)) {
-      LOG(ERROR) << "Failed to read message: " << read_result;
+    read_result =
+        read_stream_.ReadAtCurrentPos(string_as_array(&message_data), 1);
+    if (read_result != 1) {
+      LOG(ERROR) << "Failed to read message type: " << read_result;
       NotifyError();
       return;
     }
 
+    // Read the message payload, if one exists.
+    if (total_message_size_bytes > 1) {
+      uint32_t remaining_bytes = total_message_size_bytes - 1;
+      read_result =
+          read_stream_.ReadAtCurrentPos(&message_data[1], remaining_bytes);

[Sergey Ulanov, 2016/04/18 18:57:32]

I don't understand why you need to call ReadAtCurrentPos() twice.
ReadAtCurrentPos() is supposed to read as many bites as requested, unless error
occurs. It shouldn't matter if the other end writes data in chunks or not.
Otherwise you would need to handle partial reads for payload as well (in case
it's sent with 2 write() calls).

The posix version of ReadAtCurrenPos() handles partial reads explicitly by
retrying read(), but the windows version doesn't:
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_wi...
It's not clear to me what's the expected behavior for ReadFile() when reading
from pipes, but it's likely that it can performs partial reads there. So it
looks like the problem here is that ReadAtCurrenPos() behavior is not consistent
across the two implementations (at least when used for pipes). The comments for
ReadAtCurrenPos() say that "it is not intended for stream oriented files". I
think we need to either use ReadAtCurrentPosNoBestEffort() and retry after
partial reads or to fix the windows implementation of ReadAtCurrenPos() to make
it consistent with POSIX.

[joedow, 2016/04/19 17:28:10]

This is a good point, my workaround isn't robust enough.  I agree that the
behavior for the Read* functions feels wrong.  I am a bit hesitant to change the
base function since it is simple enough to treat it as a non-blocking read
operation.

I'd like to fix this issue to unblock my change and the Dr. memory fix and have
filed crbug.com/604787 to track the file change.

+      // The static cast is safe as we know the value is smaller than max int.
+      if (read_result != static_cast<int>(remaining_bytes)) {
+        LOG(ERROR) << "Failed to read message payload: " << read_result;
+        NotifyError();
+        return;
+      }
+    }
+
     std::unique_ptr<SecurityKeyMessage> message(new SecurityKeyMessage());
     if (!message->ParseMessage(message_data)) {
       LOG(ERROR) << "Invalid message data received.";
       NotifyError();
       return;
     }
 
     // Notify callback of the new message received.
     main_task_runner_->PostTask(
         FROM_HERE,
@@ skipping 16 matching lines @@
   DCHECK(main_task_runner_->RunsTasksOnCurrentThread());
   message_callback_.Run(std::move(message));
 }
 
 void RemoteSecurityKeyMessageReaderImpl::InvokeErrorCallback() {
   DCHECK(main_task_runner_->RunsTasksOnCurrentThread());
   error_callback_.Run();
 }
 
 }  // namespace remoting