Skip to content

Commit

Permalink
Replace the one use of mktemp in the git module
Browse files Browse the repository at this point in the history
This makes two related changes to git.index.util.TemporaryFileSwap:

- Replace mktemp with mkstemp and then immediately closing the file.
  This avoids two possible name clashes: the usual name clash where
  the file may be created by another process between when mktemp
  generates the name and when the file is created, and the problem
  that mktemp does not check for files that contain the generated
  name as a part. The latter is specific to the use here, where
  a string generated by mktemp was manually concatenated to the
  base filename. This addresses that by passing the filename as the
  prefix for mkstemp to use.

- Replace os.remove with os.replace and simplify. This is made
  necessary on Windows by the file already existing, due to mkstemp
  creating it. Deleting the file and allowing it to be recreated
  would reproduce the mktemp race condition in full (obscured and
  with extra steps). But os.replace supports an existing target
  file on all platforms. It is now also used in __exit__, where it
  allows the Windows check for the file to be removed, and (in any
  OS) better expresses the intent of the code to human readers.
  Furthermore, because one of the "look before you leap" checks in
  __exit__ is removed, the minor race condition in cleaning up the
  file is somewhat decreased.

A small amount of related refactoring is included. The interface is
not changed, even where it could be simplified (by letting __exit__
return None), and resource acquisition remains done on construction
rather than in __enter__, because changing those aspects of the
design could break some existing uses.

Although the use of mktemp replaced here was in the git module and
not the test suite, its use was to generate filenames for use in a
directory that would ordinarily be under the user's control, such
that the ability to carry out typical mktemp-related attacks would
already require the ability to achieve the same goals more easily
and reliably. Although TemporaryFileSwap is a public class that may
be used directly by applications, it is only useful for making a
temporary file in the same directory as an existing file. Thus the
intended benefits of this change are mainly to code quality and
a slight improvement in robustness.
  • Loading branch information
EliahKagan committed Dec 13, 2023
1 parent 41fac85 commit 9e86053
Showing 1 changed file with 6 additions and 10 deletions.
16 changes: 6 additions & 10 deletions git/index/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

"""Index utilities."""

import contextlib
from functools import wraps
import os
import os.path as osp
Expand Down Expand Up @@ -40,12 +41,10 @@ class TemporaryFileSwap:

def __init__(self, file_path: PathLike) -> None:
self.file_path = file_path
self.tmp_file_path = str(self.file_path) + tempfile.mktemp("", "", "")
# It may be that the source does not exist.
try:
os.rename(self.file_path, self.tmp_file_path)
except OSError:
pass
fd, self.tmp_file_path = tempfile.mkstemp(prefix=self.file_path, dir="")
os.close(fd)
with contextlib.suppress(OSError): # It may be that the source does not exist.
os.replace(self.file_path, self.tmp_file_path)

def __enter__(self) -> "TemporaryFileSwap":
return self
Expand All @@ -57,10 +56,7 @@ def __exit__(
exc_tb: Optional[TracebackType],
) -> bool:
if osp.isfile(self.tmp_file_path):
if os.name == "nt" and osp.exists(self.file_path):
os.remove(self.file_path)
os.rename(self.tmp_file_path, self.file_path)

os.replace(self.tmp_file_path, self.file_path)
return False


Expand Down

0 comments on commit 9e86053

Please sign in to comment.