From e372e971908df9f55d643ff8b2562b5336195483 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 17 Apr 2019 16:26:36 +0200 Subject: [PATCH 1/3] bpo-35755: shutil.which() uses os.confstr("CS_PATH") (GH-12858) shutil.which() and distutils.spawn.find_executable() now use os.confstr("CS_PATH") if available instead of os.defpath, if the PATH environment variable is not set. Don't use os.confstr("CS_PATH") nor os.defpath if the PATH environment variable is set to an empty string to mimick Unix 'which' command behavior. Changes: * find_executable() now starts by checking for the executable in the current working directly case. Add an explicit "if not path: return None". * Add tests for PATH='' (empty string), PATH=':' and for PATHEXT. (cherry picked from commit 228a3c99bdb2d02771bead66a0beabafad3a90d3) --- Lib/distutils/spawn.py | 39 +++++++---- Lib/distutils/tests/test_spawn.py | 47 ++++++++++++- Lib/shutil.py | 12 +++- Lib/test/test_shutil.py | 68 +++++++++++++++++++ .../2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst | 6 ++ 5 files changed, 154 insertions(+), 18 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst diff --git a/Lib/distutils/spawn.py b/Lib/distutils/spawn.py index 538768809327955..888327270e22941 100644 --- a/Lib/distutils/spawn.py +++ b/Lib/distutils/spawn.py @@ -172,21 +172,32 @@ def find_executable(executable, path=None): A string listing directories separated by 'os.pathsep'; defaults to os.environ['PATH']. Returns the complete filename or None if not found. """ - if path is None: - path = os.environ.get('PATH', os.defpath) - - paths = path.split(os.pathsep) - base, ext = os.path.splitext(executable) - + _, ext = os.path.splitext(executable) if (sys.platform == 'win32') and (ext != '.exe'): executable = executable + '.exe' - if not os.path.isfile(executable): - for p in paths: - f = os.path.join(p, executable) - if os.path.isfile(f): - # the file exists, we have a shot at spawn working - return f - return None - else: + if os.path.isfile(executable): return executable + + if path is None: + path = os.environ.get('PATH', None) + if path is None: + try: + path = os.confstr("CS_PATH") + except (AttributeError, ValueError): + # os.confstr() or CS_PATH is not available + path = os.defpath + # bpo-35755: Don't use os.defpath if the PATH environment variable is + # set to an empty string to mimick Unix which command behavior + + # PATH='' doesn't match, whereas PATH=':' looks in the current directory + if not path: + return None + + paths = path.split(os.pathsep) + for p in paths: + f = os.path.join(p, executable) + if os.path.isfile(f): + # the file exists, we have a shot at spawn working + return f + return None diff --git a/Lib/distutils/tests/test_spawn.py b/Lib/distutils/tests/test_spawn.py index 0d455385d8ace9a..f9ae69ef86b3dae 100644 --- a/Lib/distutils/tests/test_spawn.py +++ b/Lib/distutils/tests/test_spawn.py @@ -87,11 +87,52 @@ def test_find_executable(self): rv = find_executable(dont_exist_program , path=tmp_dir) self.assertIsNone(rv) - # test os.defpath: missing PATH environment variable + # PATH='': no match, except in the current directory with test_support.EnvironmentVarGuard() as env: - with mock.patch('distutils.spawn.os.defpath', tmp_dir): - env.pop('PATH') + env['PATH'] = '' + with unittest.mock.patch('distutils.spawn.os.confstr', + return_value=tmp_dir, create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', + tmp_dir): + rv = find_executable(program) + self.assertIsNone(rv) + + # look in current directory + with test_support.change_cwd(tmp_dir): + rv = find_executable(program) + self.assertEqual(rv, program) + + # PATH=':': explicitly looks in the current directory + with test_support.EnvironmentVarGuard() as env: + env['PATH'] = os.pathsep + with unittest.mock.patch('distutils.spawn.os.confstr', + return_value='', create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', ''): + rv = find_executable(program) + self.assertIsNone(rv) + + # look in current directory + with test_support.change_cwd(tmp_dir): + rv = find_executable(program) + self.assertEqual(rv, program) + + # missing PATH: test os.confstr("CS_PATH") and os.defpath + with test_support.EnvironmentVarGuard() as env: + env.pop('PATH', None) + + # without confstr + with unittest.mock.patch('distutils.spawn.os.confstr', + side_effect=ValueError, + create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', + tmp_dir): + rv = find_executable(program) + self.assertEqual(rv, filename) + # with confstr + with unittest.mock.patch('distutils.spawn.os.confstr', + return_value=tmp_dir, create=True), \ + unittest.mock.patch('distutils.spawn.os.defpath', ''): rv = find_executable(program) self.assertEqual(rv, filename) diff --git a/Lib/shutil.py b/Lib/shutil.py index f32c66b3550c91f..cbc16fc0c2095dd 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -1138,7 +1138,17 @@ def _access_check(fn, mode): return None if path is None: - path = os.environ.get("PATH", os.defpath) + path = os.environ.get("PATH", None) + if path is None: + try: + path = os.confstr("CS_PATH") + except (AttributeError, ValueError): + # os.confstr() or CS_PATH is not available + path = os.defpath + # bpo-35755: Don't use os.defpath if the PATH environment variable is + # set to an empty string to mimick Unix which command behavior + + # PATH='' doesn't match, whereas PATH=':' looks in the current directory if not path: return None path = path.split(os.pathsep) diff --git a/Lib/test/test_shutil.py b/Lib/test/test_shutil.py index 6e2b1004d309c96..197dd130a96474a 100644 --- a/Lib/test/test_shutil.py +++ b/Lib/test/test_shutil.py @@ -1500,6 +1500,57 @@ def test_environ_path(self): rv = shutil.which(self.file) self.assertEqual(rv, self.temp_file.name) + def test_environ_path_empty(self): + # PATH='': no match + with support.EnvironmentVarGuard() as env: + env['PATH'] = '' + with unittest.mock.patch('os.confstr', return_value=self.dir, \ + create=True), \ + support.swap_attr(os, 'defpath', self.dir), \ + support.change_cwd(self.dir): + rv = shutil.which(self.file) + self.assertIsNone(rv) + + def test_environ_path_cwd(self): + expected_cwd = os.path.basename(self.temp_file.name) + if sys.platform == "win32": + curdir = os.curdir + if isinstance(expected_cwd, bytes): + curdir = os.fsencode(curdir) + expected_cwd = os.path.join(curdir, expected_cwd) + + # PATH=':': explicitly looks in the current directory + with support.EnvironmentVarGuard() as env: + env['PATH'] = os.pathsep + with unittest.mock.patch('os.confstr', return_value=self.dir, \ + create=True), \ + support.swap_attr(os, 'defpath', self.dir): + rv = shutil.which(self.file) + self.assertIsNone(rv) + + # look in current directory + with support.change_cwd(self.dir): + rv = shutil.which(self.file) + self.assertEqual(rv, expected_cwd) + + def test_environ_path_missing(self): + with support.EnvironmentVarGuard() as env: + env.pop('PATH', None) + + # without confstr + with unittest.mock.patch('os.confstr', side_effect=ValueError, \ + create=True), \ + support.swap_attr(os, 'defpath', self.dir): + rv = shutil.which(self.file) + self.assertEqual(rv, self.temp_file.name) + + # with confstr + with unittest.mock.patch('os.confstr', return_value=self.dir, \ + create=True), \ + support.swap_attr(os, 'defpath', ''): + rv = shutil.which(self.file) + self.assertEqual(rv, self.temp_file.name) + def test_empty_path(self): base_dir = os.path.dirname(self.dir) with support.change_cwd(path=self.dir), \ @@ -1514,6 +1565,23 @@ def test_empty_path_no_PATH(self): rv = shutil.which(self.file) self.assertIsNone(rv) + @unittest.skipUnless(sys.platform == "win32", 'test specific to Windows') + def test_pathext(self): + ext = ".xyz" + temp_filexyz = tempfile.NamedTemporaryFile(dir=self.temp_dir, + prefix="Tmp2", suffix=ext) + os.chmod(temp_filexyz.name, stat.S_IXUSR) + self.addCleanup(temp_filexyz.close) + + # strip path and extension + program = os.path.basename(temp_filexyz.name) + program = os.path.splitext(program)[0] + + with support.EnvironmentVarGuard() as env: + env['PATHEXT'] = ext + rv = shutil.which(program, path=self.temp_dir) + self.assertEqual(rv, temp_filexyz.name) + class TestMove(unittest.TestCase): diff --git a/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst b/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst new file mode 100644 index 000000000000000..8e92ffdec235a98 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst @@ -0,0 +1,6 @@ +:func:`shutil.which` and :func:`distutils.spawn.find_executable` now use +``os.confstr("CS_PATH")`` if available instead of :data:`os.defpath`, if the +``PATH`` environment variable is not set. Moreover, don't use +``os.confstr("CS_PATH")`` nor :data:`os.defpath` if the ``PATH`` environment +variable is set to an empty string to mimick Unix ``which`` command +behavior. From 64bd42263ec9db862386709c927492012bf0e176 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 17 Apr 2019 17:05:30 +0200 Subject: [PATCH 2/3] bpo-35755: Remove current directory from posixpath.defpath (GH-11586) Document the change in a NEWS entry of the Security category. (cherry picked from commit 2c4c02f8a876fcf084575dcaf857a0236c81261a) --- Lib/posixpath.py | 2 +- .../next/Security/2019-01-17-10-03-48.bpo-35755.GmllIs.rst | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) create mode 100644 Misc/NEWS.d/next/Security/2019-01-17-10-03-48.bpo-35755.GmllIs.rst diff --git a/Lib/posixpath.py b/Lib/posixpath.py index ca578a5df35c223..785aa728b3658c6 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -18,7 +18,7 @@ extsep = '.' sep = '/' pathsep = ':' -defpath = ':/bin:/usr/bin' +defpath = '/bin:/usr/bin' altsep = None devnull = '/dev/null' diff --git a/Misc/NEWS.d/next/Security/2019-01-17-10-03-48.bpo-35755.GmllIs.rst b/Misc/NEWS.d/next/Security/2019-01-17-10-03-48.bpo-35755.GmllIs.rst new file mode 100644 index 000000000000000..959aafd73449e15 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2019-01-17-10-03-48.bpo-35755.GmllIs.rst @@ -0,0 +1,5 @@ +:func:`shutil.which` now uses ``os.confstr("CS_PATH")`` if available and if the +:envvar:`PATH` environment variable is not set. Remove also the current +directory from :data:`posixpath.defpath`. On Unix, :func:`shutil.which` and the +:mod:`subprocess` module no longer search the executable in the current +directory if the :envvar:`PATH` environment variable is not set. From df517cf146723b24f399f9bb6ba07ce7e633cfb6 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 17 Apr 2019 17:44:06 +0200 Subject: [PATCH 3/3] bpo-35755: Don't say "to mimick Unix which command behavior" (GH-12861) (cherry picked from commit 197f0447e3bcfa4f529fedab09966d7e3d283979) --- Lib/distutils/spawn.py | 2 +- Lib/shutil.py | 2 +- .../next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/Lib/distutils/spawn.py b/Lib/distutils/spawn.py index 888327270e22941..d3a12c283397827 100644 --- a/Lib/distutils/spawn.py +++ b/Lib/distutils/spawn.py @@ -188,7 +188,7 @@ def find_executable(executable, path=None): # os.confstr() or CS_PATH is not available path = os.defpath # bpo-35755: Don't use os.defpath if the PATH environment variable is - # set to an empty string to mimick Unix which command behavior + # set to an empty string # PATH='' doesn't match, whereas PATH=':' looks in the current directory if not path: diff --git a/Lib/shutil.py b/Lib/shutil.py index cbc16fc0c2095dd..b0a53dba3a3477a 100644 --- a/Lib/shutil.py +++ b/Lib/shutil.py @@ -1146,7 +1146,7 @@ def _access_check(fn, mode): # os.confstr() or CS_PATH is not available path = os.defpath # bpo-35755: Don't use os.defpath if the PATH environment variable is - # set to an empty string to mimick Unix which command behavior + # set to an empty string # PATH='' doesn't match, whereas PATH=':' looks in the current directory if not path: diff --git a/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst b/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst index 8e92ffdec235a98..d84f63bf7b838ba 100644 --- a/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst +++ b/Misc/NEWS.d/next/Library/2019-04-16-17-50-39.bpo-35755.Fg4EXb.rst @@ -2,5 +2,4 @@ ``os.confstr("CS_PATH")`` if available instead of :data:`os.defpath`, if the ``PATH`` environment variable is not set. Moreover, don't use ``os.confstr("CS_PATH")`` nor :data:`os.defpath` if the ``PATH`` environment -variable is set to an empty string to mimick Unix ``which`` command -behavior. +variable is set to an empty string.