Fixing a Dr. Memory hang in the remoting_unittests

remoting/host/security_key/remote_security_key_message_reader_impl.cc

Index: remoting/host/security_key/remote_security_key_message_reader_impl.cc
diff --git a/remoting/host/security_key/remote_security_key_message_reader_impl.cc b/remoting/host/security_key/remote_security_key_message_reader_impl.cc
index 55c4061d1af7b7366f307885e3fb5ce0e04ac87f..618ebfe029988d9e7a26ccc97954af93fca5cba8 100644
--- a/remoting/host/security_key/remote_security_key_message_reader_impl.cc
+++ b/remoting/host/security_key/remote_security_key_message_reader_impl.cc
@@ -85,16 +85,29 @@ void RemoteSecurityKeyMessageReaderImpl::ReadMessage() {
       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.";