From b4bc7a62f435c0eb65c0a3e2f46f286fa2875165 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 15 Aug 2019 00:38:40 -0400 Subject: [PATCH 1/4] Remove lockfile-dependent test. --- tests/unit/test_unit_outdated.py | 46 +------------------------------- 1 file changed, 1 insertion(+), 45 deletions(-) diff --git a/tests/unit/test_unit_outdated.py b/tests/unit/test_unit_outdated.py index 092e7499205..d7230288b02 100644 --- a/tests/unit/test_unit_outdated.py +++ b/tests/unit/test_unit_outdated.py @@ -2,12 +2,11 @@ import json import os import sys -from contextlib import contextmanager import freezegun import pretend import pytest -from pip._vendor import lockfile, pkg_resources +from pip._vendor import pkg_resources from pip._internal.index import InstallationCandidate from pip._internal.utils import outdated @@ -162,49 +161,6 @@ def _get_statefile_path(cache_dir, key): ) -def test_self_check_state(monkeypatch, tmpdir): - CONTENT = '''{"key": "pip_prefix", "last_check": "1970-01-02T11:00:00Z", - "pypi_version": "1.0"}''' - fake_file = pretend.stub( - read=pretend.call_recorder(lambda: CONTENT), - write=pretend.call_recorder(lambda s: None), - ) - - @pretend.call_recorder - @contextmanager - def fake_open(filename, mode='r'): - yield fake_file - - monkeypatch.setattr(outdated, 'open', fake_open, raising=False) - - @pretend.call_recorder - @contextmanager - def fake_lock(filename): - yield - - monkeypatch.setattr(outdated, "check_path_owner", lambda p: True) - - monkeypatch.setattr(lockfile, 'LockFile', fake_lock) - - cache_dir = tmpdir / 'cache_dir' - key = 'pip_prefix' - monkeypatch.setattr(sys, 'prefix', key) - - state = SelfCheckState(cache_dir=cache_dir) - state.save('2.0', datetime.datetime.utcnow()) - - expected_path = _get_statefile_path(str(cache_dir), key) - assert fake_lock.calls == [pretend.call(expected_path)] - - assert fake_open.calls == [ - pretend.call(expected_path), - pretend.call(expected_path, 'w'), - ] - - # json.dumps will call this a number of times - assert len(fake_file.write.calls) - - def test_self_check_state_no_cache_dir(): state = SelfCheckState(cache_dir=False) assert state.state == {} From 6617d158387370c6890bd2d0562c84908187e646 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 15 Aug 2019 00:39:19 -0400 Subject: [PATCH 2/4] Write to tempfile and move instead of locking selfcheck file. --- src/pip/_internal/utils/filesystem.py | 47 +++++++++++++++++++++++++++ src/pip/_internal/utils/outdated.py | 20 ++++++++---- 2 files changed, 61 insertions(+), 6 deletions(-) diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index c5233ebbc71..56224e20ae6 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -2,8 +2,25 @@ import os.path import shutil import stat +from contextlib import contextmanager +from tempfile import NamedTemporaryFile + +# NOTE: retrying is not annotated in typeshed as on 2017-07-17, which is +# why we ignore the type on this import. +from pip._vendor.retrying import retry # type: ignore from pip._internal.utils.compat import get_path_uid +from pip._internal.utils.misc import cast +from pip._internal.utils.typing import MYPY_CHECK_RUNNING + +if MYPY_CHECK_RUNNING: + from typing import BinaryIO, Iterator + + class NamedTemporaryFileResult(BinaryIO): + @property + def file(self): + # type: () -> BinaryIO + pass def check_path_owner(path): @@ -59,3 +76,33 @@ def copy2_fixed(src, dest): def is_socket(path): # type: (str) -> bool return stat.S_ISSOCK(os.lstat(path).st_mode) + + +@contextmanager +def adjacent_tmp_file(path): + # type: (str) -> Iterator[NamedTemporaryFileResult] + """Given a path to a file, open a temp file next to it securely and ensure + it is written to disk after the context reaches its end. + """ + with NamedTemporaryFile( + delete=False, + dir=os.path.dirname(path), + prefix=os.path.basename(path), + suffix='.tmp', + ) as f: + result = cast('NamedTemporaryFileResult', f) + try: + yield result + finally: + result.file.flush() + os.fsync(result.file.fileno()) + + +@retry(stop_max_delay=1000, wait_fixed=250) +def replace(src, dest): + # type: (str, str) -> None + try: + os.rename(src, dest) + except OSError: + os.remove(dest) + os.rename(src, dest) diff --git a/src/pip/_internal/utils/outdated.py b/src/pip/_internal/utils/outdated.py index e0c90e15c49..241599b274a 100644 --- a/src/pip/_internal/utils/outdated.py +++ b/src/pip/_internal/utils/outdated.py @@ -7,7 +7,7 @@ import os.path import sys -from pip._vendor import lockfile, pkg_resources +from pip._vendor import pkg_resources from pip._vendor.packaging import version as packaging_version from pip._vendor.six import ensure_binary @@ -15,7 +15,11 @@ from pip._internal.index import PackageFinder from pip._internal.models.selection_prefs import SelectionPreferences from pip._internal.utils.compat import WINDOWS -from pip._internal.utils.filesystem import check_path_owner +from pip._internal.utils.filesystem import ( + adjacent_tmp_file, + check_path_owner, + replace, +) from pip._internal.utils.misc import ensure_dir, get_installed_version from pip._internal.utils.packaging import get_installer from pip._internal.utils.typing import MYPY_CHECK_RUNNING @@ -86,12 +90,16 @@ def save(self, pypi_version, current_time): text = json.dumps(state, sort_keys=True, separators=(",", ":")) - # Attempt to write out our version check file - with lockfile.LockFile(self.statefile_path): + with adjacent_tmp_file(self.statefile_path) as f: + f.write(ensure_binary(text)) + + try: # Since we have a prefix-specific state file, we can just # overwrite whatever is there, no need to check. - with open(self.statefile_path, "w") as statefile: - statefile.write(text) + replace(f.name, self.statefile_path) + except OSError: + # Best effort. + pass def was_installed_by_pip(pkg): From fc45975c8400e369a1e97176fbdec341cee71aa5 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Thu, 15 Aug 2019 01:06:59 -0400 Subject: [PATCH 3/4] Add news files. --- news/6954.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/6954.bugfix diff --git a/news/6954.bugfix b/news/6954.bugfix new file mode 100644 index 00000000000..8f6f67109cb --- /dev/null +++ b/news/6954.bugfix @@ -0,0 +1 @@ +Don't use hardlinks for locking selfcheck state file. From 68e6feb31a98b1327b01b17f63074c49ac0e2068 Mon Sep 17 00:00:00 2001 From: Chris Hunt Date: Tue, 3 Sep 2019 20:08:58 -0400 Subject: [PATCH 4/4] Use os.replace in Python 3. --- src/pip/_internal/utils/filesystem.py | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/pip/_internal/utils/filesystem.py b/src/pip/_internal/utils/filesystem.py index 56224e20ae6..f4a389cd92f 100644 --- a/src/pip/_internal/utils/filesystem.py +++ b/src/pip/_internal/utils/filesystem.py @@ -8,6 +8,7 @@ # NOTE: retrying is not annotated in typeshed as on 2017-07-17, which is # why we ignore the type on this import. from pip._vendor.retrying import retry # type: ignore +from pip._vendor.six import PY2 from pip._internal.utils.compat import get_path_uid from pip._internal.utils.misc import cast @@ -98,11 +99,17 @@ def adjacent_tmp_file(path): os.fsync(result.file.fileno()) -@retry(stop_max_delay=1000, wait_fixed=250) -def replace(src, dest): - # type: (str, str) -> None - try: - os.rename(src, dest) - except OSError: - os.remove(dest) - os.rename(src, dest) +_replace_retry = retry(stop_max_delay=1000, wait_fixed=250) + +if PY2: + @_replace_retry + def replace(src, dest): + # type: (str, str) -> None + try: + os.rename(src, dest) + except OSError: + os.remove(dest) + os.rename(src, dest) + +else: + replace = _replace_retry(os.replace)