Movatterモバイル変換

[0]ホーム
URL:

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

 Sign up
Appearance settings

Commit41fac85

Browse files
committed
Avoid mktemp in tests, in straightforward cases
The tempfile.mktemp function is deprecated, because of a racecondition where the file may be concurrently created between whenits name is generated and when it is opened. Other faciliies in thetempfile module overcome this by generating a name, attempting tocreate the file or directory in a way that guarantees failure if italready existed, and, in the occasional case that it did alreadyexist, generating another name and trying again (stopping after apredefined limit). For further information on mktemp deprecation:-https://docs.python.org/3/library/tempfile.html#tempfile.mktemp-gitpython-developers/smmap#41The security risk of calls to mktemp in this project's test suiteis low. However, it is still best to avoid using it, because it isdeprecated, because it is (at least slightly) brittle, and becauseany use of mktemp looks like a potential security risk and therebyimposes a burden on working with the code (which could potentiallybe addressed with detailed comments analyzing why it is believedsafe in particular cases, but this would typically be more verbose,and at least as challenging to add, as replacing mktemp with abetter alternative).This commit replaces *some* uses of mktemp in the test suite: thosewhere it is readily clear how to do so in a way that preserves thecode'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 theirintent, with respect to whether the file should be retained after atest failure. Others appear deliberately to avoid creating a fileor directory where the code under test should do so, possibly tocheck that this is done properly. (One way to preserve that latterbehavior, while avoiding the weakness of using mktemp and alsoavoiding 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 codeunder test (i.e., outside the test suite, inside the git module).
1 parent3ac7e78 commit41fac85

File tree

6 files changed

+42
-45
lines changed

6 files changed

+42
-45
lines changed

‎test/lib/helper.py

Lines 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
defwrapper(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
returnfunc(self,path)

‎test/performance/lib.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,8 +65,7 @@ class TestBigRepoRW(TestBigRepoR):
6565
defsetUp(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

Lines 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
assertdata
7070

71-
tmpfilename=tempfile.mktemp(suffix="test-stream")
72-
withopen(tmpfilename,"wb+")astmpfile:
71+
withtempfile.NamedTemporaryFile(suffix="test-stream",delete=False)astmpfile:
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

Lines 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-
importos
4+
importos.pathasosp
55
importtempfile
66

77
fromgit.objectsimportIndexObject
88
fromgit.refsimportRefLogEntry,RefLog
99
fromtest.libimportTestBase,fixture_path
1010
fromgit.utilimportActor,rmtree,hex_to_bin
1111

12-
importos.pathasosp
13-
1412

1513
classTestRefLog(TestBase):
1614
deftest_reflogentry(self):
@@ -35,8 +33,7 @@ def test_reflogentry(self):
3533
deftest_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
assertosp.isfile(rlp_master_ro)

‎test/test_repo.py

Lines 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
deftest_archive(self):
670-
tmpfile=tempfile.mktemp(suffix="archive-test")
671-
withopen(tmpfile,"wb")asstream:
670+
withtempfile.NamedTemporaryFile("wb",suffix="archive-test",delete=False)asstream:
672671
self.rorepo.archive(stream,"0.1.6",path="doc")
673672
assertstream.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
deftest_should_display_blame_information(self,git):

‎test/test_util.py

Lines 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
deftest_lock_file(self):
362-
my_file=tempfile.mktemp()
363-
lock_file=LockFile(my_file)
364-
assertnotlock_file._has_lock()
365-
# Release lock we don't have - fine.
366-
lock_file._release_lock()
362+
withtempfile.TemporaryDirectory()astdir:
363+
my_file=os.path.join(tdir,"my-lock-file")
364+
lock_file=LockFile(my_file)
365+
assertnotlock_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-
assertlock_file._has_lock()
369+
# Get lock.
370+
lock_file._obtain_lock_or_raise()
371+
assertlock_file._has_lock()
371372

372-
# Concurrent access.
373-
other_lock_file=LockFile(my_file)
374-
assertnotother_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+
assertnotother_lock_file._has_lock()
376+
self.assertRaises(IOError,other_lock_file._obtain_lock_or_raise)
376377

377-
lock_file._release_lock()
378-
assertnotlock_file._has_lock()
378+
lock_file._release_lock()
379+
assertnotlock_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-
delother_lock_file
385-
lock_file._obtain_lock_or_raise()
386-
lock_file._release_lock()
384+
# Auto-release on destruction.
385+
delother_lock_file
386+
lock_file._obtain_lock_or_raise()
387+
lock_file._release_lock()
387388

388389
deftest_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+
withtempfile.TemporaryDirectory()astdir:
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
ifos.name=="nt"orsys.platform=="cygwin":
401404
extra_time*=6# Without this, we get indeterministic failures on Windows.
402405
elifsys.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
deftest_user_id(self):

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp