mesos git commit: Fixed invalid handle bug in `os::process()`.

[jp...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master 01b7041fa -> 11de04c1b


Fixed invalid handle bug in `os::process()`.

This patch fixes a bug found by running `mesos-tests` under Application
Verifier on Windows. Mesos was inadvertently attempting to get a process
handle for the System Idle Process with PID `0`, which is not permitted
by the OS. To remedy this, we now check if `os::process` receives `0`
for its argument, and return an error if so. Furthermore, we remove the
PID `0` from the `os::pids` API, as it is not useful to the programmer,
and only serves to cause errors later. Finally, the return value for
`OpenProcess` was being incorrectly checked, as the API returns a
`nullptr` on failure, not `INVALID_HANDLE_VALUE`.

Review: https://reviews.apache.org/r/62391/


Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/11de04c1
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/11de04c1
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/11de04c1

Branch: refs/heads/master
Commit: 11de04c1bbf65d0a90576642f8ea9574e05a317a
Parents: 01b7041
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Tue Sep 19 17:19:59 2017 -0700
Committer: James Peach <jp...@apache.org>
Committed: Tue Sep 19 17:19:59 2017 -0700

----------------------------------------------------------------------
 3rdparty/stout/include/stout/windows/os.hpp | 21 +++++++++++++++++++--
 3rdparty/stout/tests/os/process_tests.cpp   | 13 +++++++++++--
 2 files changed, 30 insertions(+), 4 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/11de04c1/3rdparty/stout/include/stout/windows/os.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/windows/os.hpp b/3rdparty/stout/include/stout/windows/os.hpp
index f35bf31..a70a61c 100644
--- a/3rdparty/stout/include/stout/windows/os.hpp
+++ b/3rdparty/stout/include/stout/windows/os.hpp
@@ -179,7 +179,16 @@ inline Try<std::set<pid_t>> pids(Option<pid_t> group, Option<pid_t> session)
     max_items *= 2;
   } while (bytes_returned >= size_in_bytes);
 
-  return std::set<pid_t>(processes.begin(), processes.end());
+  std::set<pid_t> pids_set(processes.begin(), processes.end());
+
+  // NOTE: The PID `0` will always be returned by `EnumProcesses`; however, it
+  // is the PID of Windows' System Idle Process. While the PID is valid, using
+  // it for anything is almost always invalid. For instance, `OpenProcess` will
+  // fail with an invalid parameter error if the user tries to get a handle for
+  // PID `0`. In the interest of safety, we prevent the `pids` API from ever
+  // including the PID `0`.
+  pids_set.erase(0);
+  return pids_set;
 }
 
 
@@ -534,6 +543,13 @@ inline Result<PROCESSENTRY32W> process_entry(pid_t pid)
 // something went wrong.
 inline Result<Process> process(pid_t pid)
 {
+  if (pid == 0) {
+    // The 0th PID is that of the System Idle Process on Windows. However, it is
+    // invalid to attempt to get a proces handle or else perform any operation
+    // on this pseudo-process.
+    return Error("os::process: Invalid parameter: pid == 0");
+  }
+
   // Find process with pid.
   Result<PROCESSENTRY32W> entry = process_entry(pid);
 
@@ -548,7 +564,8 @@ inline Result<Process> process(pid_t pid)
       false,
       pid);
 
-  if (process_handle == INVALID_HANDLE_VALUE) {
+  // ::OpenProcess returns `NULL`, not `INVALID_HANDLE_VALUE` on failure.
+  if (process_handle == nullptr) {
     return WindowsError("os::process: Call to `OpenProcess` failed");
   }
 

http://git-wip-us.apache.org/repos/asf/mesos/blob/11de04c1/3rdparty/stout/tests/os/process_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/tests/os/process_tests.cpp b/3rdparty/stout/tests/os/process_tests.cpp
index 963eb4e..cbae66c 100644
--- a/3rdparty/stout/tests/os/process_tests.cpp
+++ b/3rdparty/stout/tests/os/process_tests.cpp
@@ -44,6 +44,7 @@ using std::string;
 
 const unsigned int init_pid =
 #ifdef __WINDOWS__
+    // NOTE: This is also known as the System Idle Process.
     0;
 #else
     1;
@@ -105,8 +106,9 @@ TEST_F(ProcessTest, Process)
   // Assert init.
   Result<Process> init_process = os::process(init_pid);
 #ifdef __WINDOWS__
-  // NOTE: On Windows, inspecting other processes usually requires privileges.
-  // So we expect it to error out instead of succeed, unlike the POSIX version.
+  // NOTE: On Windows, the init process is a pseudo-process, and it is not
+  // possible to get a handle to it. So we expect it to error out instead of
+  // succeed, unlike the POSIX version.
   EXPECT_ERROR(init_process);
 #elif __FreeBSD__
   // In a FreeBSD jail, we wont find an init process.
@@ -173,7 +175,14 @@ TEST_F(ProcessTest, Pids)
 #ifdef __FreeBSD__
   if (!isJailed()) {
 #endif
+#ifdef __WINDOWS__
+    // On Windows, we explicitly do not return the PID of the System Idle
+    // Process, because doing so can cause unexpected bugs.
+    EXPECT_EQ(0u, pids.get().count(init_pid));
+#else
+    // Elsewhere we always expect exactly 1 init PID.
     EXPECT_EQ(1u, pids.get().count(init_pid));
+#endif
 #ifdef __FreeBSD__
   }
 #endif