Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 41fac85

Browse filesBrowse files
EliahKaganEliahKagan
committed
Avoid mktemp in tests, in straightforward cases
The tempfile.mktemp function is deprecated, because of a race condition where the file may be concurrently created between when its name is generated and when it is opened. Other faciliies in the tempfile module overcome this by generating a name, attempting to create the file or directory in a way that guarantees failure if it already existed, and, in the occasional case that it did already exist, generating another name and trying again (stopping after a predefined limit). For further information on mktemp deprecation: - https://docs.python.org/3/library/tempfile.html#tempfile.mktemp - gitpython-developers/smmap#41 The security risk of calls to mktemp in this project's test suite is low. However, it is still best to avoid using it, because it is deprecated, because it is (at least slightly) brittle, and because any use of mktemp looks like a potential security risk and thereby imposes a burden on working with the code (which could potentially be addressed with detailed comments analyzing why it is believed safe in particular cases, but this would typically be more verbose, and at least as challenging to add, as replacing mktemp with a better alternative). This commit replaces *some* uses of mktemp in the test suite: those where it is readily clear how to do so in a way that preserves the code's intent: - Where a name for a temporary directory is generated with mktemp and os.mkdir is called immediately, mkdtemp is now used. - Where a name for a temporary file that is not customized (such as with a prefix) is generated with mktemp, such that the code under test never uses the filename but only the already-open file-like object, TemporaryFile is now used. As the name isn't customized, the test code in these cases does not express an intent to allow the developer to inspect the file after a test failure, so even if the file wasn't guaranteed to be deleted with a finally block or context manager, it is fine to do so. TemporaryFile supports this use case well on all systems including Windows, and automatically deletes the file. - Where a name for a temporary file that *is* customized (such as with a prefix) to reflect the way the test uses it is generated with mktemp, and the test code does not attempt deterministic deletion of the file when an exception would make the test fail, NamedTemporaryFile with delete=False is now used. The original code to remove the file when the test succeeds is modified accordingly to do the same job, and also commented to explain that it is being handled this way to allow the file to be kept and examined when a test failure occurs. Other cases in the test suite should also be feasible to replace, but are left alone for now. Some of them are ambiguous in their intent, with respect to whether the file should be retained after a test failure. Others appear deliberately to avoid creating a file or directory where the code under test should do so, possibly to check that this is done properly. (One way to preserve that latter behavior, while avoiding the weakness of using mktemp and also avoiding inadverently reproducing that weakness by other means, could be to use a path in a temporary directory made for the test.) This commit also doesn't address the one use of mktemp in the code under test (i.e., outside the test suite, inside the git module).
1 parent 3ac7e78 commit 41fac85
Copy full SHA for 41fac85

File tree

6 files changed

+42
-45
lines changed
Filter options

6 files changed

+42
-45
lines changed

‎test/lib/helper.py

Copy file name to clipboardExpand all lines: test/lib/helper.py
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,8 +89,7 @@ def with_rw_directory(func):
8989

9090
@wraps(func)
9191
def wrapper(self):
92-
path = tempfile.mktemp(prefix=func.__name__)
93-
os.mkdir(path)
92+
path = tempfile.mkdtemp(prefix=func.__name__)
9493
keep = False
9594
try:
9695
return func(self, path)

‎test/performance/lib.py

Copy file name to clipboardExpand all lines: test/performance/lib.py
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,7 @@ class TestBigRepoRW(TestBigRepoR):
6565
def setUp(self):
6666
self.gitrwrepo = None
6767
super().setUp()
68-
dirname = tempfile.mktemp()
69-
os.mkdir(dirname)
68+
dirname = tempfile.mkdtemp()
7069
self.gitrwrepo = self.gitrorepo.clone(dirname, shared=True, bare=True, odbt=GitCmdObjectDB)
7170
self.puregitrwrepo = Repo(dirname, odbt=GitDB)
7271

‎test/test_base.py

Copy file name to clipboardExpand all lines: test/test_base.py
+2-3Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -68,12 +68,11 @@ def test_base_object(self):
6868
data = data_stream.read()
6969
assert data
7070

71-
tmpfilename = tempfile.mktemp(suffix="test-stream")
72-
with open(tmpfilename, "wb+") as tmpfile:
71+
with tempfile.NamedTemporaryFile(suffix="test-stream", delete=False) as tmpfile:
7372
self.assertEqual(item, item.stream_data(tmpfile))
7473
tmpfile.seek(0)
7574
self.assertEqual(tmpfile.read(), data)
76-
os.remove(tmpfilename)
75+
os.remove(tmpfile.name) # Do it this way so we can inspect the file on failure.
7776
# END for each object type to create
7877

7978
# Each has a unique sha.

‎test/test_reflog.py

Copy file name to clipboardExpand all lines: test/test_reflog.py
+2-5Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,14 @@
11
# This module is part of GitPython and is released under the
22
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/
33

4-
import os
4+
import os.path as osp
55
import tempfile
66

77
from git.objects import IndexObject
88
from git.refs import RefLogEntry, RefLog
99
from test.lib import TestBase, fixture_path
1010
from git.util import Actor, rmtree, hex_to_bin
1111

