From 84a80b3b332a23c64a5822ea966c3e3462cfae17 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Fri, 1 Jun 2018 15:06:10 -0700 Subject: [PATCH 1/4] Repair hermetic_environment_as for C-level environment variable access. --- src/python/pants/util/contextutil.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/python/pants/util/contextutil.py b/src/python/pants/util/contextutil.py index d904ea2c8a4..df98346d34e 100644 --- a/src/python/pants/util/contextutil.py +++ b/src/python/pants/util/contextutil.py @@ -76,15 +76,31 @@ def setenv(key, val): setenv(key, val) +def _copy_and_purge_env(): + # N.B. Without the use of `del` here (which calls `os.unsetenv` under the hood), subprocess32 + # invokes or other things that may access the environment at the C level may not see the + # correct env vars (i.e. we can't just replace os.environ with an empty dict). + # See https://docs.python.org/2/library/os.html#os.unsetenv for more info. + original = os.environ.copy() + for k in os.environ.keys(): + del os.environ[k] + return original + + +def _restore_env(env): + for k, v in env.items(): + os.environ[k] = v + + @contextmanager def hermetic_environment_as(**kwargs): """Set the environment to the supplied values from an empty state.""" - old_environment, os.environ = os.environ, {} + old_environment = _copy_and_purge_env() try: with environment_as(**kwargs): yield finally: - os.environ = old_environment + _restore_env(old_environment) @contextmanager From e423fdf6105a6703895a4817010e20271f476579 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Fri, 1 Jun 2018 15:12:01 -0700 Subject: [PATCH 2/4] Test. --- tests/python/pants_test/util/test_contextutil.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/python/pants_test/util/test_contextutil.py b/tests/python/pants_test/util/test_contextutil.py index 2a018636647..bae5a257972 100644 --- a/tests/python/pants_test/util/test_contextutil.py +++ b/tests/python/pants_test/util/test_contextutil.py @@ -64,6 +64,12 @@ def test_hermetic_environment(self): with hermetic_environment_as(**{}): self.assertNotIn('USER', os.environ) + def test_hermetic_environment_subprocesses(self): + self.assertIn('USER', os.environ) + with hermetic_environment_as(**{}): + output = subprocess.check_output('env', shell=True) + self.assertNotIn('USER=', output) + def test_simple_pushd(self): pre_cwd = os.getcwd() with temporary_dir() as tempdir: From 1f29df6c10c03ff2d0191eaecd2c31c2a4474da2 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Fri, 1 Jun 2018 15:21:00 -0700 Subject: [PATCH 3/4] Avoid mutations during env restoration. --- src/python/pants/util/contextutil.py | 8 ++++---- tests/python/pants_test/util/test_contextutil.py | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/python/pants/util/contextutil.py b/src/python/pants/util/contextutil.py index df98346d34e..bf6c3baeada 100644 --- a/src/python/pants/util/contextutil.py +++ b/src/python/pants/util/contextutil.py @@ -76,15 +76,13 @@ def setenv(key, val): setenv(key, val) -def _copy_and_purge_env(): +def _purge_env(): # N.B. Without the use of `del` here (which calls `os.unsetenv` under the hood), subprocess32 # invokes or other things that may access the environment at the C level may not see the # correct env vars (i.e. we can't just replace os.environ with an empty dict). # See https://docs.python.org/2/library/os.html#os.unsetenv for more info. - original = os.environ.copy() for k in os.environ.keys(): del os.environ[k] - return original def _restore_env(env): @@ -95,11 +93,13 @@ def _restore_env(env): @contextmanager def hermetic_environment_as(**kwargs): """Set the environment to the supplied values from an empty state.""" - old_environment = _copy_and_purge_env() + old_environment = os.environ.copy() + _purge_env() try: with environment_as(**kwargs): yield finally: + _purge_env() _restore_env(old_environment) diff --git a/tests/python/pants_test/util/test_contextutil.py b/tests/python/pants_test/util/test_contextutil.py index bae5a257972..58bd7ca294d 100644 --- a/tests/python/pants_test/util/test_contextutil.py +++ b/tests/python/pants_test/util/test_contextutil.py @@ -69,6 +69,7 @@ def test_hermetic_environment_subprocesses(self): with hermetic_environment_as(**{}): output = subprocess.check_output('env', shell=True) self.assertNotIn('USER=', output) + self.assertIn('USER', os.environ) def test_simple_pushd(self): pre_cwd = os.getcwd() From 0a78d6af0b9854c5306b22ed08160ad3872ab5b4 Mon Sep 17 00:00:00 2001 From: Kris Wilson Date: Fri, 1 Jun 2018 15:27:00 -0700 Subject: [PATCH 4/4] Harden test. --- tests/python/pants_test/util/test_contextutil.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/python/pants_test/util/test_contextutil.py b/tests/python/pants_test/util/test_contextutil.py index 58bd7ca294d..03b1cda71b9 100644 --- a/tests/python/pants_test/util/test_contextutil.py +++ b/tests/python/pants_test/util/test_contextutil.py @@ -66,10 +66,13 @@ def test_hermetic_environment(self): def test_hermetic_environment_subprocesses(self): self.assertIn('USER', os.environ) - with hermetic_environment_as(**{}): + with hermetic_environment_as(**dict(AAA='333')): output = subprocess.check_output('env', shell=True) self.assertNotIn('USER=', output) + self.assertIn('AAA', os.environ) + self.assertEquals(os.environ['AAA'], '333') self.assertIn('USER', os.environ) + self.assertNotIn('AAA', os.environ) def test_simple_pushd(self): pre_cwd = os.getcwd()