Skip to content

Commit

Permalink
Avoid mktemp in tests, in straightforward cases
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
EliahKagan committed Dec 13, 2023
1 parent 3ac7e78 commit 41fac85
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 45 deletions.
3 changes: 1 addition & 2 deletions test/lib/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,7 @@ def with_rw_directory(func):

@wraps(func)
def wrapper(self):
path = tempfile.mktemp(prefix=func.__name__)
os.mkdir(path)
path = tempfile.mkdtemp(prefix=func.__name__)
keep = False
try:
return func(self, path)
Expand Down
3 changes: 1 addition & 2 deletions test/performance/lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,7 @@ class TestBigRepoRW(TestBigRepoR):
def setUp(self):
self.gitrwrepo = None
super().setUp()
dirname = tempfile.mktemp()
os.mkdir(dirname)
dirname = tempfile.mkdtemp()
self.gitrwrepo = self.gitrorepo.clone(dirname, shared=True, bare=True, odbt=GitCmdObjectDB)
self.puregitrwrepo = Repo(dirname, odbt=GitDB)

Expand Down
5 changes: 2 additions & 3 deletions test/test_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,12 +68,11 @@ def test_base_object(self):
data = data_stream.read()
assert data

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

# Each has a unique sha.
Expand Down
7 changes: 2 additions & 5 deletions test/test_reflog.py
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
# This module is part of GitPython and is released under the
# 3-Clause BSD License: https://opensource.org/license/bsd-3-clause/

import os
import os.path as osp
import tempfile

from git.objects import IndexObject
from git.refs import RefLogEntry, RefLog
from test.lib import TestBase, fixture_path
from git.util import Actor, rmtree, hex_to_bin

import os.path as osp


class TestRefLog(TestBase):
def test_reflogentry(self):
Expand All @@ -35,8 +33,7 @@ def test_reflogentry(self):
def test_base(self):
rlp_head = fixture_path("reflog_HEAD")
rlp_master = fixture_path("reflog_master")
tdir = tempfile.mktemp(suffix="test_reflogs")
os.mkdir(tdir)
tdir = tempfile.mkdtemp(suffix="test_reflogs")

rlp_master_ro = RefLog.path(self.rorepo.head)
assert osp.isfile(rlp_master_ro)
Expand Down
5 changes: 2 additions & 3 deletions test/test_repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -667,11 +667,10 @@ def test_tag_to_full_tag_path(self):
self.assertEqual(value_errors, [])

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

@mock.patch.object(Git, "_call_process")
def test_should_display_blame_information(self, git):
Expand Down
64 changes: 34 additions & 30 deletions test/test_util.py
Original file line number Diff line number Diff line change
Expand Up @@ -359,48 +359,52 @@ def test_it_should_dashify(self):
self.assertEqual("foo", dashify("foo"))

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

# Get lock.
lock_file._obtain_lock_or_raise()
assert lock_file._has_lock()
# Get lock.
lock_file._obtain_lock_or_raise()
assert lock_file._has_lock()

# Concurrent access.
other_lock_file = LockFile(my_file)
assert not other_lock_file._has_lock()
self.assertRaises(IOError, other_lock_file._obtain_lock_or_raise)
# Concurrent access.
other_lock_file = LockFile(my_file)
assert not other_lock_file._has_lock()
self.assertRaises(IOError, other_lock_file._obtain_lock_or_raise)

lock_file._release_lock()
assert not lock_file._has_lock()
lock_file._release_lock()
assert not lock_file._has_lock()

other_lock_file._obtain_lock_or_raise()
self.assertRaises(IOError, lock_file._obtain_lock_or_raise)
other_lock_file._obtain_lock_or_raise()
self.assertRaises(IOError, lock_file._obtain_lock_or_raise)

# Auto-release on destruction.
del other_lock_file
lock_file._obtain_lock_or_raise()
lock_file._release_lock()
# Auto-release on destruction.
del other_lock_file
lock_file._obtain_lock_or_raise()
lock_file._release_lock()

def test_blocking_lock_file(self):
my_file = tempfile.mktemp()
lock_file = BlockingLockFile(my_file)
lock_file._obtain_lock()

# Next one waits for the lock.
start = time.time()
wait_time = 0.1
wait_lock = BlockingLockFile(my_file, 0.05, wait_time)
self.assertRaises(IOError, wait_lock._obtain_lock)
elapsed = time.time() - start
with tempfile.TemporaryDirectory() as tdir:
my_file = os.path.join(tdir, "my-lock-file")
lock_file = BlockingLockFile(my_file)
lock_file._obtain_lock()

# Next one waits for the lock.
start = time.time()
wait_time = 0.1
wait_lock = BlockingLockFile(my_file, 0.05, wait_time)
self.assertRaises(IOError, wait_lock._obtain_lock)
elapsed = time.time() - start

extra_time = 0.02
if os.name == "nt" or sys.platform == "cygwin":
extra_time *= 6 # Without this, we get indeterministic failures on Windows.
elif sys.platform == "darwin":
extra_time *= 18 # The situation on macOS is similar, but with more delay.

self.assertLess(elapsed, wait_time + extra_time)

def test_user_id(self):
Expand Down

0 comments on commit 41fac85

Please sign in to comment.