Skip to content

Commit

Permalink
Avoid subprocess-writable temp file race condition
Browse files Browse the repository at this point in the history
This lets the Windows subprocess open or rename onto the temporary
file using a more robust approach than in b12fd4a, avoiding the
race condition described there, where the filename could be
inadvertently reused between deletion and recreation of the file.

This creates a context manager helper for the temporary index file
used in IndexFile.from_tree, whose implementation differs by
operating system:

- Delegating straightforwardly to NamedTempoaryFile on POSIX
  systems where an open file can replaced by having another file
  renamed to it (just as it can be deleted).

- Employing custom logic on Windows, using mkstemp, closing the
  temporary file without immediately deleting it (so it won't be
  reused by any process seeking to create a temporary file), and
  then deleting it on context manager exit.

IndexFile.from_tree now calls this helper instead of
NamedTemporaryFile. For convenience, the helper provides the path,
i.e. the "name", when entered, so tmp_index is now just that path.

(At least for now, this helper is implemented as a nonpublic
function in the git.index.base module, rather than in the
git.index.util module. If it were public in git.index.util like the
other facilities there, then some later changes to it, or its later
removal, would be a breaking change to GitPython. If it were
nonpublic in git.index.util, then this would not be a concern, but
it would be unintuitive for it to be accessed from code in the
git.index.base module. In the future, one way to address this might
be to have one or more nonpublic _util modules with public members.
Because it would still be a breaking change to drop existing public
util modules, that would be more utility modules in total, so such
a change isn't included here just for this one used-once function.)
  • Loading branch information
EliahKagan committed Dec 1, 2023
1 parent 928ca1e commit 9e5d0aa
Showing 1 changed file with 27 additions and 7 deletions.
34 changes: 27 additions & 7 deletions git/index/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"""Module containing IndexFile, an Index implementation facilitating all kinds of index
manipulations such as querying and merging."""

from contextlib import ExitStack
import contextlib
import datetime
import glob
from io import BytesIO
Expand Down Expand Up @@ -67,6 +67,7 @@
BinaryIO,
Callable,
Dict,
Generator,
IO,
Iterable,
Iterator,
Expand Down Expand Up @@ -96,6 +97,27 @@
__all__ = ("IndexFile", "CheckoutError", "StageType")


@contextlib.contextmanager
def _named_temporary_file_for_subprocess(directory: PathLike) -> Generator[str, None, None]:
"""Create a named temporary file git subprocesses can open, deleting it afterward.
:param directory: The directory in which the file is created.
:return: A context manager object that creates the file and provides its name on
entry, and deletes it on exit.
"""
if os.name == "nt":
fd, name = tempfile.mkstemp(dir=directory)
os.close(fd)
try:
yield name
finally:
os.remove(name)
else:
with tempfile.NamedTemporaryFile(dir=directory) as ctx:
yield ctx.name


class IndexFile(LazyMixin, git_diff.Diffable, Serializable):
"""An Index that can be manipulated using a native implementation in order to save
git command function calls wherever possible.
Expand Down Expand Up @@ -359,11 +381,9 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile

# tmp file created in git home directory to be sure renaming
# works - /tmp/ dirs could be on another device.
with ExitStack() as stack:
tmp_index = stack.enter_context(tempfile.NamedTemporaryFile(dir=repo.git_dir))
if os.name == "nt":
tmp_index.close()
arg_list.append("--index-output=%s" % tmp_index.name)
with contextlib.ExitStack() as stack:
tmp_index = stack.enter_context(_named_temporary_file_for_subprocess(repo.git_dir))
arg_list.append("--index-output=%s" % tmp_index)
arg_list.extend(treeish)

# Move current index out of the way - otherwise the merge may fail
Expand All @@ -373,7 +393,7 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile

stack.enter_context(TemporaryFileSwap(join_path_native(repo.git_dir, "index")))
repo.git.read_tree(*arg_list, **kwargs)
index = cls(repo, tmp_index.name)
index = cls(repo, tmp_index)
index.entries # Force it to read the file as we will delete the temp-file.
return index
# END index merge handling
Expand Down

0 comments on commit 9e5d0aa

Please sign in to comment.