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());

[Sergey Ulanov, 2016/12/19 23:19:18]

If we get a request and some extra data at the end it would be returned here
(e.g. if the sender tries to send second request without waiting for the
response to the first one).

[joedow, 2016/12/20 00:28:00]

That's true, but it would have happened with the previous implementation as
well.  It shouldn't be too hard to fix it (use the header bytes to define the
number of bytes to transfer and save the rest) but I'd prefer to do that in a
follow-up CL.  I'll add a TODO for now.

   request_data_.clear();
   return true;
 }
 
 void SecurityKeySocket::SendResponse(const std::string& response_data) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!write_buffer_);
 
   std::string response_length_string = GetResponseLengthAsBytes(response_data);
   int response_len = response_length_string.size() + response_data.size();
   std::unique_ptr<std::string> response(
       new std::string(response_length_string + response_data));
   write_buffer_ = new net::DrainableIOBuffer(
       new net::StringIOBuffer(std::move(response)), response_len);
 
   DCHECK(write_buffer_->BytesRemaining());
   DoWrite();
 }
 
 void SecurityKeySocket::SendSshError() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   SendResponse(std::string(kSshError, arraysize(kSshError)));
 }
 
 void SecurityKeySocket::StartReadingRequest(

[Sergey Ulanov, 2016/12/19 23:19:18]

Maybe rename this to ReadRequest()? That would make it more consistent with
other async calls (e.g. net::Socket::Read()).

     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();

[Sergey Ulanov, 2016/12/19 23:19:18]

Do we need to check if there a complete request in |read_buffer_| before trying
to read again?

[joedow, 2016/12/20 00:28:00]

Right now we drain everything from the buffer (line 131/132) w/o honoring the
length of the request.  I do think this will be necessary if we add some logic
around the request length.

 }
 
 void SecurityKeySocket::OnDataWritten(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(write_buffer_);
 
   if (result < 0) {
     LOG(ERROR) << "Error sending response: " << result;
     return;
   }
@@ skipping 19 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();
   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