mesos git commit: Ported `EnvTest.EraseEnv` to Windows, with added comments.

[an...@apache.org]

Repository: mesos
Updated Branches:
  refs/heads/master b1e88f73b -> abef72521


Ported `EnvTest.EraseEnv` to Windows, with added comments.

The difference between `os::unsetenv` and `os::eraseenv` is important,
but subtle. Now the reason for the existence of `os::eraseenv` is
documented in comments.

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


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

Branch: refs/heads/master
Commit: abef72521a9a74fe4d6e7c796d6c99f02ad811dd
Parents: b1e88f7
Author: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Authored: Thu Jun 7 15:53:06 2018 -0700
Committer: Andrew Schwartzmeyer <an...@schwartzmeyer.com>
Committed: Mon Jun 11 15:32:03 2018 -0700

----------------------------------------------------------------------
 3rdparty/stout/include/stout/posix/os.hpp   | 14 ++++++++++----
 3rdparty/stout/include/stout/windows/os.hpp |  5 ++++-
 3rdparty/stout/tests/os/env_tests.cpp       | 18 ++++++++++++++----
 3 files changed, 28 insertions(+), 9 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/mesos/blob/abef7252/3rdparty/stout/include/stout/posix/os.hpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/include/stout/posix/os.hpp b/3rdparty/stout/include/stout/posix/os.hpp
index f898f07..91ef5bc 100644
--- a/3rdparty/stout/include/stout/posix/os.hpp
+++ b/3rdparty/stout/include/stout/posix/os.hpp
@@ -166,18 +166,24 @@ inline void setenv(const std::string& key,
 // environment variables.
 inline void unsetenv(const std::string& key)
 {
+  // NOTE: This function explicitly does not clear from
+  // `/proc/$pid/environ` because that is defined to be the initial
+  // environment that was set when the process was started, so don't
+  // use this to delete secrets! Instead, see `os::eraseenv`.
   ::unsetenv(key.c_str());
 }
 
 
 // Erases the value associated with the specified key from the set of
-// environment variables.
+// environment variables. By erase, we even clear it from
+// `/proc/$pid/environ` so that sensitive information conveyed via
+// environment variables (such as secrets) can be cleaned up.
 inline void eraseenv(const std::string& key)
 {
-  char * value = ::getenv(key.c_str());
+  char* value = ::getenv(key.c_str());
 
-  // Erase the old value so that on Linux it can't be inspected through
-  // /proc/$pid/environ.
+  // Erase the old value so that on Linux it can't be inspected
+  // through `/proc/$pid/environ`, useful for secrets.
   if (value) {
     ::memset(value, '\0', ::strlen(value));
   }

http://git-wip-us.apache.org/repos/asf/mesos/blob/abef7252/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 3a728f8..46cd667 100644
--- a/3rdparty/stout/include/stout/windows/os.hpp
+++ b/3rdparty/stout/include/stout/windows/os.hpp
@@ -172,7 +172,10 @@ inline void unsetenv(const std::string& key)
 }
 
 
-// NOTE: This exists for compatibility with the POSIX API.
+// NOTE: This exists for compatibility with the POSIX API. On Windows,
+// either function is suitable for clearing a secret from the
+// environment, as the pointers returned by `GetEnvironmentVariable`
+// and `GetEnvironmentStrings` are to blocks allocated on invocation.
 inline void eraseenv(const std::string& key)
 {
   unsetenv(key);

http://git-wip-us.apache.org/repos/asf/mesos/blob/abef7252/3rdparty/stout/tests/os/env_tests.cpp
----------------------------------------------------------------------
diff --git a/3rdparty/stout/tests/os/env_tests.cpp b/3rdparty/stout/tests/os/env_tests.cpp
index b5b124d..f1ac392 100644
--- a/3rdparty/stout/tests/os/env_tests.cpp
+++ b/3rdparty/stout/tests/os/env_tests.cpp
@@ -13,6 +13,7 @@
 #include <gtest/gtest.h>
 
 #include <stout/os.hpp>
+#include <stout/os/environment.hpp>
 
 #include <stout/tests/utils.hpp>
 
@@ -64,27 +65,36 @@ TEST(EnvTest, SimpleEnvTest)
 }
 
 
-#ifndef __WINDOWS__
 TEST(EnvTest, EraseEnv)
 {
   os::setenv("key", "value");
 
   std::map<string, string> pre = os::environment();
 
+#ifdef __WINDOWS__
+  Option<string> value = os::getenv("key");
+  EXPECT_SOME_EQ("value", value);
+#else
   // We use ::getenv rather than `os::getenv` so that we can
   // keep the pointer across the `os::eraseenv` and verify that
   // `os::eraseenv` clears existing values rather than just
   // making them unavailable.
-  char * value = ::getenv("key");
+  char* value = ::getenv("key");
   EXPECT_STREQ("value", value);
+#endif // __WINDOWS__
 
   os::eraseenv("key");
+
+#ifndef __WINDOWS__
+  // On POSIX, we check that the pointer itself was cleared. This does
+  // not apply to Windows.
   EXPECT_STREQ("", value);
+#endif // __WINDOWS__
+
   EXPECT_NONE(os::getenv("key"));
 
   // Verify that erasing "key" removed the environment variable without
-  // damaging any other ernvironment variables.
+  // damaging any other environment variables.
   pre.erase("key");
   EXPECT_EQ(pre, os::environment());
 }
-#endif // __WINDOWS__