Skip to content

Commit

Permalink
Fix mypy error with creationflags in subprocess module
Browse files Browse the repository at this point in the history
On Windows, Python's subprocess module contains constants useful to
pass to the `creationflags` parameter of subprocess.Popen. These
are absent on other platforms, where they are not meaningful.

The code was already safe at runtime from any AttributeError
related to these constants, because they were only used in
git.cmd._safer_popen_windows, which was defined on Windows.

But in gitpython-developers#1792 I did not write the code in a way where mypy can
verify its correctness. So if a regression of the kind mypy can in
principle catch were to occur, it would be harder to notice it.

This refactors the code, keeping the same behavior but expressing
it in a way mypy can understand. This consists of two changes:

1. Only define _safer_popen_windows when the platform is Windows,
   placing it in the first branch of the `if` statement.

   This is needed because mypy will not take the only current call
   to that nonpublic function being on Windows as sufficient
   evidence that the platform is always Windows when it is run.

2. Determine the platform, for this purpose, using sys.platform
   instead of os.name.

   These are far from equivalent in general (see the deprecation
   rationale for is_<platform> in gitpython-developers#1732, revised in a0fa2bd
   in gitpython-developers#1787). However, in Python 3 (GitPython no longer supports
   Python 2), in the specific case of Windows, we have a choice of
   which to use, as both `sys.platform == "win32"` and
   `os.name == "nt"`.

   os.name is "nt" on native Windows, and "posix" on Cygwin.
   sys.platform is "win32" on native Windows (including 64-bit
   systems with 64-bit Python builds), and "cygwin" on Cygwin. See:
   https://docs.python.org/3/library/sys.html#sys.platform

   This is needed because the type stubs for the subprocess module
   use this sys.platform check (rather than an os.name check) to
   determine if the platform is Windows for the purpose of deciding
   which constants to say the subprocess module defines.

I have verified that neither of these changes is enough by itself.
  • Loading branch information
EliahKagan committed Mar 7, 2024
1 parent 43b7f8a commit dc95a76
Showing 1 changed file with 57 additions and 56 deletions.
113 changes: 57 additions & 56 deletions git/cmd.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import signal
from subprocess import Popen, PIPE, DEVNULL
import subprocess
import sys
import threading
from textwrap import dedent

Expand Down Expand Up @@ -220,67 +221,67 @@ def pump_stream(
finalizer(process)


def _safer_popen_windows(
command: Union[str, Sequence[Any]],
*,
shell: bool = False,
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.
safer_popen: Callable[..., Popen]

: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.
if sys.platform == "win32":

: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.
"""
# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
# 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.)
if shell:
# 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.)
with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
return Popen(
command,
shell=shell,
env=env,
creationflags=creationflags,
**kwargs,
)
def _safer_popen_windows(
command: Union[str, Sequence[Any]],
*,
shell: bool = False,
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.
: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.
safer_popen: Callable[..., Popen]
: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.
"""
# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
# 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.)
if shell:
# 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.)
with patch_env("NoDefaultCurrentDirectoryInExePath", "1"):
return Popen(
command,
shell=shell,
env=env,
creationflags=creationflags,
**kwargs,
)

if os.name == "nt":
safer_popen = _safer_popen_windows
else:
safer_popen = Popen
Expand Down

0 comments on commit dc95a76

Please sign in to comment.