Removing 'AllowScopedIO' exception from SecurityKeyAuthHandlerLinux

remoting/host/security_key/security_key_auth_handler_linux.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 <stdint.h>
 #include <unistd.h>
 
+#include <cstdint>
+#include <map>
 #include <memory>
+#include <string>
+#include <utility>
 
 #include "base/bind.h"
+#include "base/callback.h"
+#include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
+#include "base/location.h"
 #include "base/logging.h"
-#include "base/stl_util.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/single_thread_task_runner.h"
 #include "base/threading/thread_checker.h"
-#include "base/threading/thread_restrictions.h"
-#include "base/values.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_errors.h"
 #include "net/socket/stream_socket.h"
 #include "net/socket/unix_domain_server_socket_posix.h"
 #include "remoting/base/logging.h"
 #include "remoting/host/security_key/security_key_socket.h"
 
 namespace {
 
 const int64_t kDefaultRequestTimeoutSeconds = 60;
 
 // The name of the socket to listen for security key requests on.
 base::LazyInstance<base::FilePath>::Leaky g_security_key_socket_name =
     LAZY_INSTANCE_INITIALIZER;
 
+void DeleteExistingSocketFile() {
+  // If the file already exists, a socket in use error is returned.
+  base::DeleteFile(g_security_key_socket_name.Get(), false);
+}
+
 // Socket authentication function that only allows connections from callers with
 // the current uid.
 bool MatchUid(const net::UnixDomainServerSocket::Credentials& credentials) {
   bool allowed = credentials.user_id == getuid();
   if (!allowed)
     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]);
 }
 
 }  // namespace
 
 namespace remoting {
 
 class SecurityKeyAuthHandlerLinux : public SecurityKeyAuthHandler {
  public:
-  SecurityKeyAuthHandlerLinux();
+  explicit SecurityKeyAuthHandlerLinux(
+      scoped_refptr<base::SingleThreadTaskRunner> file_task_runner);
   ~SecurityKeyAuthHandlerLinux() override;
 
  private:
-  typedef std::map<int, SecurityKeySocket*> ActiveSockets;
+  typedef std::map<int, std::unique_ptr<SecurityKeySocket>> ActiveSockets;
 
   // SecurityKeyAuthHandler interface.
   void CreateSecurityKeyConnection() override;
   bool IsValidConnectionId(int security_key_connection_id) const override;
   void SendClientResponse(int security_key_connection_id,
                           const std::string& response) override;
   void SendErrorAndCloseConnection(int security_key_connection_id) override;
   void SetSendMessageCallback(const SendMessageCallback& callback) override;
   size_t GetActiveConnectionCountForTest() const override;
   void SetRequestTimeoutForTest(base::TimeDelta timeout) override;
 
+  // Sets up the socket used for accepting new connections.
+  void CreateSocket();
+
   // Starts listening for connection.
   void DoAccept();
 
   // Called when a connection is accepted.
   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|.
@@ skipping 12 matching lines @@
   // Socket used to listen for authorization requests.
   std::unique_ptr<net::UnixDomainServerSocket> auth_socket_;
 
   // A temporary holder for an accepted connection.
   std::unique_ptr<net::StreamSocket> accept_socket_;
 
   // Used to pass security key extension messages to the client.
   SendMessageCallback send_message_callback_;
 
   // The last assigned security key connection id.
-  int last_connection_id_;
+  int last_connection_id_ = 0;
 
   // Sockets by connection id used to process gnubbyd requests.
   ActiveSockets active_sockets_;
 
+  // Used to perform File IO via Unix Domain Sockets.
+  scoped_refptr<base::SingleThreadTaskRunner> file_task_runner_;
+
   // Timeout used for a request.
   base::TimeDelta request_timeout_;
 
+  base::WeakPtrFactory<SecurityKeyAuthHandlerLinux> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(SecurityKeyAuthHandlerLinux);
 };
 
 std::unique_ptr<SecurityKeyAuthHandler> SecurityKeyAuthHandler::Create(
     ClientSessionDetails* client_session_details,
-    const SendMessageCallback& send_message_callback) {
+    const SendMessageCallback& send_message_callback,
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner) {
   std::unique_ptr<SecurityKeyAuthHandler> auth_handler(
-      new SecurityKeyAuthHandlerLinux());
+      new SecurityKeyAuthHandlerLinux(file_task_runner));
   auth_handler->SetSendMessageCallback(send_message_callback);
   return auth_handler;
 }
 
 void SecurityKeyAuthHandler::SetSecurityKeySocketName(
     const base::FilePath& security_key_socket_name) {
   g_security_key_socket_name.Get() = security_key_socket_name;
 }
 
-SecurityKeyAuthHandlerLinux::SecurityKeyAuthHandlerLinux()
-    : last_connection_id_(0),
+SecurityKeyAuthHandlerLinux::SecurityKeyAuthHandlerLinux(
+    scoped_refptr<base::SingleThreadTaskRunner> file_task_runner)
+    : file_task_runner_(file_task_runner),
       request_timeout_(
-          base::TimeDelta::FromSeconds(kDefaultRequestTimeoutSeconds)) {}
+          base::TimeDelta::FromSeconds(kDefaultRequestTimeoutSeconds)),
+      weak_factory_(this) {}
 
-SecurityKeyAuthHandlerLinux::~SecurityKeyAuthHandlerLinux() {
-  STLDeleteValues(&active_sockets_);
-}
+SecurityKeyAuthHandlerLinux::~SecurityKeyAuthHandlerLinux() {}
 
 void SecurityKeyAuthHandlerLinux::CreateSecurityKeyConnection() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!g_security_key_socket_name.Get().empty());
 
-  {
-    // DeleteFile() is a blocking operation, but so is creation of the unix
-    // socket below. Consider moving this class to a different thread if this
-    // causes any problems. See crbug.com/509807.
-    // TODO(joedow): Since this code now runs as a host extension, we should
-    //               perform our IO on a separate thread: crbug.com/591739
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
+  file_task_runner_->PostTaskAndReply(

[Sergey Ulanov, 2016/07/25 18:29:55]

add a comment to explain why this is necessary.

[joedow, 2016/07/25 22:39:25]

Done.

+      FROM_HERE, base::Bind(&DeleteExistingSocketFile),

[Sergey Ulanov, 2016/07/25 18:29:55]

nit: here you can post task for base::DeleteFile(), then you wouldn't need
DeleteExistingSocketFile().

[joedow, 2016/07/25 22:39:25]

Done.

+      base::Bind(&SecurityKeyAuthHandlerLinux::CreateSocket,
+                 weak_factory_.GetWeakPtr()));
+}
 
-    // If the file already exists, a socket in use error is returned.
-    base::DeleteFile(g_security_key_socket_name.Get(), false);
-  }
-
+void SecurityKeyAuthHandlerLinux::CreateSocket() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   HOST_LOG << "Listening for security key requests on "
            << g_security_key_socket_name.Get().value();
 
   auth_socket_.reset(
       new net::UnixDomainServerSocket(base::Bind(MatchUid), false));
   int rv = auth_socket_->BindAndListen(g_security_key_socket_name.Get().value(),
                                        /*backlog=*/1);
   if (rv != net::OK) {
     LOG(ERROR) << "Failed to open socket for auth requests: '" << rv << "'";
     return;
@@ skipping 41 matching lines @@
 size_t SecurityKeyAuthHandlerLinux::GetActiveConnectionCountForTest() const {
   return active_sockets_.size();
 }
 
 void SecurityKeyAuthHandlerLinux::SetRequestTimeoutForTest(
     base::TimeDelta timeout) {
   request_timeout_ = timeout;
 }
 
 void SecurityKeyAuthHandlerLinux::DoAccept() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   int result = auth_socket_->Accept(
       &accept_socket_, base::Bind(&SecurityKeyAuthHandlerLinux::OnAccepted,
-                                  base::Unretained(this)));
+                                  weak_factory_.GetWeakPtr()));
   if (result != net::ERR_IO_PENDING)
     OnAccepted(result);
 }
 
 void SecurityKeyAuthHandlerLinux::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(&SecurityKeyAuthHandlerLinux::RequestTimedOut,
-                 base::Unretained(this), security_key_connection_id));
-  active_sockets_[security_key_connection_id] = socket;
+                 weak_factory_.GetWeakPtr(), security_key_connection_id));

[Sergey Ulanov, 2016/07/25 18:29:55]

nit: don't really need to use weak-ptrs here because this object owns the
socket, but feel free to keep it for consistency.

[joedow, 2016/07/25 22:39:25]

Done.  I was thinking this would be safer in case something was changed later,
but the important methods already use a thread checker so this isn't as helpful
as I had first thought.

+  active_sockets_[security_key_connection_id] = base::WrapUnique(socket);
   socket->StartReadingRequest(
       base::Bind(&SecurityKeyAuthHandlerLinux::OnReadComplete,
-                 base::Unretained(this), security_key_connection_id));
+                 weak_factory_.GetWeakPtr(), security_key_connection_id));
 
   // Continue accepting new connections.
   DoAccept();
 }
 
 void SecurityKeyAuthHandlerLinux::OnReadComplete(
     int security_key_connection_id) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   ActiveSockets::const_iterator iter =
       active_sockets_.find(security_key_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);
 
   iter->second->StartReadingRequest(
       base::Bind(&SecurityKeyAuthHandlerLinux::OnReadComplete,
-                 base::Unretained(this), security_key_connection_id));
+                 weak_factory_.GetWeakPtr(), security_key_connection_id));
 }
 
 SecurityKeyAuthHandlerLinux::ActiveSockets::const_iterator
 SecurityKeyAuthHandlerLinux::GetSocketForConnectionId(
     int security_key_connection_id) const {
   return active_sockets_.find(security_key_connection_id);
 }
 
 void SecurityKeyAuthHandlerLinux::SendErrorAndCloseActiveSocket(
     const ActiveSockets::const_iterator& iter) {
   iter->second->SendSshError();
-  delete iter->second;
   active_sockets_.erase(iter);
 }
 
 void SecurityKeyAuthHandlerLinux::RequestTimedOut(
     int security_key_connection_id) {
   HOST_LOG << "SecurityKey request timed out";
   ActiveSockets::const_iterator iter =
       active_sockets_.find(security_key_connection_id);
   if (iter != active_sockets_.end())
     SendErrorAndCloseActiveSocket(iter);
 }
 
 }  // namespace remoting