Base: Remove Receive() from ScopedHandle.

In general, the OS API contract doesn't guarantee that output variables are not modified on failure, so a Reeceive pattern is fundamentally insecure. BUG=318531 TEST=current tests tbr'ing owners for the consumers. TBR=jvoung@chromium.org, thakis@chromium.org, sergeyu@chromium.org, grt@chromium.org, gene@chromium.org, youngki@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237459 Review URL: https://codereview.chromium.org/71013004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237675 0039d316-1c4b-4281-b951-d872f2087c98
hylschwy · Nov 28, 2013 · 5be06e4 · 5be06e4
1 parent 9679867
commit 5be06e4
Show file tree

Hide file tree

Showing 44 changed files with 226 additions and 242 deletions.
diff --git a/base/base.gyp b/base/base.gyp
@@ -680,7 +680,6 @@
         'win/registry_unittest.cc',
         'win/scoped_bstr_unittest.cc',
         'win/scoped_comptr_unittest.cc',
-        'win/scoped_handle_unittest.cc',
         'win/scoped_process_information_unittest.cc',
         'win/scoped_variant_unittest.cc',
         'win/shortcut_unittest.cc',

diff --git a/base/memory/shared_memory_unittest.cc b/base/memory/shared_memory_unittest.cc
@@ -425,16 +425,18 @@ TEST(SharedMemoryTest, ShareReadOnly) {
   EXPECT_EQ(NULL, MapViewOfFile(handle, FILE_MAP_WRITE, 0, 0, 0))
       << "Shouldn't be able to map memory writable.";
 
-  base::win::ScopedHandle writable_handle;
-  EXPECT_EQ(0,
-            ::DuplicateHandle(GetCurrentProcess(),
+  HANDLE temp_handle;
+  BOOL rv = ::DuplicateHandle(GetCurrentProcess(),
                               handle,
                               GetCurrentProcess,
-                              writable_handle.Receive(),
+                              &temp_handle,
                               FILE_MAP_ALL_ACCESS,
                               false,
-                              0))
+                              0);
+  EXPECT_EQ(FALSE, rv)
       << "Shouldn't be able to duplicate the handle into a writable one.";
+  if (rv)
+    base::win::ScopedHandle writable_handle(temp_handle);
 #else
 #error Unexpected platform; write a test that tries to make 'handle' writable.
 #endif  // defined(OS_POSIX) || defined(OS_WIN)

diff --git a/base/process/launch.h b/base/process/launch.h
@@ -21,6 +21,7 @@
 #include "base/posix/file_descriptor_shuffle.h"
 #elif defined(OS_WIN)
 #include <windows.h>
+#include "base/win/scoped_handle.h"
 #endif
 
 class CommandLine;
@@ -146,7 +147,7 @@ BASE_EXPORT bool LaunchProcess(const CommandLine& cmdline,
 //  cmdline = "c:\windows\explorer.exe" -foo "c:\bar\"
 BASE_EXPORT bool LaunchProcess(const string16& cmdline,
                                const LaunchOptions& options,
-                               ProcessHandle* process_handle);
+                               win::ScopedHandle* process_handle);
 
 #elif defined(OS_POSIX)
 // A POSIX-specific version of LaunchProcess that takes an argv array

diff --git a/base/process/launch_win.cc b/base/process/launch_win.cc
@@ -103,7 +103,7 @@ void RouteStdioToConsole() {
 
 bool LaunchProcess(const string16& cmdline,
                    const LaunchOptions& options,
-                   ProcessHandle* process_handle) {
+                   win::ScopedHandle* process_handle) {
   STARTUPINFO startup_info = {};
   startup_info.cb = sizeof(startup_info);
   if (options.empty_desktop_name)
@@ -136,7 +136,7 @@ bool LaunchProcess(const string16& cmdline,
   if (options.force_breakaway_from_job_)
     flags |= CREATE_BREAKAWAY_FROM_JOB;
 
-  base::win::ScopedProcessInformation process_info;
+  PROCESS_INFORMATION temp_process_info = {};
 
   if (options.as_user) {
     flags |= CREATE_UNICODE_ENVIRONMENT;
@@ -152,7 +152,7 @@ bool LaunchProcess(const string16& cmdline,
                             const_cast<wchar_t*>(cmdline.c_str()),
                             NULL, NULL, options.inherit_handles, flags,
                             enviroment_block, NULL, &startup_info,
-                            process_info.Receive());
+                            &temp_process_info);
     DestroyEnvironmentBlock(enviroment_block);
     if (!launched) {
       DPLOG(ERROR);
@@ -162,11 +162,12 @@ bool LaunchProcess(const string16& cmdline,
     if (!CreateProcess(NULL,
                        const_cast<wchar_t*>(cmdline.c_str()), NULL, NULL,
                        options.inherit_handles, flags, NULL, NULL,
-                       &startup_info, process_info.Receive())) {
+                       &startup_info, &temp_process_info)) {
       DPLOG(ERROR);
       return false;
     }
   }
+  base::win::ScopedProcessInformation process_info(temp_process_info);
 
   if (options.job_handle) {
     if (0 == AssignProcessToJobObject(options.job_handle,
@@ -184,15 +185,21 @@ bool LaunchProcess(const string16& cmdline,
 
   // If the caller wants the process handle, we won't close it.
   if (process_handle)
-    *process_handle = process_info.TakeProcessHandle();
+    process_handle->Set(process_info.TakeProcessHandle());
 
   return true;
 }
 
 bool LaunchProcess(const CommandLine& cmdline,
                    const LaunchOptions& options,
                    ProcessHandle* process_handle) {
-  return LaunchProcess(cmdline.GetCommandLineString(), options, process_handle);
+  if (!process_handle)
+    return LaunchProcess(cmdline.GetCommandLineString(), options, NULL);
+
+  win::ScopedHandle process;
+  bool rv = LaunchProcess(cmdline.GetCommandLineString(), options, &process);
+  *process_handle = process.Take();
+  return rv;
 }
 
 bool SetJobObjectLimitFlags(HANDLE job_object, DWORD limit_flags) {
@@ -233,8 +240,7 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
 
   FilePath::StringType writable_command_line_string(cl.GetCommandLineString());
 
-  base::win::ScopedProcessInformation proc_info;
-  STARTUPINFO start_info = { 0 };
+  STARTUPINFO start_info = {};
 
   start_info.cb = sizeof(STARTUPINFO);
   start_info.hStdOutput = out_write;
@@ -244,14 +250,16 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
   start_info.dwFlags |= STARTF_USESTDHANDLES;
 
   // Create the child process.
+  PROCESS_INFORMATION temp_process_info = {};
   if (!CreateProcess(NULL,
                      &writable_command_line_string[0],
                      NULL, NULL,
                      TRUE,  // Handles are inherited.
-                     0, NULL, NULL, &start_info, proc_info.Receive())) {
+                     0, NULL, NULL, &start_info, &temp_process_info)) {
     NOTREACHED() << "Failed to start process";
     return false;
   }
+  base::win::ScopedProcessInformation proc_info(temp_process_info);
 
   // Close our writing end of pipe now. Otherwise later read would not be able
   // to detect end of child's output.

diff --git a/base/win/scoped_handle.h b/base/win/scoped_handle.h
@@ -39,22 +39,6 @@ class GenericScopedHandle {
  public:
   typedef typename Traits::Handle Handle;
 
-  // Helper object to contain the effect of Receive() to the function that needs
-  // a pointer, and allow proper tracking of the handle.
-  class Receiver {
-   public:
-    explicit Receiver(GenericScopedHandle* owner)
-        : handle_(Traits::NullHandle()),
-          owner_(owner) {}
-    ~Receiver() { owner_->Set(handle_); }
-
-    operator Handle*() { return &handle_; }
-
-   private:
-    Handle handle_;
-    GenericScopedHandle* owner_;
-  };
-
   GenericScopedHandle() : handle_(Traits::NullHandle()) {}
 
   explicit GenericScopedHandle(Handle handle) : handle_(Traits::NullHandle()) {
@@ -102,16 +86,6 @@ class GenericScopedHandle {
     return handle_;
   }
 
-  // This method is intended to be used with functions that require a pointer to
-  // a destination handle, like so:
-  //    void CreateRequiredHandle(Handle* out_handle);
-  //    ScopedHandle a;
-  //    CreateRequiredHandle(a.Receive());
-  Receiver Receive() {
-    DCHECK(!Traits::IsHandleValid(handle_)) << "Handle must be NULL";
-    return Receiver(this);
-  }
-
   // Transfers ownership away from this object.
   Handle Take() {
     Handle temp = handle_;

diff --git a/base/win/scoped_handle_unittest.cc b/base/win/scoped_handle_unittest.cc
diff --git a/base/win/scoped_process_information.cc b/base/win/scoped_process_information.cc
@@ -15,7 +15,7 @@ namespace {
 // Duplicates source into target, returning true upon success. |target| is
 // guaranteed to be untouched in case of failure. Succeeds with no side-effects
 // if source is NULL.
-bool CheckAndDuplicateHandle(HANDLE source, HANDLE* target) {
+bool CheckAndDuplicateHandle(HANDLE source, ScopedHandle* target) {
   if (!source)
     return true;
 
@@ -26,7 +26,7 @@ bool CheckAndDuplicateHandle(HANDLE source, HANDLE* target) {
     DPLOG(ERROR) << "Failed to duplicate a handle.";
     return false;
   }
-  *target = temp;
+  target->Set(temp);
   return true;
 }
 
@@ -36,13 +36,13 @@ ScopedProcessInformation::ScopedProcessInformation()
     : process_id_(0), thread_id_(0) {
 }
 
-ScopedProcessInformation::~ScopedProcessInformation() {
-  Close();
+ScopedProcessInformation::ScopedProcessInformation(
+    const PROCESS_INFORMATION& process_info) : process_id_(0), thread_id_(0) {
+  Set(process_info);
 }
 
-ScopedProcessInformation::Receiver ScopedProcessInformation::Receive() {
-  DCHECK(!IsValid()) << "process_information_ must be NULL";
-  return Receiver(this);
+ScopedProcessInformation::~ScopedProcessInformation() {
+  Close();
 }
 
 bool ScopedProcessInformation::IsValid() const {
@@ -72,10 +72,8 @@ bool ScopedProcessInformation::DuplicateFrom(
   DCHECK(!IsValid()) << "target ScopedProcessInformation must be NULL";
   DCHECK(other.IsValid()) << "source ScopedProcessInformation must be valid";
 
-  if (CheckAndDuplicateHandle(other.process_handle(),
-                              process_handle_.Receive()) &&
-      CheckAndDuplicateHandle(other.thread_handle(),
-                              thread_handle_.Receive())) {
+  if (CheckAndDuplicateHandle(other.process_handle(), &process_handle_) &&
+      CheckAndDuplicateHandle(other.thread_handle(), &thread_handle_)) {
     process_id_ = other.process_id();
     thread_id_ = other.thread_id();
     return true;

diff --git a/base/win/scoped_process_information.h b/base/win/scoped_process_information.h
@@ -18,33 +18,10 @@ namespace win {
 // structures. Allows clients to take ownership of either handle independently.
 class BASE_EXPORT ScopedProcessInformation {
  public:
-  // Helper object to contain the effect of Receive() to the funtion that needs
-  // a pointer.
-  class Receiver {
-   public:
-    explicit Receiver(ScopedProcessInformation* owner)
-        : info_(),
-          owner_(owner) {}
-    ~Receiver() { owner_->Set(info_); }
-
-    operator PROCESS_INFORMATION*() { return &info_; }
-
-   private:
-    PROCESS_INFORMATION info_;
-    ScopedProcessInformation* owner_;
-  };
-
   ScopedProcessInformation();
+  explicit ScopedProcessInformation(const PROCESS_INFORMATION& process_info);
   ~ScopedProcessInformation();
 
-  // Returns an object that may be passed to API calls such as CreateProcess.
-  // DCHECKs that the object is not currently holding any handles.
-  // HANDLEs stored in the returned PROCESS_INFORMATION will be owned by this
-  // instance.
-  // The intended use case is something like this:
-  //   if (::CreateProcess(..., startup_info, scoped_proces_info.Receive()))
-  Receiver Receive();
-
   // Returns true iff this instance is holding a thread and/or process handle.
   bool IsValid() const;
 

diff --git a/base/win/scoped_process_information_unittest.cc b/base/win/scoped_process_information_unittest.cc
@@ -19,11 +19,13 @@ const DWORD kThreadId = 1234;
 const HANDLE kProcessHandle = reinterpret_cast<HANDLE>(7651);
 const HANDLE kThreadHandle = reinterpret_cast<HANDLE>(1567);
 
-void MockCreateProcess(PROCESS_INFORMATION* process_info) {
-  process_info->dwProcessId = kProcessId;
-  process_info->dwThreadId = kThreadId;
-  process_info->hProcess = kProcessHandle;
-  process_info->hThread = kThreadHandle;
+void MockCreateProcess(base::win::ScopedProcessInformation* process_info) {
+  PROCESS_INFORMATION process_information = {};
+  process_information.dwProcessId = kProcessId;
+  process_information.dwThreadId = kThreadId;
+  process_information.hProcess = kProcessHandle;
+  process_information.hThread = kThreadHandle;
+  process_info->Set(process_information);
 }
 
 }  // namespace
@@ -62,7 +64,7 @@ TEST_F(ScopedProcessInformationTest, InitiallyInvalid) {
 
 TEST_F(ScopedProcessInformationTest, Receive) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(process_info.Receive());
+  MockCreateProcess(&process_info);
 
   EXPECT_TRUE(process_info.IsValid());
   EXPECT_EQ(kProcessId, process_info.process_id());
@@ -74,7 +76,7 @@ TEST_F(ScopedProcessInformationTest, Receive) {
 
 TEST_F(ScopedProcessInformationTest, TakeProcess) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(process_info.Receive());
+  MockCreateProcess(&process_info);
 
   HANDLE process = process_info.TakeProcessHandle();
   EXPECT_EQ(kProcessHandle, process);
@@ -86,7 +88,7 @@ TEST_F(ScopedProcessInformationTest, TakeProcess) {
 
 TEST_F(ScopedProcessInformationTest, TakeThread) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(process_info.Receive());
+  MockCreateProcess(&process_info);
 
   HANDLE thread = process_info.TakeThreadHandle();
   EXPECT_EQ(kThreadHandle, thread);
@@ -98,7 +100,7 @@ TEST_F(ScopedProcessInformationTest, TakeThread) {
 
 TEST_F(ScopedProcessInformationTest, TakeBoth) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(process_info.Receive());
+  MockCreateProcess(&process_info);
 
   HANDLE process = process_info.TakeProcessHandle();
   HANDLE thread = process_info.TakeThreadHandle();
@@ -108,7 +110,7 @@ TEST_F(ScopedProcessInformationTest, TakeBoth) {
 
 TEST_F(ScopedProcessInformationTest, TakeWholeStruct) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(process_info.Receive());
+  MockCreateProcess(&process_info);
 
   PROCESS_INFORMATION to_discard = process_info.Take();
   EXPECT_EQ(kProcessId, to_discard.dwProcessId);
@@ -119,8 +121,11 @@ TEST_F(ScopedProcessInformationTest, TakeWholeStruct) {
 }
 
 TEST_F(ScopedProcessInformationTest, Duplicate) {
+  PROCESS_INFORMATION temp_process_information;
+  DoCreateProcess("ReturnSeven", &temp_process_information);
   base::win::ScopedProcessInformation process_info;
-  DoCreateProcess("ReturnSeven", process_info.Receive());
+  process_info.Set(temp_process_information);
+
   base::win::ScopedProcessInformation duplicate;
   duplicate.DuplicateFrom(process_info);
 
@@ -146,15 +151,17 @@ TEST_F(ScopedProcessInformationTest, Duplicate) {
 }
 
 TEST_F(ScopedProcessInformationTest, Set) {
-  PROCESS_INFORMATION base_process_info = {};
+  base::win::ScopedProcessInformation base_process_info;
   MockCreateProcess(&base_process_info);
 
+  PROCESS_INFORMATION base_struct = base_process_info.Take();
+
   base::win::ScopedProcessInformation process_info;
-  process_info.Set(base_process_info);
+  process_info.Set(base_struct);
 
   EXPECT_EQ(kProcessId, process_info.process_id());
   EXPECT_EQ(kThreadId, process_info.thread_id());
   EXPECT_EQ(kProcessHandle, process_info.process_handle());
   EXPECT_EQ(kThreadHandle, process_info.thread_handle());
-  base_process_info = process_info.Take();
+  base_struct = process_info.Take();
 }