FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(util): acquire Windows locks without following symlinks · gitpython-developers/GitPython@97a5468 · GitHub

  1. gitpython-developers
  2. GitPython

Repository navigation

Commit 97a5468

Browse files
Byron
and
codex
committed
fix(util): acquire Windows locks without following symlinks
<!-- Byron --> Took brief look only. Hope this code soon won't be present anymore. <!-- agent --> Windows follows dangling symlinks even when `os.open` uses `O_CREAT | O_EXCL`, so acquiring a lock could create its symlink target and incorrectly report ownership. The `_winapi.CreateFile` workaround also uses the ANSI API on Python 3.8 through 3.10, causing failures in Unicode directories or creating locks under mangled filenames. Use `CreateFileW` with `CREATE_NEW` and `FILE_FLAG_OPEN_REPARSE_POINT` to create the lock atomically while rejecting existing links. Declare the `ctypes` argument and return types explicitly so Unicode paths and native handle sizes are preserved. Close the handle before recording ownership, propagate Windows errors, and reject embedded NULs before the native API can truncate a path. Preserve exclusive `os.open` creation on POSIX. Expand the lock tests to cover Unicode filenames and directories, including non-BMP characters, and verify that the requested lock path is actually created and removed. Check NUL rejection, preserve both existing and missing symlink targets, and explicitly release the concurrent test's acquired locks. Reproduced the CI failure in `test_clone_from_with_path_contains_unicode` on Windows/Python 3.8.10 before the fix. The affected utility, clone, and configuration modules pass on Python 3.8.10 and 3.13.14: 169 passed, 41 skipped, and 2 expected failures on each version. The new Unicode and NUL regressions also failed before their respective fixes. `ruff check`, `ruff format --check`, `mypy --python-version=3.13`, and `basedpyright --warnings` pass. Assisted-by: GPT 6.0 Astra Co-authored-by: GPT 6 <codex@openai.com>
1 parent b0ae041 commit 97a5468

2 files changed

Lines changed: 76 additions & 11 deletions

File tree

‎git/util.py‎

