From ff90b44de344387a344c8f5e7956fca1ed18d50f Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 5 Dec 2018 13:11:25 +0100 Subject: [PATCH 1/3] bpo-10496: posixpath.expanduser() catchs pwd.getpwuid() error * posixpath.expanduser() now returns the input path unchanged if the HOME environment variable is not set and pwd.getpwuid() raises KeyError (the current user identifier doesn't exist in the password database). * Add test_no_home_directory() to test_site. --- Lib/posixpath.py | 9 ++- Lib/test/test_posixpath.py | 68 ++++++++++++------- Lib/test/test_site.py | 35 ++++++++++ .../2018-12-05-13-37-39.bpo-10496.VH-1Lp.rst | 5 ++ 4 files changed, 90 insertions(+), 27 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2018-12-05-13-37-39.bpo-10496.VH-1Lp.rst diff --git a/Lib/posixpath.py b/Lib/posixpath.py index 7e3f3db4b6d017..4914a1728ab557 100644 --- a/Lib/posixpath.py +++ b/Lib/posixpath.py @@ -246,7 +246,12 @@ def expanduser(path): if i == 1: if 'HOME' not in os.environ: import pwd - userhome = pwd.getpwuid(os.getuid()).pw_dir + try: + userhome = pwd.getpwuid(os.getuid()).pw_dir + except KeyError: + # bpo-10496: if the current user identifier doesn't exist in the + # password database, return the path unchanged + return path else: userhome = os.environ['HOME'] else: @@ -257,6 +262,8 @@ def expanduser(path): try: pwent = pwd.getpwnam(name) except KeyError: + # bpo-10496: if the user name from the path doesn't exist in the + # password database, return the path unchanged return path userhome = pwent.pw_dir if isinstance(path, bytes): diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index ae59ef5927be84..36ff9a69affc30 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -5,6 +5,7 @@ from posixpath import realpath, abspath, dirname, basename from test import support, test_genericpath from test.support import FakePath +from unittest import mock try: import posix @@ -242,6 +243,8 @@ def fake_lstat(path): def test_expanduser(self): self.assertEqual(posixpath.expanduser("foo"), "foo") self.assertEqual(posixpath.expanduser(b"foo"), b"foo") + + def test_expanduser_home_envvar(self): with support.EnvironmentVarGuard() as env: for home in '/', '', '//', '///': with self.subTest(home=home): @@ -249,35 +252,48 @@ def test_expanduser(self): self.assertEqual(posixpath.expanduser("~"), "/") self.assertEqual(posixpath.expanduser("~/"), "/") self.assertEqual(posixpath.expanduser("~/foo"), "/foo") + + def test_expanduser_pwd(self): try: import pwd except ImportError: - pass - else: - self.assertIsInstance(posixpath.expanduser("~/"), str) - self.assertIsInstance(posixpath.expanduser(b"~/"), bytes) - # if home directory == root directory, this test makes no sense - if posixpath.expanduser("~") != '/': - self.assertEqual( - posixpath.expanduser("~") + "/", - posixpath.expanduser("~/") - ) - self.assertEqual( - posixpath.expanduser(b"~") + b"/", - posixpath.expanduser(b"~/") - ) - self.assertIsInstance(posixpath.expanduser("~root/"), str) - self.assertIsInstance(posixpath.expanduser("~foo/"), str) - self.assertIsInstance(posixpath.expanduser(b"~root/"), bytes) - self.assertIsInstance(posixpath.expanduser(b"~foo/"), bytes) - - with support.EnvironmentVarGuard() as env: - # expanduser should fall back to using the password database - del env['HOME'] - home = pwd.getpwuid(os.getuid()).pw_dir - # $HOME can end with a trailing /, so strip it (see #17809) - home = home.rstrip("/") or '/' - self.assertEqual(posixpath.expanduser("~"), home) + self.skipTest("need pwd module") + + self.assertIsInstance(posixpath.expanduser("~/"), str) + self.assertIsInstance(posixpath.expanduser(b"~/"), bytes) + + # if home directory == root directory, this test makes no sense + if posixpath.expanduser("~") != '/': + self.assertEqual( + posixpath.expanduser("~") + "/", + posixpath.expanduser("~/") + ) + self.assertEqual( + posixpath.expanduser(b"~") + b"/", + posixpath.expanduser(b"~/") + ) + self.assertIsInstance(posixpath.expanduser("~root/"), str) + self.assertIsInstance(posixpath.expanduser("~foo/"), str) + self.assertIsInstance(posixpath.expanduser(b"~root/"), bytes) + self.assertIsInstance(posixpath.expanduser(b"~foo/"), bytes) + + with support.EnvironmentVarGuard() as env: + # expanduser should fall back to using the password database + del env['HOME'] + + home = pwd.getpwuid(os.getuid()).pw_dir + # $HOME can end with a trailing /, so strip it (see #17809) + home = home.rstrip("/") or '/' + self.assertEqual(posixpath.expanduser("~"), home) + + # bpo-10496: If the HOME environment variable is not set and the + # user (current identifier or name in the path) doesn't exist in + # the password database (pwd.getuid() or pwd.getpwnam() fail), + # expanduser() must return the path unchanged. + with mock.patch.object(pwd, 'getpwuid', side_effect=KeyError), \ + mock.patch.object(pwd, 'getpwnam', side_effect=KeyError): + for path in ('~', '~/.local', '~vstinner/'): + self.assertEqual(posixpath.expanduser(path), path) def test_normpath(self): self.assertEqual(posixpath.normpath(""), ".") diff --git a/Lib/test/test_site.py b/Lib/test/test_site.py index 33a8f1a44ccc93..b0e16d9f42d92a 100644 --- a/Lib/test/test_site.py +++ b/Lib/test/test_site.py @@ -19,6 +19,7 @@ import subprocess import sysconfig import tempfile +from unittest import mock from copy import copy # These tests are not particularly useful if Python was invoked with -S. @@ -256,6 +257,7 @@ def test_getusersitepackages(self): # the call sets USER_BASE *and* USER_SITE self.assertEqual(site.USER_SITE, user_site) self.assertTrue(user_site.startswith(site.USER_BASE), user_site) + self.assertTrue(site.USER_BASE, site.getuserbase()) def test_getsitepackages(self): site.PREFIXES = ['xoxo'] @@ -274,6 +276,39 @@ def test_getsitepackages(self): wanted = os.path.join('xoxo', 'lib', 'site-packages') self.assertEqual(dirs[1], wanted) + def test_no_home_directory(self): + # bpo-10496: getuserbase() and getusersitepackages() must not fail if + # the current user has no home directory (if expanduser() returns the + # path unchanged). + site.USER_SITE = None + site.USER_BASE = None + + with EnvironmentVarGuard() as environ, \ + mock.patch('os.path.expanduser', lambda path: path): + + del environ['PYTHONUSERBASE'] + del environ['APPDATA'] + + user_base = site.getuserbase() + self.assertTrue(user_base.startswith('~' + os.sep), + user_base) + + user_site = site.getusersitepackages() + self.assertTrue(user_site.startswith(user_base), user_site) + + with mock.patch('os.path.isdir', return_value=False) as mock_isdir, \ + mock.patch.object(site, 'addsitedir') as mock_addsitedir: + + # addusersitepackages() must not add user_site to sys.path + # if it is not an existing directory + known_paths = set() + site.addusersitepackages(known_paths) + + mock_isdir.assert_called_once_with(user_site) + mock_addsitedir.assert_not_called() + self.assertFalse(known_paths) + + class PthFile(object): """Helper class for handling testing of .pth files""" diff --git a/Misc/NEWS.d/next/Library/2018-12-05-13-37-39.bpo-10496.VH-1Lp.rst b/Misc/NEWS.d/next/Library/2018-12-05-13-37-39.bpo-10496.VH-1Lp.rst new file mode 100644 index 00000000000000..232fcc6503b029 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2018-12-05-13-37-39.bpo-10496.VH-1Lp.rst @@ -0,0 +1,5 @@ +:func:`posixpath.expanduser` now returns the input *path* unchanged if the +``HOME`` environment variable is not set and the current user has no home +directory (if the current user identifier doesn't exist in the password +database). This change fix the :mod:`site` module if the current user doesn't +exist in the password database (if the user has no home directory). From 4fd59f5cbffc8ace2ce72edf536b441e2fe2d87a Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 5 Dec 2018 15:10:03 +0100 Subject: [PATCH 2/3] Enhance tests * Add more tests to test_expanduser_home_envvar() * Fix typo: assertTrue => assertEqual * Use support.import_module() --- Lib/test/test_posixpath.py | 12 ++++++++---- Lib/test/test_site.py | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_posixpath.py b/Lib/test/test_posixpath.py index 36ff9a69affc30..983e2dd6ff2763 100644 --- a/Lib/test/test_posixpath.py +++ b/Lib/test/test_posixpath.py @@ -246,6 +246,13 @@ def test_expanduser(self): def test_expanduser_home_envvar(self): with support.EnvironmentVarGuard() as env: + env['HOME'] = '/home/victor' + self.assertEqual(posixpath.expanduser("~"), "/home/victor") + + # expanduser() strips trailing slash + env['HOME'] = '/home/victor/' + self.assertEqual(posixpath.expanduser("~"), "/home/victor") + for home in '/', '', '//', '///': with self.subTest(home=home): env['HOME'] = home @@ -254,10 +261,7 @@ def test_expanduser_home_envvar(self): self.assertEqual(posixpath.expanduser("~/foo"), "/foo") def test_expanduser_pwd(self): - try: - import pwd - except ImportError: - self.skipTest("need pwd module") + pwd = support.import_module('pwd') self.assertIsInstance(posixpath.expanduser("~/"), str) self.assertIsInstance(posixpath.expanduser(b"~/"), bytes) diff --git a/Lib/test/test_site.py b/Lib/test/test_site.py index b0e16d9f42d92a..919e789f133b00 100644 --- a/Lib/test/test_site.py +++ b/Lib/test/test_site.py @@ -257,7 +257,7 @@ def test_getusersitepackages(self): # the call sets USER_BASE *and* USER_SITE self.assertEqual(site.USER_SITE, user_site) self.assertTrue(user_site.startswith(site.USER_BASE), user_site) - self.assertTrue(site.USER_BASE, site.getuserbase()) + self.assertEqual(site.USER_BASE, site.getuserbase()) def test_getsitepackages(self): site.PREFIXES = ['xoxo'] From ad5cbb5b76b681915a763ea06ce38f8436711392 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 5 Dec 2018 16:12:44 +0100 Subject: [PATCH 3/3] Fix test_site when run from a venv --- Lib/test/test_site.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/test/test_site.py b/Lib/test/test_site.py index 919e789f133b00..f38e8d853adabd 100644 --- a/Lib/test/test_site.py +++ b/Lib/test/test_site.py @@ -6,6 +6,7 @@ """ import unittest import test.support +from test import support from test.support import (captured_stderr, TESTFN, EnvironmentVarGuard, change_cwd) import builtins @@ -297,7 +298,8 @@ def test_no_home_directory(self): self.assertTrue(user_site.startswith(user_base), user_site) with mock.patch('os.path.isdir', return_value=False) as mock_isdir, \ - mock.patch.object(site, 'addsitedir') as mock_addsitedir: + mock.patch.object(site, 'addsitedir') as mock_addsitedir, \ + support.swap_attr(site, 'ENABLE_USER_SITE', True): # addusersitepackages() must not add user_site to sys.path # if it is not an existing directory