Skip to content

Commit

Permalink
Wrap docstrings and comments in _safer_popen_windows
Browse files Browse the repository at this point in the history
It gained an indentation level in dc95a76, so its docstrings and
comments were no longer wrapped to 88 columns as most docstrings
and comments have been (absent a reason not to) since gitpython-developers#1850.

This wraps it, but some parts may benefit from some other
adjustments.
  • Loading branch information
EliahKagan committed Mar 10, 2024
1 parent 465ab56 commit ad8190b
Showing 1 changed file with 34 additions and 27 deletions.
61 changes: 34 additions & 27 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,47 +232,54 @@ def _safer_popen_windows(
env: Optional[Mapping[str, str]] = None,
**kwargs: Any,
) -> Popen:
"""Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
This avoids an untrusted search path condition where a file like ``git.exe`` in a
malicious repository would be run when GitPython operates on the repository. The
process using GitPython may have an untrusted repository's working tree as its
current working directory. Some operations may temporarily change to that directory
before running a subprocess. In addition, while by default GitPython does not run
external commands with a shell, it can be made to do so, in which case the CWD of
the subprocess, which GitPython usually sets to a repository working tree, can
itself be searched automatically by the shell. This wrapper covers all those cases.
"""Call :class:`subprocess.Popen` on Windows but don't include a CWD in the
search.
This avoids an untrusted search path condition where a file like ``git.exe`` in
a malicious repository would be run when GitPython operates on the repository.
The process using GitPython may have an untrusted repository's working tree as
its current working directory. Some operations may temporarily change to that
directory before running a subprocess. In addition, while by default GitPython
does not run external commands with a shell, it can be made to do so, in which
case the CWD of the subprocess, which GitPython usually sets to a repository
working tree, can itself be searched automatically by the shell. This wrapper
covers all those cases.
:note:
This currently works by setting the :envvar:`NoDefaultCurrentDirectoryInExePath`
environment variable during subprocess creation. It also takes care of passing
Windows-specific process creation flags, but that is unrelated to path search.
This currently works by setting the
:envvar:`NoDefaultCurrentDirectoryInExePath` environment variable during
subprocess creation. It also takes care of passing Windows-specific process
creation flags, but that is unrelated to path search.
:note:
The current implementation contains a race condition on :attr:`os.environ`.
GitPython isn't thread-safe, but a program using it on one thread should ideally
be able to mutate :attr:`os.environ` on another, without unpredictable results.
See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
GitPython isn't thread-safe, but a program using it on one thread should
ideally be able to mutate :attr:`os.environ` on another, without
unpredictable results. See comments in:
https://github.com/gitpython-developers/GitPython/pull/1650
"""
# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards.
# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
# https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
creationflags = subprocess.CREATE_NO_WINDOW | subprocess.CREATE_NEW_PROCESS_GROUP

# When using a shell, the shell is the direct subprocess, so the variable must be
# set in its environment, to affect its search behavior. (The "1" can be any value.)
# When using a shell, the shell is the direct subprocess, so the variable must
# be set in its environment, to affect its search behavior. (The "1" can be any
# value.)
if shell:
# The original may be immutable or reused by the caller. Make changes in a copy.
# The original may be immutable or reused by the caller. Make changes in a
# copy.
env = {} if env is None else dict(env)
env["NoDefaultCurrentDirectoryInExePath"] = "1"

# When not using a shell, the current process does the search in a CreateProcessW
# API call, so the variable must be set in our environment. With a shell, this is
# unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
# patched. If that is unpatched, then in the rare case the ComSpec environment
# variable is unset, the search for the shell itself is unsafe. Setting
# NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler and
# protects against that. (As above, the "1" can be any value.)
# When not using a shell, the current process does the search in a
# CreateProcessW API call, so the variable must be set in our environment. With
# a shell, this is unnecessary, in versions where
# https://github.com/python/cpython/issues/101283 is patched. If that is
# unpatched, then in the rare case the ComSpec environment variable is unset,
# the search for the shell itself is unsafe. Setting
# NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler
# and protects against that. (As above, the "1" can be any value.)
with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
return Popen(
command,
Expand Down

0 comments on commit ad8190b

Please sign in to comment.