Repository navigation
Commit b0ae041
committed
fix(util): create lock files in one exclusive step
`LockFile._obtain_lock_or_raise()` tested for the lock with `osp.isfile()`
and then created it with `open(lock_file, "w")`. Nothing keeps another
holder out between the two calls, so several can pass the test and all of
them set `_owns_lock`. Racing eight holders on one lock file leaves seven
believing they own it, which removes the mutual exclusion that
`GitConfigParser` in write mode and `RefLog.append_entry()` rely on.
`osp.isfile()` also resolves symbolic links. A dangling symlink planted at
`<file>.lock` therefore reports no lock, and the following `open()` resolves
it and creates the target, outside the repository.
Create the lock with `os.open(lock_file, os.O_WRONLY | os.O_CREAT |
os.O_EXCL, 0o600)` instead. That is the single-step exclusive create
`gitdb`'s `LockedFD.open()` and Git's own `lock_file()` already use, and
`O_EXCL` fails with `EEXIST` on a symbolic link rather than resolving it, so
both problems close together. The creation mode matches `LockedFD`; nothing
reads a lock file's contents, and breaking a stale lock needs write
permission on the containing directory rather than on the file.
`FileExistsError` is translated back into the existing "did already exist"
`OSError`, so `BlockingLockFile`'s retry loop and the existing
`test_lock_file` and `test_blocking_lock_file` cases are unaffected. A
directory at the lock path is still reported through the generic `OSError`
branch. This covers acquiring the lock only; writing the locked file stays
the caller's concern, as before.
Adds `test_lock_file_does_not_follow_a_symlink` and
`test_lock_file_is_obtained_by_a_single_holder`. Both fail on the previous
code (`1 != 7`, and the symlink target gets created) and pass here.
Validation: full `pytest` suite green on Python 3.11 on macOS, plus `ruff
check`, `ruff format`, `mypy` and `basedpyright --warnings` clean.1 parent 6fe40b7 commit b0ae041
2 files changed
Lines changed: 48 additions & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -1166,17 +1166,20 @@ def _obtain_lock_or_raise(self) -> None: | |||
| 1166 | 1166 | if self._has_lock(): | |
| 1167 | 1167 | return | |
| 1168 | 1168 | lock_file = self._lock_file_path() | |
| 1169 | - if osp.isfile(lock_file): | ||
| 1169 | + # Create the lock in one step, the way Git and gitdb's LockedFD do. Testing | ||
| 1170 | + # for the file first leaves a window in which another holder creates it and | ||
| 1171 | + # both proceed, and O_CREAT|O_EXCL additionally refuses to follow a symbolic | ||
| 1172 | + # link planted at the lock path instead of writing through it. | ||
| 1173 | + try: | ||
| 1174 | + fd = os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) | ||
| 1175 | + except FileExistsError as e: | ||
| 1170 | 1176 | raise OSError( | |
| 1171 | 1177 | "Lock for file %r did already exist, delete %r in case the lock is illegal" | |
| 1172 | 1178 | % (self._file_path, lock_file) | |
| 1173 | - ) | ||
| 1174 | - | ||
| 1175 | - try: | ||
| 1176 | - with open(lock_file, mode="w"): | ||
| 1177 | - pass | ||
| 1179 | + ) from e | ||
| 1178 | 1180 | except OSError as e: | |
| 1179 | 1181 | raise OSError(str(e)) from e | |
| 1182 | + os.close(fd) | ||
| 1180 | 1183 | ||
| 1181 | 1184 | self._owns_lock = True | |
| 1182 | 1185 | ||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -12,6 +12,7 @@ | |||
| 12 | 12 | import subprocess | |
| 13 | 13 | import sys | |
| 14 | 14 | import tempfile | |
| 15 | + import threading | ||
| 15 | 16 | import time | |
| 16 | 17 | from unittest import SkipTest, mock | |
| 17 | 18 | ||
@@ -446,6 +447,44 @@ def test_lock_file(self): | |||
| 446 | 447 | lock_file._obtain_lock_or_raise() | |
| 447 | 448 | lock_file._release_lock() | |
| 448 | 449 | ||
| 450 | + @requires_symlinks | ||
| 451 | + def test_lock_file_does_not_follow_a_symlink(self): | ||
| 452 | + with tempfile.TemporaryDirectory() as tdir: | ||
| 453 | + my_file = os.path.join(tdir, "my-lock-file") | ||
| 454 | + outside = os.path.join(tdir, "outside-the-lock") | ||
| 455 | + os.symlink(outside, my_file + ".lock") | ||
| 456 | + | ||
| 457 | + lock_file = LockFile(my_file) | ||
| 458 | + self.assertRaises(IOError, lock_file._obtain_lock_or_raise) | ||
| 459 | + assert not lock_file._has_lock() | ||
| 460 | + assert not os.path.exists(outside) | ||
| 461 | + | ||
| 462 | + def test_lock_file_is_obtained_by_a_single_holder(self): | ||
| 463 | + with tempfile.TemporaryDirectory() as tdir: | ||
| 464 | + my_file = os.path.join(tdir, "my-lock-file") | ||
| 465 | + racers = 8 | ||
| 466 | + at_the_line = threading.Barrier(racers) | ||
| 467 | + holders = [] | ||
| 468 | + guard = threading.Lock() | ||
| 469 | + | ||
| 470 | + def obtain(): | ||
| 471 | + lock_file = LockFile(my_file) | ||
| 472 | + at_the_line.wait() | ||
| 473 | + try: | ||
| 474 | + lock_file._obtain_lock_or_raise() | ||
| 475 | + except OSError: | ||
| 476 | + return | ||
| 477 | + with guard: | ||
| 478 | + holders.append(lock_file) | ||
| 479 | + | ||
| 480 | + threads = [threading.Thread(target=obtain) for _ in range(racers)] | ||
| 481 | + for thread in threads: | ||
| 482 | + thread.start() | ||
| 483 | + for thread in threads: | ||
| 484 | + thread.join() | ||
| 485 | + | ||
| 486 | + self.assertEqual(1, len(holders)) | ||
| 487 | + | ||
| 449 | 488 | def test_blocking_lock_file(self): | |
| 450 | 489 | with tempfile.TemporaryDirectory() as tdir: | |
| 451 | 490 | my_file = os.path.join(tdir, "my-lock-file") | |
0 commit comments