Fixing SecurityKeySocket unit test issues and cleanup

remoting/host/security_key/security_key_auth_handler_posix.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_auth_handler.h"
 
 #include <unistd.h>
 
 #include <cstdint>
 #include <map>
@@ skipping 36 matching lines @@
     HOST_LOG << "Refused socket connection from uid " << credentials.user_id;
   return allowed;
 }
 
 // Returns the command code (the first byte of the data) if it exists, or -1 if
 // the data is empty.
 unsigned int GetCommandCode(const std::string& data) {
   return data.empty() ? -1 : static_cast<unsigned int>(data[0]);
 }
 
+void DeleteSocket(std::unique_ptr<remoting::SecurityKeySocket> socket) {}

[Sergey Ulanov, 2016/12/07 20:41:36]

base::DeletePointer() can be used instead of this function

[joedow, 2016/12/13 20:30:44]

Acknowledged.

+
 }  // namespace
 
 namespace remoting {
 
 class SecurityKeyAuthHandlerPosix : public SecurityKeyAuthHandler {
  public:
   explicit SecurityKeyAuthHandlerPosix(
       scoped_refptr<base::SingleThreadTaskRunner> file_task_runner);
   ~SecurityKeyAuthHandlerPosix() override;
 
@@ skipping 20 matching lines @@
   void OnAccepted(int result);
 
   // Called when a SecurityKeySocket has done reading.
   void OnReadComplete(int security_key_connection_id);
 
   // Gets an active socket iterator for |security_key_connection_id|.
   ActiveSockets::const_iterator GetSocketForConnectionId(
       int security_key_connection_id) const;
 
   // Send an error and closes an active socket.
-  void SendErrorAndCloseActiveSocket(const ActiveSockets::const_iterator& iter);
+  void SendErrorAndCloseActiveSocket(const ActiveSockets::iterator& iter);
 
   // A request timed out.
   void RequestTimedOut(int security_key_connection_id);
 
   // Ensures SecurityKeyAuthHandlerPosix methods are called on the same thread.
   base::ThreadChecker thread_checker_;
 
   // Socket used to listen for authorization requests.
   std::unique_ptr<net::UnixDomainServerSocket> auth_socket_;
 
@@ skipping 89 matching lines @@
   return GetSocketForConnectionId(security_key_connection_id) !=
          active_sockets_.end();
 }
 
 void SecurityKeyAuthHandlerPosix::SendClientResponse(
     int security_key_connection_id,
     const std::string& response) {
   ActiveSockets::const_iterator iter =
       GetSocketForConnectionId(security_key_connection_id);
   if (iter != active_sockets_.end()) {
-    iter->second->SendResponse(response);
+    iter->second->SendResponse(response, base::Closure());
   } else {
     LOG(WARNING) << "Unknown gnubby-auth data connection: '"
                  << security_key_connection_id << "'";
   }
 }
 
-void SecurityKeyAuthHandlerPosix::SendErrorAndCloseConnection(
-    int security_key_connection_id) {
-  ActiveSockets::const_iterator iter =
-      GetSocketForConnectionId(security_key_connection_id);
+void SecurityKeyAuthHandlerPosix::SendErrorAndCloseConnection(int id) {
+  ActiveSockets::iterator iter = active_sockets_.find(id);
   if (iter != active_sockets_.end()) {
     HOST_LOG << "Sending security key error";
     SendErrorAndCloseActiveSocket(iter);
   } else {
-    LOG(WARNING) << "Unknown gnubby-auth data connection: '"
-                 << security_key_connection_id << "'";
+    LOG(WARNING) << "Unknown gnubby-auth data connection: '" << id << "'";
   }
 }
 
 void SecurityKeyAuthHandlerPosix::SetSendMessageCallback(
     const SendMessageCallback& callback) {
   send_message_callback_ = callback;
 }
 
 size_t SecurityKeyAuthHandlerPosix::GetActiveConnectionCountForTest() const {
   return active_sockets_.size();
 }
 
 void SecurityKeyAuthHandlerPosix::SetRequestTimeoutForTest(
     base::TimeDelta timeout) {
   request_timeout_ = timeout;
 }
 
 void SecurityKeyAuthHandlerPosix::DoAccept() {
   DCHECK(thread_checker_.CalledOnValidThread());
   int result = auth_socket_->Accept(
       &accept_socket_, base::Bind(&SecurityKeyAuthHandlerPosix::OnAccepted,
                                   base::Unretained(this)));
-  if (result != net::ERR_IO_PENDING)
+  if (result != net::ERR_IO_PENDING) {
     OnAccepted(result);
+  }
 }
 
 void SecurityKeyAuthHandlerPosix::OnAccepted(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK_NE(net::ERR_IO_PENDING, result);
 
   if (result < 0) {
     LOG(ERROR) << "Error in accepting a new connection";
     return;
   }
 
   int security_key_connection_id = ++last_connection_id_;
   SecurityKeySocket* socket = new SecurityKeySocket(
       std::move(accept_socket_), request_timeout_,
       base::Bind(&SecurityKeyAuthHandlerPosix::RequestTimedOut,
                  base::Unretained(this), security_key_connection_id));
   active_sockets_[security_key_connection_id] = base::WrapUnique(socket);
   socket->StartReadingRequest(
       base::Bind(&SecurityKeyAuthHandlerPosix::OnReadComplete,
                  base::Unretained(this), security_key_connection_id));
 
   // Continue accepting new connections.
   DoAccept();
 }
 
-void SecurityKeyAuthHandlerPosix::OnReadComplete(
-    int security_key_connection_id) {
+void SecurityKeyAuthHandlerPosix::OnReadComplete(int connection_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  ActiveSockets::const_iterator iter =
-      active_sockets_.find(security_key_connection_id);
+  ActiveSockets::iterator iter = active_sockets_.find(connection_id);
   DCHECK(iter != active_sockets_.end());
   std::string request_data;
   if (!iter->second->GetAndClearRequestData(&request_data)) {
     SendErrorAndCloseActiveSocket(iter);
     return;
   }
 
   HOST_LOG << "Received security key request: " << GetCommandCode(request_data);
-  send_message_callback_.Run(security_key_connection_id, request_data);
+  send_message_callback_.Run(connection_id, request_data);
 
   iter->second->StartReadingRequest(
       base::Bind(&SecurityKeyAuthHandlerPosix::OnReadComplete,
-                 base::Unretained(this), security_key_connection_id));
+                 base::Unretained(this), connection_id));
 }
 
 SecurityKeyAuthHandlerPosix::ActiveSockets::const_iterator
 SecurityKeyAuthHandlerPosix::GetSocketForConnectionId(
     int security_key_connection_id) const {
   return active_sockets_.find(security_key_connection_id);
 }
 
 void SecurityKeyAuthHandlerPosix::SendErrorAndCloseActiveSocket(
-    const ActiveSockets::const_iterator& iter) {
-  iter->second->SendSshError();
+    const ActiveSockets::iterator& iter) {
+  iter->second->SendSshError(
+      base::Bind(&DeleteSocket, base::Passed(&iter->second)));

[Sergey Ulanov, 2016/12/07 20:41:36]

I don't think there is anything that guarantees that we will ever succeed to
send the error message, so it's possible that the the socket will never be
deleted, so I'm not sure that just releasing it here is the right thing to do.
Also it allows for other subtle bugs, e.g. can we be sure that SecurityKeySocket
will not call timeout callback later, after SecurityKeyAuthHandlerPosix is
destroyed?

[joedow, 2016/12/13 20:30:44]

That's true.  I had a follow-up CL which used this mechanism but I think this
isn't a great approach so I will scrap it for this CL and just fix the unit
tests and comments/code alignment.

   active_sockets_.erase(iter);
 }
 
-void SecurityKeyAuthHandlerPosix::RequestTimedOut(
-    int security_key_connection_id) {
+void SecurityKeyAuthHandlerPosix::RequestTimedOut(int connection_id) {
   HOST_LOG << "SecurityKey request timed out";
-  ActiveSockets::const_iterator iter =
-      active_sockets_.find(security_key_connection_id);
-  if (iter != active_sockets_.end())
+  ActiveSockets::iterator iter = active_sockets_.find(connection_id);
+  if (iter != active_sockets_.end()) {
     SendErrorAndCloseActiveSocket(iter);
+  }
 }
 
 }  // namespace remoting