12-
import os.path as osp
13-
1412

1513
class TestRefLog(TestBase):
1614
def test_reflogentry(self):
@@ -35,8 +33,7 @@ def test_reflogentry(self):
3533
def test_base(self):
3634
rlp_head = fixture_path("reflog_HEAD")
3735
rlp_master = fixture_path("reflog_master")
38-
tdir = tempfile.mktemp(suffix="test_reflogs")
39-
os.mkdir(tdir)
36+
tdir = tempfile.mkdtemp(suffix="test_reflogs")
4037

4138
rlp_master_ro = RefLog.path(self.rorepo.head)
4239
assert osp.isfile(rlp_master_ro)

‎test/test_repo.py

Copy file name to clipboardExpand all lines: test/test_repo.py
+2-3Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -667,11 +667,10 @@ def test_tag_to_full_tag_path(self):
667667
self.assertEqual(value_errors, [])
668668

669669
def test_archive(self):
670-
tmpfile = tempfile.mktemp(suffix="archive-test")
671-
with open(tmpfile, "wb") as stream:
670+
with tempfile.NamedTemporaryFile("wb", suffix="archive-test", delete=False) as stream:
672671
self.rorepo.archive(stream, "0.1.6", path="doc")
673672
assert stream.tell()
674-
os.remove(tmpfile)
673+
os.remove(stream.name) # Do it this way so we can inspect the file on failure.
675674

676675
@mock.patch.object(Git, "_call_process")
677676
def test_should_display_blame_information(self, git):

‎test/test_util.py

Copy file name to clipboardExpand all lines: test/test_util.py
+34-30Lines changed: 34 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -359,48 +359,52 @@ def test_it_should_dashify(self):
359359
self.assertEqual("foo", dashify("foo"))
360360

361361
def test_lock_file(self):
362-
my_file = tempfile.mktemp()
363-
lock_file = LockFile(my_file)
364-
assert not lock_file._has_lock()
365-
# Release lock we don't have - fine.
366-
lock_file._release_lock()
362+
with tempfile.TemporaryDirectory() as tdir:
363+
my_file = os.path.join(tdir, "my-lock-file")
364+
lock_file = LockFile(my_file)
365+
assert not lock_file._has_lock()
366+
# Release lock we don't have - fine.
367+
lock_file._release_lock()
367368

368-
# Get lock.
369-
lock_file._obtain_lock_or_raise()
370-
assert lock_file._has_lock()
369+
# Get lock.
370+
lock_file._obtain_lock_or_raise()
371+
assert lock_file._has_lock()
371372

372-
# Concurrent access.
373-
other_lock_file = LockFile(my_file)
374-
assert not other_lock_file._has_lock()
375-
self.assertRaises(IOError, other_lock_file._obtain_lock_or_raise)
373+
# Concurrent access.
374+
other_lock_file = LockFile(my_file)
375+
assert not other_lock_file._has_lock()
376+
self.assertRaises(IOError, other_lock_file._obtain_lock_or_raise)
376377

377-
lock_file._release_lock()
378-
assert not lock_file._has_lock()
378+
lock_file._release_lock()
379+
assert not lock_file._has_lock()
379380

380-
other_lock_file._obtain_lock_or_raise()
381-
self.assertRaises(IOError, lock_file._obtain_lock_or_raise)
381+
other_lock_file._obtain_lock_or_raise()
382+
self.assertRaises(IOError, lock_file._obtain_lock_or_raise)
382383

383-
# Auto-release on destruction.
384-
del other_lock_file
385-
lock_file._obtain_lock_or_raise()
386-
lock_file._release_lock()
384+
# Auto-release on destruction.
385+
del other_lock_file
386+
lock_file._obtain_lock_or_raise()
387+
lock_file._release_lock()
387388

388389
def test_blocking_lock_file(self):
389-
my_file = tempfile.mktemp()
390-
lock_file = BlockingLockFile(my_file)
391-
lock_file._obtain_lock()
392-
393-
# Next one waits for the lock.
394-
start = time.time()
395-
wait_time = 0.1
396-
wait_lock = BlockingLockFile(my_file, 0.05, wait_time)
397-
self.assertRaises(IOError, wait_lock._obtain_lock)
398-
elapsed = time.time() - start
390+
with tempfile.TemporaryDirectory() as tdir:
391+
my_file = os.path.join(tdir, "my-lock-file")
392+
lock_file = BlockingLockFile(my_file)
393+
lock_file._obtain_lock()
394+
395+
# Next one waits for the lock.
396+
start = time.time()
397+
wait_time = 0.1
398+
wait_lock = BlockingLockFile(my_file, 0.05, wait_time)
399+
self.assertRaises(IOError, wait_lock._obtain_lock)
400+
elapsed = time.time() - start
401+
399402
extra_time = 0.02
400403
if os.name == "nt" or sys.platform == "cygwin":
401404
extra_time *= 6 # Without this, we get indeterministic failures on Windows.
402405
elif sys.platform == "darwin":
403406
extra_time *= 18 # The situation on macOS is similar, but with more delay.
407+
404408
self.assertLess(elapsed, wait_time + extra_time)
405409

406410
def test_user_id(self):

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.