IPC: Use ScopedHandle instead of a raw HANDLE for the private members.

ipc/ipc_channel_win.cc

 // Copyright (c) 2012 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 "ipc/ipc_channel_win.h"
 
 #include <windows.h>
 
 #include "base/auto_reset.h"
 #include "base/bind.h"
@@ skipping 41 matching lines @@
 ChannelWin::State::~State() {
   COMPILE_ASSERT(!offsetof(ChannelWin::State, context),
                  starts_with_io_context);
 }
 
 ChannelWin::ChannelWin(const IPC::ChannelHandle &channel_handle,
                        Mode mode, Listener* listener)
     : ChannelReader(listener),
       input_state_(this),
       output_state_(this),
-      pipe_(INVALID_HANDLE_VALUE),
       peer_pid_(base::kNullProcessId),
       waiting_connect_(mode & MODE_SERVER_FLAG),
       processing_incoming_(false),
       validate_client_(false),
       writing_(false),
       debug_flags_(0),
       write_error_(0),
       last_write_error_(0),
       write_size_(0),
       client_secret_(0),
       weak_factory_(this) {
   CreatePipe(channel_handle, mode);
 }
 
 ChannelWin::~ChannelWin() {
   Close();
 }
 
 void ChannelWin::Close() {
   if (thread_check_.get()) {
     DCHECK(thread_check_->CalledOnValidThread());
   }
   debug_flags_ |= CLOSED;
 
   if (input_state_.is_pending || output_state_.is_pending)
-    CancelIo(pipe_);
+    CancelIo(pipe_.Get());
 
   // Closing the handle at this point prevents us from issuing more requests
   // form OnIOCompleted().
-  if (pipe_ != INVALID_HANDLE_VALUE) {
-    CloseHandle(pipe_);
-    pipe_ = INVALID_HANDLE_VALUE;
-  }
+  if (pipe_.IsValid())
+    pipe_.Close();
 
   if (input_state_.is_pending)
     debug_flags_ |= WAIT_FOR_READ;
 
   if (output_state_.is_pending)
     debug_flags_ |= WAIT_FOR_WRITE;
 
   // Make sure all IO has completed.
   base::Time start = base::Time::Now();
   while (input_state_.is_pending || output_state_.is_pending) {
@@ skipping 30 matching lines @@
   return true;
 }
 
 base::ProcessId ChannelWin::GetPeerPID() const {
   return peer_pid_;
 }
 
 base::ProcessId ChannelWin::GetSelfPID() const {
   return GetCurrentProcessId();
 }
 

[cpu_(ooo_6.6-7.5), 2014/09/25 23:07:27]

morita has a lgtmed change here.

[rvargas (doing something else), 2014/09/25 23:26:20]

Yes, I know. This part is just for completeness.

 ChannelHandle ChannelWin::TakePipeHandle() {
-  ChannelHandle handle = ChannelHandle(pipe_);
-  pipe_ = INVALID_HANDLE_VALUE;
-  return handle;
+  return ChannelHandle(pipe_.Take());
 }
 
 // static
 bool ChannelWin::IsNamedServerInitialized(
     const std::string& channel_id) {
   if (WaitNamedPipe(PipeName(channel_id, NULL).c_str(), 1))
     return true;
   // If ERROR_SEM_TIMEOUT occurred, the pipe exists but is handling another
   // connection.
   return GetLastError() == ERROR_SEM_TIMEOUT;
 }
 
 ChannelWin::ReadState ChannelWin::ReadData(
     char* buffer,
     int buffer_len,
     int* /* bytes_read */) {
-  if (INVALID_HANDLE_VALUE == pipe_)
+  if (!pipe_.IsValid())
     return READ_FAILED;
 
   debug_flags_ |= READ_MSG;
   DWORD bytes_read = 0;
-  BOOL ok = ReadFile(pipe_, buffer, buffer_len,
+  BOOL ok = ReadFile(pipe_.Get(), buffer, buffer_len,
                      &bytes_read, &input_state_.context.overlapped);
   if (!ok) {
     DWORD err = GetLastError();
     if (err == ERROR_IO_PENDING) {
       input_state_.is_pending = true;
       return READ_PENDING;
     }
     LOG(ERROR) << "pipe error: " << err;
     return READ_FAILED;
   }
@@ skipping 60 matching lines @@
     return base::ASCIIToWide(name.append(channel_id.substr(0, index - 1)));
   }
 
   // This case is here to support predictable named pipes in tests.
   if (secret)
     *secret = 0;
   return base::ASCIIToWide(name.append(channel_id));
 }
 
 bool ChannelWin::CreatePipe(const IPC::ChannelHandle &channel_handle,
-                                      Mode mode) {
-  DCHECK_EQ(INVALID_HANDLE_VALUE, pipe_);
+                            Mode mode) {
+  DCHECK(!pipe_.IsValid());
   base::string16 pipe_name;
   // If we already have a valid pipe for channel just copy it.
   if (channel_handle.pipe.handle) {
+    // TODO(rvargas) crbug.com/415294: ChannelHandle should either go away in
+    // favor of two independent entities (name/file), or it should be a move-
+    // only type with a base::File member. In any case, this code should not

[cpu_(ooo_6.6-7.5), 2014/09/25 23:07:27]

is this justin thing I was talking about?

[rvargas (doing something else), 2014/09/25 23:26:20]

Nope. Amit added this long time ago... it's not used by Chrome proper, but it
has a few calls from remoting and cloud print.

+    // call DuplicateHandle.
     DCHECK(channel_handle.name.empty());
     pipe_name = L"Not Available";  // Just used for LOG
     // Check that the given pipe confirms to the specified mode.  We can
     // only check for PIPE_TYPE_MESSAGE & PIPE_SERVER_END flags since the
     // other flags (PIPE_TYPE_BYTE, and PIPE_CLIENT_END) are defined as 0.
     DWORD flags = 0;
     GetNamedPipeInfo(channel_handle.pipe.handle, &flags, NULL, NULL, NULL);
     DCHECK(!(flags & PIPE_TYPE_MESSAGE));
     if (((mode & MODE_SERVER_FLAG) && !(flags & PIPE_SERVER_END)) ||
         ((mode & MODE_CLIENT_FLAG) && (flags & PIPE_SERVER_END))) {
       LOG(WARNING) << "Inconsistent open mode. Mode :" << mode;
       return false;
     }
+    HANDLE local_handle;
     if (!DuplicateHandle(GetCurrentProcess(),
                          channel_handle.pipe.handle,
                          GetCurrentProcess(),
-                         &pipe_,
+                         &local_handle,
                          0,
                          FALSE,
                          DUPLICATE_SAME_ACCESS)) {
       LOG(WARNING) << "DuplicateHandle failed. Error :" << GetLastError();
       return false;
     }
+    pipe_.Set(local_handle);
   } else if (mode & MODE_SERVER_FLAG) {
     DCHECK(!channel_handle.pipe.handle);
     const DWORD open_mode = PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED |
                             FILE_FLAG_FIRST_PIPE_INSTANCE;
     pipe_name = PipeName(channel_handle.name, &client_secret_);
     validate_client_ = !!client_secret_;
-    pipe_ = CreateNamedPipeW(pipe_name.c_str(),
-                             open_mode,
-                             PIPE_TYPE_BYTE | PIPE_READMODE_BYTE,
-                             1,
-                             Channel::kReadBufferSize,
-                             Channel::kReadBufferSize,
-                             5000,
-                             NULL);
+    pipe_.Set(CreateNamedPipeW(pipe_name.c_str(),
+                               open_mode,
+                               PIPE_TYPE_BYTE | PIPE_READMODE_BYTE,
+                               1,
+                               Channel::kReadBufferSize,
+                               Channel::kReadBufferSize,
+                               5000,
+                               NULL));
   } else if (mode & MODE_CLIENT_FLAG) {
     DCHECK(!channel_handle.pipe.handle);
     pipe_name = PipeName(channel_handle.name, &client_secret_);
-    pipe_ = CreateFileW(pipe_name.c_str(),
-                        GENERIC_READ | GENERIC_WRITE,
-                        0,
-                        NULL,
-                        OPEN_EXISTING,
-                        SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION |
-                            FILE_FLAG_OVERLAPPED,
-                        NULL);
+    pipe_.Set(CreateFileW(pipe_name.c_str(),
+                          GENERIC_READ | GENERIC_WRITE,
+                          0,
+                          NULL,
+                          OPEN_EXISTING,
+                          SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION |
+                              FILE_FLAG_OVERLAPPED,
+                          NULL));
   } else {
     NOTREACHED();
   }
 
-  if (pipe_ == INVALID_HANDLE_VALUE) {
+  if (!pipe_.IsValid()) {
     // If this process is being closed, the pipe may be gone already.
     PLOG(WARNING) << "Unable to create pipe \"" << pipe_name << "\" in "
                   << (mode & MODE_SERVER_FLAG ? "server" : "client") << " mode";
     return false;
   }
 
   // Create the Hello message to be sent when Connect is called
   scoped_ptr<Message> m(new Message(MSG_ROUTING_NONE,
                                     HELLO_MESSAGE_TYPE,
                                     IPC::Message::PRIORITY_NORMAL));
 
   // Don't send the secret to the untrusted process, and don't send a secret
   // if the value is zero (for IPC backwards compatability).
   int32 secret = validate_client_ ? 0 : client_secret_;
   if (!m->WriteInt(GetCurrentProcessId()) ||
       (secret && !m->WriteUInt32(secret))) {
-    CloseHandle(pipe_);
-    pipe_ = INVALID_HANDLE_VALUE;
+    pipe_.Close();
     return false;
   }
 
   debug_flags_ |= INIT_DONE;
 
   output_queue_.push(m.release());
   return true;
 }
 
 bool ChannelWin::Connect() {
   DLOG_IF(WARNING, thread_check_.get()) << "Connect called more than once";
 
   if (!thread_check_.get())
     thread_check_.reset(new base::ThreadChecker());
 
-  if (pipe_ == INVALID_HANDLE_VALUE)
+  if (!pipe_.IsValid())
     return false;
 
-  base::MessageLoopForIO::current()->RegisterIOHandler(pipe_, this);
+  base::MessageLoopForIO::current()->RegisterIOHandler(pipe_.Get(), this);
 
   // Check to see if there is a client connected to our pipe...
   if (waiting_connect_)
     ProcessConnection();
 
   if (!input_state_.is_pending) {
     // Complete setup asynchronously. By not setting input_state_.is_pending
     // to true, we indicate to OnIOCompleted that this is the special
     // initialization signal.
     base::MessageLoopForIO::current()->PostTask(
         FROM_HERE,
         base::Bind(&ChannelWin::OnIOCompleted,
                    weak_factory_.GetWeakPtr(),
                    &input_state_.context,
                    0,
                    0));
   }
 
   if (!waiting_connect_)
     ProcessOutgoingMessages(NULL, 0);
   return true;
 }
 
 bool ChannelWin::ProcessConnection() {
   DCHECK(thread_check_->CalledOnValidThread());
   if (input_state_.is_pending)
     input_state_.is_pending = false;
 
   // Do we have a client connected to our pipe?
-  if (INVALID_HANDLE_VALUE == pipe_)
+  if (!pipe_.IsValid())
     return false;
 
-  BOOL ok = ConnectNamedPipe(pipe_, &input_state_.context.overlapped);
+  BOOL ok = ConnectNamedPipe(pipe_.Get(), &input_state_.context.overlapped);
   debug_flags_ |= CALLED_CONNECT;
 
   DWORD err = GetLastError();
   if (ok) {
     // Uhm, the API documentation says that this function should never
     // return success when used in overlapped mode.
     NOTREACHED();
     return false;
   }
 
@@ skipping 35 matching lines @@
     // Message was sent.
     CHECK(!output_queue_.empty());
     Message* m = output_queue_.front();
     output_queue_.pop();
     delete m;
   }
 
   if (output_queue_.empty())
     return true;
 
-  if (INVALID_HANDLE_VALUE == pipe_)
+  if (!pipe_.IsValid())
     return false;
 
   // Write to pipe...
   Message* m = output_queue_.front();
   DCHECK(m->size() <= INT_MAX);
   debug_flags_ |= WRITE_MSG;
   CHECK(!writing_);
   writing_ = true;
   write_size_ = static_cast<uint32>(m->size());
   write_error_ = 0;
-  BOOL ok = WriteFile(pipe_,
+  BOOL ok = WriteFile(pipe_.Get(),
                       m->data(),
                       write_size_,
                       NULL,
                       &output_state_.context.overlapped);
   if (!ok) {
     write_error_ = GetLastError();
     if (write_error_ == ERROR_IO_PENDING) {
       output_state_.is_pending = true;
 
       DVLOG(2) << "sent pending message @" << m << " on channel @" << this
@@ skipping 42 matching lines @@
     if (input_state_.is_pending) {
       // This is the normal case for everything except the initialization step.
       debug_flags_ |= READ_COMPLETED;
       if (debug_flags_ & WAIT_FOR_READ) {
         CHECK(!(debug_flags_ & WAIT_FOR_READ_COMPLETE));
         debug_flags_ |= WAIT_FOR_READ_COMPLETE;
       }
       input_state_.is_pending = false;
       if (!bytes_transfered)
         ok = false;
-      else if (pipe_ != INVALID_HANDLE_VALUE)
+      else if (pipe_.IsValid())
         ok = AsyncReadComplete(bytes_transfered);
     } else {
       DCHECK(!bytes_transfered);
     }
 
     // Request more data.
     if (ok)
       ok = ProcessIncomingMessages();
   } else {
     DCHECK(context == &output_state_.context);
     CHECK(writing_);
     CHECK(output_state_.is_pending);
     writing_ = false;
     debug_flags_ |= WRITE_COMPLETED;
     if (debug_flags_ & WAIT_FOR_WRITE) {
       CHECK(!(debug_flags_ & WAIT_FOR_WRITE_COMPLETE));
       debug_flags_ |= WAIT_FOR_WRITE_COMPLETE;
     }
     ok = ProcessOutgoingMessages(context, bytes_transfered);
   }
-  if (!ok && INVALID_HANDLE_VALUE != pipe_) {
+  if (!ok && pipe_.IsValid()) {
     // We don't want to re-enter Close().
     Close();
     listener()->OnChannelError();
   }
 }
 
 //------------------------------------------------------------------------------
 // Channel's methods
 
 // static
@@ skipping 22 matching lines @@
   int secret;
   do {  // Guarantee we get a non-zero value.
     secret = base::RandInt(0, std::numeric_limits<int>::max());
   } while (secret == 0);
 
   id.append(GenerateUniqueRandomChannelID());
   return id.append(base::StringPrintf("\\%d", secret));
 }
 
 }  // namespace IPC