From c31f19fcaea8e64ca04de2976d53b681170e406b Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Sun, 3 Feb 2019 04:39:06 -0800 Subject: [PATCH 1/5] Fixes #6169: AdjacentTempDirectory should fail on unwritable directory --- .gitignore | 4 + news/6169.bugfix | 1 + news/6194.bugfix | 2 +- src/pip/_internal/req/req_uninstall.py | 161 +++++++++++++++++++------ src/pip/_internal/utils/temp_dir.py | 7 +- tests/unit/test_utils.py | 30 +++++ 6 files changed, 163 insertions(+), 42 deletions(-) create mode 100644 news/6169.bugfix diff --git a/.gitignore b/.gitignore index dfa41c022..6c387e2f0 100644 --- a/.gitignore +++ b/.gitignore @@ -31,9 +31,13 @@ tests/data/common_wheels/ # Misc *~ .*.sw? +.env/ # For IntelliJ IDEs (basically PyCharm) .idea/ +# For Visual Studio Code +.vscode/ + # Scratch Pad for experiments .scratch/ diff --git a/news/6169.bugfix b/news/6169.bugfix new file mode 100644 index 000000000..ad821391c --- /dev/null +++ b/news/6169.bugfix @@ -0,0 +1 @@ +``AdjacentTempDirectory`` fails on unwritable directory instead of locking up the uninstall command. \ No newline at end of file diff --git a/news/6194.bugfix b/news/6194.bugfix index 146b0faa5..dc55a110d 100644 --- a/news/6194.bugfix +++ b/news/6194.bugfix @@ -1 +1 @@ -Make failed uninstalls roll back more reliably and better at avoiding naming conflicts. \ No newline at end of file +Make failed uninstalls roll back more reliably and better at avoiding naming conflicts. diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py index ffe962c00..e95368bca 100644 --- a/src/pip/_internal/req/req_uninstall.py +++ b/src/pip/_internal/req/req_uninstall.py @@ -17,7 +17,7 @@ from pip._internal.utils.misc import ( FakeFile, ask, dist_in_usersite, dist_is_local, egg_link_path, is_local, normalize_path, renames, rmtree, ) -from pip._internal.utils.temp_dir import AdjacentTempDirectory +from pip._internal.utils.temp_dir import AdjacentTempDirectory, TempDirectory logger = logging.getLogger(__name__) @@ -183,6 +183,112 @@ def compress_for_output_listing(paths): return will_remove, will_skip +class StashedUninstallPathSet(object): + """A set of file rename operations to stash files while + tentatively uninstalling them.""" + def __init__(self): + # Mapping from source file root to [Adjacent]TempDirectory + # for files under that directory. + self._save_dirs = {} + # (old path, new path) tuples for each move that may need + # to be undone. + self._moves = [] + + def _add_root(self, path): + """Create or return the stash directory for a given path. + + For internal use only. External users should call add_root()""" + key = os.path.normcase(path).rstrip("/\\") + + if key in self._save_dirs: + return key, self._save_dirs[key] + + save_dir = AdjacentTempDirectory(path) + try: + save_dir.create() + except OSError: + save_dir = TempDirectory(kind='uninstall') + save_dir.create() + + self._save_dirs[key] = save_dir + return key, save_dir + + def add_root(self, path): + """Adds a root directory that we will be moving files from.""" + if not os.path.isdir(path): + raise ValueError("Roots must be directories") + + # Keep return values internal + self._add_root(path) + + def _get_stash_path(self, path): + """Finds a place to stash the path + + If no root has been provided, one will be created for the directory + passed.""" + path = os.path.normcase(path) + head, old_head = os.path.dirname(path), None + best_head, best_save_dir = None, None + + while head != old_head: + save_dir = self._save_dirs.get(head) + if save_dir: + break + head, old_head = os.path.dirname(head), head + + if not best_save_dir: + head = path if os.path.isdir(path) else os.path.dirname(path) + best_head, best_save_dir = self._add_root(head) + + relpath = os.path.relpath(path, best_head) + if not relpath or relpath == os.path.curdir: + return best_save_dir.path + return os.path.join(best_save_dir.path, relpath) + + def stash(self, path): + """Stashes a file somewhere out of the way.""" + new_path = self._get_stash_path(path) + self._moves.append((path, new_path)) + if os.path.isdir(path) and os.path.isdir(new_path): + # If we're moving a directory, we need to + # remove the destination first or else it will be + # moved to inside the existing directory. + # We just created new_path ourselves, so it will + # be removable. + os.rmdir(new_path) + renames(path, new_path) + + def commit(self): + """Commits the uninstall by removing stashed files.""" + for _, save_dir in self._save_dirs.items(): + save_dir.cleanup() + self._moves = [] + self._save_dirs = {} + + def rollback(self): + """Undoes the uninstall by moving stashed files back.""" + for p in self._moves: + logging.info("Moving to %s\n from %s", *p) + + for new_path, path in self._moves: + try: + logger.debug('Replacing %s from %s', new_path, path) + if os.path.isfile(new_path): + os.unlink(new_path) + elif os.path.isdir(new_path): + rmtree(new_path) + renames(path, new_path) + except OSError as ex: + logger.error("Failed to restore %s", new_path) + logger.debug("Exception: %s", ex) + + self.commit() + + @property + def can_rollback(self): + return bool(self._moves) + + class UninstallPathSet(object): """A set of file paths to be removed in the uninstallation of a requirement.""" @@ -191,8 +297,7 @@ class UninstallPathSet(object): self._refuse = set() self.pth = {} self.dist = dist - self._save_dirs = [] - self._moved_paths = [] + self._moved_paths = StashedUninstallPathSet() def _permitted(self, path): """ @@ -230,22 +335,6 @@ class UninstallPathSet(object): else: self._refuse.add(pth_file) - def _stash(self, path): - best = None - for save_dir in self._save_dirs: - if not path.startswith(save_dir.original + os.sep): - continue - if not best or len(save_dir.original) > len(best.original): - best = save_dir - if best is None: - best = AdjacentTempDirectory(os.path.dirname(path)) - best.create() - self._save_dirs.append(best) - relpath = os.path.relpath(path, best.original) - if not relpath or relpath == os.path.curdir: - return best.path - return os.path.join(best.path, relpath) - def remove(self, auto_confirm=False, verbose=False): """Remove paths in ``self.paths`` with confirmation (unless ``auto_confirm`` is True).""" @@ -264,18 +353,18 @@ class UninstallPathSet(object): with indent_log(): if auto_confirm or self._allowed_to_proceed(verbose): - for path in sorted(compact(compress_for_rename(self.paths))): - new_path = self._stash(path) + moved = self._moved_paths + + for_rename = compress_for_rename(self.paths) + + for path in for_rename: + if os.path.isdir(path): + moved.add_root(path) + + for path in sorted(compact(for_rename)): + moved.stash(path) logger.debug('Removing file or directory %s', path) - self._moved_paths.append((path, new_path)) - if os.path.isdir(path) and os.path.isdir(new_path): - # If we're moving a directory, we need to - # remove the destination first or else it will be - # moved to inside the existing directory. - # We just created new_path ourselves, so it will - # be removable. - os.rmdir(new_path) - renames(path, new_path) + for pth in self.pth.values(): pth.remove() @@ -312,18 +401,14 @@ class UninstallPathSet(object): def rollback(self): """Rollback the changes previously made by remove().""" - if not self._save_dirs: + if not self._moved_paths.can_rollback: logger.error( "Can't roll back %s; was not uninstalled", self.dist.project_name, ) return False logger.info('Rolling back uninstall of %s', self.dist.project_name) - for path, tmp_path in self._moved_paths: - logger.debug('Replacing %s', path) - if os.path.isdir(tmp_path) and os.path.isdir(path): - rmtree(path) - renames(tmp_path, path) + self._moved_paths.rollback() for pth in self.pth.values(): pth.rollback() for save_dir in self._save_dirs: @@ -331,9 +416,7 @@ class UninstallPathSet(object): def commit(self): """Remove temporary save dir: rollback will no longer be possible.""" - for save_dir in self._save_dirs: - save_dir.cleanup() - self._moved_paths = [] + self._moved_paths.commit() @classmethod def from_dist(cls, dist): diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index d27084cc8..b745d9cdc 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -1,5 +1,6 @@ from __future__ import absolute_import +import errno import itertools import logging import os.path @@ -132,8 +133,10 @@ class AdjacentTempDirectory(TempDirectory): path = os.path.join(root, candidate) try: os.mkdir(path) - except OSError: - pass + except OSError as ex: + # Continue if the name exists already + if ex.errno != errno.EEXIST: + raise else: self.path = os.path.realpath(path) break diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 459fb7a71..9674d1cd8 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -578,6 +578,36 @@ class TestTempDirectory(object): for x, y in zip(some_names, expected_names): assert x == y + def test_adjacent_directory_exists(self): + name = "ABC" + block_name, expect_name = itertools.islice( + AdjacentTempDirectory._generate_names(name), 2) + with TempDirectory() as tmp_dir: + original = os.path.join(tmp_dir.path, name) + blocker = os.path.join(tmp_dir.path, block_name) + + ensure_dir(original) + ensure_dir(blocker) + + with AdjacentTempDirectory(original) as atmp_dir: + assert expect_name == os.path.split(atmp_dir.path)[1] + + def test_adjacent_directory_permission_error(self, monkeypatch): + name = "ABC" + + def raising_mkdir(*args, **kwargs): + raise OSError("Unknown OSError") + + with TempDirectory() as tmp_dir: + original = os.path.join(tmp_dir.path, name) + + ensure_dir(original) + monkeypatch.setattr("os.mkdir", raising_mkdir) + + with pytest.raises(OSError): + with AdjacentTempDirectory(original): + pass + class TestGlibc(object): def test_manylinux_check_glibc_version(self): From a56854dd9ca4655e8327f4753bbad29eb02000fb Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 4 Feb 2019 11:25:13 -0800 Subject: [PATCH 2/5] Remove mismerged line --- src/pip/_internal/req/req_uninstall.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py index e95368bca..4356cacf7 100644 --- a/src/pip/_internal/req/req_uninstall.py +++ b/src/pip/_internal/req/req_uninstall.py @@ -411,8 +411,6 @@ class UninstallPathSet(object): self._moved_paths.rollback() for pth in self.pth.values(): pth.rollback() - for save_dir in self._save_dirs: - save_dir.cleanup() def commit(self): """Remove temporary save dir: rollback will no longer be possible.""" From 53d28bae66a5cc951e25d817989435c10da35231 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Mon, 4 Feb 2019 14:48:41 -0800 Subject: [PATCH 3/5] Added more tests and handling for short directory names --- src/pip/_internal/req/req_uninstall.py | 71 ++++++++++--------- src/pip/_internal/utils/temp_dir.py | 10 ++- tests/unit/test_req_uninstall.py | 94 ++++++++++++++++++++++++++ tests/unit/test_utils.py | 75 +++++++++++++++----- 4 files changed, 194 insertions(+), 56 deletions(-) diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py index 4356cacf7..47d109bc8 100644 --- a/src/pip/_internal/req/req_uninstall.py +++ b/src/pip/_internal/req/req_uninstall.py @@ -127,7 +127,7 @@ def compress_for_rename(paths): # If all the files we found are in our remaining set of files to # remove, then remove them from the latter set and add a wildcard # for the directory. - if len(all_files - remaining) == 0: + if not (all_files - remaining): remaining.difference_update(all_files) wildcards.add(root + os.sep) @@ -194,60 +194,58 @@ class StashedUninstallPathSet(object): # to be undone. self._moves = [] - def _add_root(self, path): - """Create or return the stash directory for a given path. + def _get_directory_stash(self, path): + """Stashes a directory. - For internal use only. External users should call add_root()""" - key = os.path.normcase(path).rstrip("/\\") + Directories are stashed adjacent to their original location if + possible, or else moved/copied into the user's temp dir.""" - if key in self._save_dirs: - return key, self._save_dirs[key] - - save_dir = AdjacentTempDirectory(path) try: + save_dir = AdjacentTempDirectory(path) save_dir.create() except OSError: - save_dir = TempDirectory(kind='uninstall') + save_dir = TempDirectory(kind="uninstall") save_dir.create() + self._save_dirs[os.path.normcase(path)] = save_dir - self._save_dirs[key] = save_dir - return key, save_dir + return save_dir.path - def add_root(self, path): - """Adds a root directory that we will be moving files from.""" - if not os.path.isdir(path): - raise ValueError("Roots must be directories") - - # Keep return values internal - self._add_root(path) - - def _get_stash_path(self, path): - """Finds a place to stash the path + def _get_file_stash(self, path): + """Stashes a file. If no root has been provided, one will be created for the directory - passed.""" + in the user's temp directory.""" path = os.path.normcase(path) head, old_head = os.path.dirname(path), None - best_head, best_save_dir = None, None + save_dir = None while head != old_head: - save_dir = self._save_dirs.get(head) - if save_dir: + try: + save_dir = self._save_dirs[head] break + except KeyError: + pass head, old_head = os.path.dirname(head), head + else: + # Did not find any suitable root + head = os.path.dirname(path) + save_dir = TempDirectory(kind='uninstall') + save_dir.create() + self._save_dirs[head] = save_dir - if not best_save_dir: - head = path if os.path.isdir(path) else os.path.dirname(path) - best_head, best_save_dir = self._add_root(head) - - relpath = os.path.relpath(path, best_head) - if not relpath or relpath == os.path.curdir: - return best_save_dir.path - return os.path.join(best_save_dir.path, relpath) + relpath = os.path.relpath(path, head) + if relpath and relpath != os.path.curdir: + return os.path.join(save_dir.path, relpath) + return save_dir.path def stash(self, path): - """Stashes a file somewhere out of the way.""" - new_path = self._get_stash_path(path) + """Stashes the directory or file and returns its new location. + """ + if os.path.isdir(path): + new_path = self._get_directory_stash(path) + else: + new_path = self._get_file_stash(path) + self._moves.append((path, new_path)) if os.path.isdir(path) and os.path.isdir(new_path): # If we're moving a directory, we need to @@ -257,6 +255,7 @@ class StashedUninstallPathSet(object): # be removable. os.rmdir(new_path) renames(path, new_path) + return new_path def commit(self): """Commits the uninstall by removing stashed files.""" diff --git a/src/pip/_internal/utils/temp_dir.py b/src/pip/_internal/utils/temp_dir.py index b745d9cdc..2c81ad554 100644 --- a/src/pip/_internal/utils/temp_dir.py +++ b/src/pip/_internal/utils/temp_dir.py @@ -119,14 +119,20 @@ class AdjacentTempDirectory(TempDirectory): package). """ for i in range(1, len(name)): - if name[i] in cls.LEADING_CHARS: - continue for candidate in itertools.combinations_with_replacement( cls.LEADING_CHARS, i - 1): new_name = '~' + ''.join(candidate) + name[i:] if new_name != name: yield new_name + # If we make it this far, we will have to make a longer name + for i in range(len(cls.LEADING_CHARS)): + for candidate in itertools.combinations_with_replacement( + cls.LEADING_CHARS, i): + new_name = '~' + ''.join(candidate) + name + if new_name != name: + yield new_name + def create(self): root, name = os.path.split(self.original) for candidate in self._generate_names(name): diff --git a/tests/unit/test_req_uninstall.py b/tests/unit/test_req_uninstall.py index 076ae05c4..e80d7f372 100644 --- a/tests/unit/test_req_uninstall.py +++ b/tests/unit/test_req_uninstall.py @@ -7,6 +7,7 @@ import pip._internal.req.req_uninstall from pip._internal.req.req_uninstall import ( UninstallPathSet, compact, compress_for_output_listing, compress_for_rename, uninstallation_paths, + StashedUninstallPathSet, ) from tests.lib import create_file @@ -175,3 +176,96 @@ class TestUninstallPathSet(object): ups.add(path1) ups.add(path2) assert ups.paths == {path1} + + +class TestStashedUninstallPathSet(object): + WALK_RESULT = [ + ("A", ["B", "C"], ["a.py"]), + ("A/B", ["D"], ["b.py"]), + ("A/B/D", [], ["c.py"]), + ("A/C", [], ["d.py", "e.py"]), + ("A/E", ["F"], ["f.py"]), + ("A/E/F", [], []), + ("A/G", ["H"], ["g.py"]), + ("A/G/H", [], ["h.py"]), + ] + + @classmethod + def mock_walk(cls, root): + for dirname, subdirs, files in cls.WALK_RESULT: + dirname = os.path.sep.join(dirname.split("/")) + if dirname.startswith(root): + yield dirname[len(root) + 1:], subdirs, files + + def test_compress_for_rename(self, monkeypatch): + paths = [os.path.sep.join(p.split("/")) for p in [ + "A/B/b.py", + "A/B/D/c.py", + "A/C/d.py", + "A/E/f.py", + "A/G/g.py", + ]] + + expected_paths = [os.path.sep.join(p.split("/")) for p in [ + "A/B/", # selected everything below A/B + "A/C/d.py", # did not select everything below A/C + "A/E/", # only empty folders remain under A/E + "A/G/g.py", # non-empty folder remains under A/G + ]] + + monkeypatch.setattr('os.walk', self.mock_walk) + + actual_paths = compress_for_rename(paths) + assert set(expected_paths) == set(actual_paths) + + @classmethod + def make_stash(cls, tmpdir, paths): + for dirname, subdirs, files in cls.WALK_RESULT: + root = os.path.join(tmpdir, *dirname.split("/")) + if not os.path.exists(root): + os.mkdir(root) + for d in subdirs: + os.mkdir(os.path.join(root, d)) + for f in files: + with open(os.path.join(root, f), "wb"): + pass + + pathset = StashedUninstallPathSet() + + paths = [os.path.join(tmpdir, *p.split('/')) for p in paths] + stashed_paths = [(p, pathset.stash(p)) for p in paths] + + return pathset, stashed_paths + + def test_stash(self, tmpdir): + pathset, stashed_paths = self.make_stash(tmpdir, [ + "A/B/", "A/C/d.py", "A/E/", "A/G/g.py", + ]) + + for old_path, new_path in stashed_paths: + assert not os.path.exists(old_path) + assert os.path.exists(new_path) + + assert stashed_paths == pathset._moves + + def test_commit(self, tmpdir): + pathset, stashed_paths = self.make_stash(tmpdir, [ + "A/B/", "A/C/d.py", "A/E/", "A/G/g.py", + ]) + + pathset.commit() + + for old_path, new_path in stashed_paths: + assert not os.path.exists(old_path) + assert not os.path.exists(new_path) + + def test_rollback(self, tmpdir): + pathset, stashed_paths = self.make_stash(tmpdir, [ + "A/B/", "A/C/d.py", "A/E/", "A/G/g.py", + ]) + + pathset.rollback() + + for old_path, new_path in stashed_paths: + assert os.path.exists(old_path) + assert not os.path.exists(new_path) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 9674d1cd8..2c5c27306 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -553,6 +553,10 @@ class TestTempDirectory(object): "ABC.dist-info", "_+-", "_package", + "A......B", + "AB", + "A", + "2", ]) def test_adjacent_directory_names(self, name): def names(): @@ -566,31 +570,66 @@ class TestTempDirectory(object): # result that works, provided there are many of those # and that shorter names result in totally unique sets, # it's okay to skip part of the test.) - some_names = list(itertools.islice(names(), 10000)) - assert len(some_names) == len(set(some_names)) + some_names = list(itertools.islice(names(), 1000)) + # We should always get at least 1000 names + assert len(some_names) == 1000 - # Ensure original name does not appear - assert not any(n == name for n in names()) + # Ensure original name does not appear early in the set + assert name not in some_names - # Check the first group are correct - expected_names = ['~' + name[1:]] - expected_names.extend('~' + c + name[2:] for c in chars) - for x, y in zip(some_names, expected_names): - assert x == y + if len(name) > 2: + # Names should be at least 90% unique (given the infinite + # range of inputs, and the possibility that generated names + # may already exist on disk anyway, this is a much cheaper + # criteria to enforce than complete uniqueness). + assert len(some_names) > 0.9 * len(set(some_names)) - def test_adjacent_directory_exists(self): - name = "ABC" + # Ensure the first few names are the same length as the original + same_len = list(itertools.takewhile( + lambda x: len(x) == len(name), + some_names + )) + assert len(same_len) > 10 + + # Check the first group are correct + expected_names = ['~' + name[1:]] + expected_names.extend('~' + c + name[2:] for c in chars) + for x, y in zip(some_names, expected_names): + assert x == y + + else: + # All names are going to be longer than our original + assert min(len(x) for x in some_names) > 1 + + # All names are going to be unqiue + assert len(some_names) == len(set(some_names)) + + if len(name) == 2: + # All but the first name are going to end with our original + assert all(x.endswith(name) for x in some_names[1:]) + else: + # All names are going to end with our original + assert all(x.endswith(name) for x in some_names) + + @pytest.mark.parametrize("name", [ + "A", + "ABC", + "ABC.dist-info", + "_+-", + "_package", + ]) + def test_adjacent_directory_exists(self, name, tmpdir): block_name, expect_name = itertools.islice( AdjacentTempDirectory._generate_names(name), 2) - with TempDirectory() as tmp_dir: - original = os.path.join(tmp_dir.path, name) - blocker = os.path.join(tmp_dir.path, block_name) - ensure_dir(original) - ensure_dir(blocker) + original = os.path.join(tmpdir, name) + blocker = os.path.join(tmpdir, block_name) - with AdjacentTempDirectory(original) as atmp_dir: - assert expect_name == os.path.split(atmp_dir.path)[1] + ensure_dir(original) + ensure_dir(blocker) + + with AdjacentTempDirectory(original) as atmp_dir: + assert expect_name == os.path.split(atmp_dir.path)[1] def test_adjacent_directory_permission_error(self, monkeypatch): name = "ABC" From aba2943dace4f966d6515eb669a7bb66dd2da9fe Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 5 Feb 2019 14:08:58 -0800 Subject: [PATCH 4/5] Remove unused code --- src/pip/_internal/req/req_uninstall.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/pip/_internal/req/req_uninstall.py b/src/pip/_internal/req/req_uninstall.py index 47d109bc8..c80959e43 100644 --- a/src/pip/_internal/req/req_uninstall.py +++ b/src/pip/_internal/req/req_uninstall.py @@ -356,10 +356,6 @@ class UninstallPathSet(object): for_rename = compress_for_rename(self.paths) - for path in for_rename: - if os.path.isdir(path): - moved.add_root(path) - for path in sorted(compact(for_rename)): moved.stash(path) logger.debug('Removing file or directory %s', path) From c64503e58c907216d383d33516c4c8eae5388117 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 5 Feb 2019 15:07:19 -0800 Subject: [PATCH 5/5] Fixes ordering of imports --- tests/unit/test_req_uninstall.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/test_req_uninstall.py b/tests/unit/test_req_uninstall.py index e80d7f372..37edb7dfa 100644 --- a/tests/unit/test_req_uninstall.py +++ b/tests/unit/test_req_uninstall.py @@ -5,9 +5,8 @@ from mock import Mock import pip._internal.req.req_uninstall from pip._internal.req.req_uninstall import ( - UninstallPathSet, compact, compress_for_output_listing, - compress_for_rename, uninstallation_paths, - StashedUninstallPathSet, + StashedUninstallPathSet, UninstallPathSet, compact, + compress_for_output_listing, compress_for_rename, uninstallation_paths, ) from tests.lib import create_file