Updating SecurityKeyAuthHandlerPosix socket lifetime management

remoting/host/security_key/security_key_socket.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/security_key_socket.h"
 
 #include <utility>
 
 #include "base/callback_helpers.h"
 #include "base/macros.h"
@@ skipping 12 matching lines @@
 
 // SSH Failure Code
 const char kSshError[] = {0x05};
 
 }  // namespace
 
 SecurityKeySocket::SecurityKeySocket(std::unique_ptr<net::StreamSocket> socket,
                                      base::TimeDelta timeout,
                                      const base::Closure& timeout_callback)
     : socket_(std::move(socket)),
-      read_completed_(false),
       read_buffer_(new net::IOBufferWithSize(kRequestReadBufferLength)) {
   timer_.reset(new base::Timer(false, false));
   timer_->Start(FROM_HERE, timeout, timeout_callback);
 }
 
 SecurityKeySocket::~SecurityKeySocket() {}
 
 bool SecurityKeySocket::GetAndClearRequestData(std::string* data_out) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(read_completed_);
+  DCHECK(!waiting_for_request_);
 
-  if (!read_completed_) {
-    return false;
-  }
   if (!IsRequestComplete() || IsRequestTooLarge()) {
     return false;
   }
   // The request size is not part of the data; don't send it.
   data_out->assign(request_data_.begin() + kRequestSizeBytes,
                    request_data_.end());
   request_data_.clear();
   return true;
 }
 
@@ skipping 16 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
 
   SendResponse(std::string(kSshError, arraysize(kSshError)));
 }
 
 void SecurityKeySocket::StartReadingRequest(
     const base::Closure& request_received_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(request_received_callback_.is_null());
 
+  waiting_for_request_ = true;
   request_received_callback_ = request_received_callback;
+
   DoRead();
 }
 
 void SecurityKeySocket::OnDataWritten(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(write_buffer_);
 
   if (result < 0) {
     LOG(ERROR) << "Error sending response: " << result;
     return;
@@ skipping 20 matching lines @@
     OnDataWritten(result);
   }
 }
 
 void SecurityKeySocket::OnDataRead(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (result <= 0) {
     if (result < 0) {
       LOG(ERROR) << "Error reading request: " << result;
+      socket_read_error_ = true;
     }
-    read_completed_ = true;
+    waiting_for_request_ = false;
     base::ResetAndReturn(&request_received_callback_).Run();
     return;
   }
 
   ResetTimer();
+  // TODO(joedow): If there are multiple requests in a burst, it is possible
+  // that we could read too many bytes from the buffer (e.g. all of request #1
+  // and some of request #2).  We should consider using the request header to
+  // determine the request length and only read that amount from buffer.

[Sergey Ulanov, 2016/12/20 00:54:23]

I think it would be easier to keep the current reading logic and update the code
that handles compelte requests: when extra bytes are read they need to be kept
in the buffer and handled later.

[joedow, 2016/12/20 02:24:03]

That's reasonable.  I'll work on that tomorrow and get a CL out for it.

   request_data_.insert(request_data_.end(), read_buffer_->data(),
                        read_buffer_->data() + result);
   if (IsRequestComplete()) {
-    read_completed_ = true;
+    waiting_for_request_ = false;
     base::ResetAndReturn(&request_received_callback_).Run();
     return;
   }
 
   DoRead();
 }
 
 void SecurityKeySocket::DoRead() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ skipping 45 matching lines @@
   return response_len;
 }
 
 void SecurityKeySocket::ResetTimer() {
   if (timer_->IsRunning()) {
     timer_->Reset();
   }
 }
 
 }  // namespace remoting