Skip to content

Don't use lockfile to protect updates to selfcheck file. #6879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Sep 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/6954.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Don't use hardlinks for locking selfcheck state file.
54 changes: 54 additions & 0 deletions src/pip/_internal/utils/filesystem.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,26 @@
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._vendor.six import PY2

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):
Expand Down Expand Up @@ -59,3 +77,39 @@ 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())


_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)
20 changes: 14 additions & 6 deletions src/pip/_internal/utils/outdated.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,19 @@
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

from pip._internal.cli.cmdoptions import make_search_scope
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
Expand Down Expand Up @@ -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):
Expand Down
46 changes: 1 addition & 45 deletions tests/unit/test_unit_outdated.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 == {}
Expand Down