Lines changed: 44 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1166,20 +1166,58 @@ def _obtain_lock_or_raise(self) -> None:
11661166
if self._has_lock():
11671167
return
11681168
lock_file = self._lock_file_path()
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.
1169+
# Create the lock in one step. Checking for it first would allow another
1170+
# holder to create it between the check and the open.
11731171
try:
1174-
fd = os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
1172+
if sys.platform == "win32":
1173+
if "\0" in lock_file:
1174+
raise ValueError("embedded null character")
1175+
1176+
import ctypes
1177+
from ctypes import wintypes
1178+
1179+
# Unlike POSIX, Windows follows dangling symlinks even with O_EXCL.
1180+
# Open the reparse point itself so an existing link is rejected.
1181+
# Call the Unicode API directly: older _winapi.CreateFile wrappers
1182+
# use the ANSI API and can create a lock under the wrong filename.
1183+
kernel32 = ctypes.WinDLL("kernel32", use_last_error=True)
1184+
create_file = kernel32.CreateFileW
1185+
create_file.argtypes = (
1186+
wintypes.LPCWSTR,
1187+
wintypes.DWORD,
1188+
wintypes.DWORD,
1189+
wintypes.LPVOID,
1190+
wintypes.DWORD,
1191+
wintypes.DWORD,
1192+
wintypes.HANDLE,
1193+
)
1194+
create_file.restype = wintypes.HANDLE
1195+
close_handle = kernel32.CloseHandle
1196+
close_handle.argtypes = (wintypes.HANDLE,)
1197+
close_handle.restype = wintypes.BOOL
1198+
handle = create_file(
1199+
lock_file,
1200+
0x40000000, # GENERIC_WRITE
1201+
0,
1202+
None,
1203+
1, # CREATE_NEW
1204+
0x00200000, # FILE_FLAG_OPEN_REPARSE_POINT
1205+
None,
1206+
)
1207+
if handle == wintypes.HANDLE(-1).value:
1208+
raise ctypes.WinError(ctypes.get_last_error())
1209+
if not close_handle(handle):
1210+
raise ctypes.WinError(ctypes.get_last_error())
1211+
else:
1212+
fd = os.open(lock_file, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
1213+
os.close(fd)
11751214
except FileExistsError as e:
11761215
raise OSError(
11771216
"Lock for file %r did already exist, delete %r in case the lock is illegal"
11781217
% (self._file_path, lock_file)
11791218
) from e
11801219
except OSError as e:
11811220
raise OSError(str(e)) from e
1182-
os.close(fd)
11831221

11841222
self._owns_lock = True
11851223

‎test/test_util.py‎

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -419,9 +419,11 @@ def test_it_should_dashify(self):
419419
self.assertEqual("this-is-my-argument", dashify("this_is_my_argument"))
420420
self.assertEqual("foo", dashify("foo"))
421421

422-
def test_lock_file(self):
422+
@ddt.data("my-lock-file", "my-lock-file-\u0394", "\u0394/my-lock-file", "\U0001f680/my-lock-file")
423+
def test_lock_file(self, filename):
423424
with tempfile.TemporaryDirectory() as tdir:
424-
my_file = os.path.join(tdir, "my-lock-file")
425+
my_file = os.path.join(tdir, filename)
426+
os.makedirs(os.path.dirname(my_file), exist_ok=True)
425427
lock_file = LockFile(my_file)
426428
assert not lock_file._has_lock()
427429
# Release lock we don't have - fine.
@@ -430,6 +432,7 @@ def test_lock_file(self):
430432
# Get lock.
431433
lock_file._obtain_lock_or_raise()
432434
assert lock_file._has_lock()
435+
assert os.path.isfile(my_file + ".lock")
433436

434437
# Concurrent access.
435438
other_lock_file = LockFile(my_file)
@@ -438,6 +441,7 @@ def test_lock_file(self):
438441

439442
lock_file._release_lock()
440443
assert not lock_file._has_lock()
444+
assert not os.path.exists(my_file + ".lock")
441445

442446
other_lock_file._obtain_lock_or_raise()
443447
self.assertRaises(IOError, lock_file._obtain_lock_or_raise)
@@ -447,17 +451,36 @@ def test_lock_file(self):
447451
lock_file._obtain_lock_or_raise()
448452
lock_file._release_lock()
449453

454+
def test_lock_file_rejects_embedded_nul(self):
455+
with tempfile.TemporaryDirectory() as tdir:
456+
my_file = os.path.join(tdir, "my-lock-file")
457+
lock_file = LockFile(my_file + "\0suffix")
458+
self.assertRaises(ValueError, lock_file._obtain_lock_or_raise)
459+
assert not lock_file._has_lock()
460+
assert not os.path.exists(my_file)
461+
462+
@ddt.data(False, True)
450463
@requires_symlinks
451-
def test_lock_file_does_not_follow_a_symlink(self):
464+
def test_lock_file_does_not_follow_a_symlink(self, target_exists):
452465
with tempfile.TemporaryDirectory() as tdir:
453466
my_file = os.path.join(tdir, "my-lock-file")
454467
outside = os.path.join(tdir, "outside-the-lock")
468+
content = b"Do not modify the symlink target."
469+
if target_exists:
470+
with open(outside, "wb") as stream:
471+
stream.write(content)
455472
os.symlink(outside, my_file + ".lock")
456473

457474
lock_file = LockFile(my_file)
458475
self.assertRaises(IOError, lock_file._obtain_lock_or_raise)
459476
assert not lock_file._has_lock()
460-
assert not os.path.exists(outside)
477+
lock_file._release_lock()
478+
assert os.path.islink(my_file + ".lock")
479+
if target_exists:
480+
with open(outside, "rb") as stream:
481+
self.assertEqual(stream.read(), content)
482+
else:
483+
assert not os.path.exists(outside)
461484

462485
def test_lock_file_is_obtained_by_a_single_holder(self):
463486
with tempfile.TemporaryDirectory() as tdir:
@@ -483,7 +506,11 @@ def obtain():
483506
for thread in threads:
484507
thread.join()
485508

486-
self.assertEqual(1, len(holders))
509+
try:
510+
self.assertEqual(1, len(holders))
511+
finally:
512+
for lock_file in holders:
513+
lock_file._release_lock()
487514

488515
def test_blocking_lock_file(self):
489516
with tempfile.TemporaryDirectory() as tdir:

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL