Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitdc95a76

Browse files
committed
Fix mypy error with creationflags in subprocess module
On Windows, Python's subprocess module contains constants useful topass to the `creationflags` parameter of subprocess.Popen. Theseare absent on other platforms, where they are not meaningful.The code was already safe at runtime from any AttributeErrorrelated to these constants, because they were only used ingit.cmd._safer_popen_windows, which was defined on Windows.But in#1792 I did not write the code in a way where mypy canverify its correctness. So if a regression of the kind mypy can inprinciple catch were to occur, it would be harder to notice it.This refactors the code, keeping the same behavior but expressingit 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#1732, revised ina0fa2bd in#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.
1 parent43b7f8a commitdc95a76

File tree

1 file changed

+57
-56
lines changed

1 file changed

+57
-56
lines changed

‎git/cmd.py

Lines changed: 57 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
importsignal
1515
fromsubprocessimportPopen,PIPE,DEVNULL
1616
importsubprocess
17+
importsys
1718
importthreading
1819
fromtextwrapimportdedent
1920

@@ -220,67 +221,67 @@ def pump_stream(
220221
finalizer(process)
221222

222223

223-
def_safer_popen_windows(
224-
command:Union[str,Sequence[Any]],
225-
*,
226-
shell:bool=False,
227-
env:Optional[Mapping[str,str]]=None,
228-
**kwargs:Any,
229-
)->Popen:
230-
"""Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
231-
232-
This avoids an untrusted search path condition where a file like ``git.exe`` in a
233-
malicious repository would be run when GitPython operates on the repository. The
234-
process using GitPython may have an untrusted repository's working tree as its
235-
current working directory. Some operations may temporarily change to that directory
236-
before running a subprocess. In addition, while by default GitPython does not run
237-
external commands with a shell, it can be made to do so, in which case the CWD of
238-
the subprocess, which GitPython usually sets to a repository working tree, can
239-
itself be searched automatically by the shell. This wrapper covers all those cases.
224+
safer_popen:Callable[...,Popen]
240225

241-
:note:
242-
This currently works by setting the :envvar:`NoDefaultCurrentDirectoryInExePath`
243-
environment variable during subprocess creation. It also takes care of passing
244-
Windows-specific process creation flags, but that is unrelated to path search.
226+
ifsys.platform=="win32":
245227

246-
:note:
247-
The current implementation contains a race condition on :attr:`os.environ`.
248-
GitPython isn't thread-safe, but a program using it on one thread should ideally
249-
be able to mutate :attr:`os.environ` on another, without unpredictable results.
250-
See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
251-
"""
252-
# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
253-
# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
254-
# https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
255-
creationflags=subprocess.CREATE_NO_WINDOW|subprocess.CREATE_NEW_PROCESS_GROUP
256-
257-
# When using a shell, the shell is the direct subprocess, so the variable must be
258-
# set in its environment, to affect its search behavior. (The "1" can be any value.)
259-
ifshell:
260-
# The original may be immutable or reused by the caller. Make changes in a copy.
261-
env= {}ifenvisNoneelsedict(env)
262-
env["NoDefaultCurrentDirectoryInExePath"]="1"
263-
264-
# When not using a shell, the current process does the search in a CreateProcessW
265-
# API call, so the variable must be set in our environment. With a shell, this is
266-
# unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
267-
# patched. If that is unpatched, then in the rare case the ComSpec environment
268-
# variable is unset, the search for the shell itself is unsafe. Setting
269-
# NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler and
270-
# protects against that. (As above, the "1" can be any value.)
271-
withpatch_env("NoDefaultCurrentDirectoryInExePath","1"):
272-
returnPopen(
273-
command,
274-
shell=shell,
275-
env=env,
276-
creationflags=creationflags,
277-
**kwargs,
278-
)
228+
def_safer_popen_windows(
229+
command:Union[str,Sequence[Any]],
230+
*,
231+
shell:bool=False,
232+
env:Optional[Mapping[str,str]]=None,
233+
**kwargs:Any,
234+
)->Popen:
235+
"""Call :class:`subprocess.Popen` on Windows but don't include a CWD in the search.
236+
237+
This avoids an untrusted search path condition where a file like ``git.exe`` in a
238+
malicious repository would be run when GitPython operates on the repository. The
239+
process using GitPython may have an untrusted repository's working tree as its
240+
current working directory. Some operations may temporarily change to that directory
241+
before running a subprocess. In addition, while by default GitPython does not run
242+
external commands with a shell, it can be made to do so, in which case the CWD of
243+
the subprocess, which GitPython usually sets to a repository working tree, can
244+
itself be searched automatically by the shell. This wrapper covers all those cases.
279245
246+
:note:
247+
This currently works by setting the :envvar:`NoDefaultCurrentDirectoryInExePath`
248+
environment variable during subprocess creation. It also takes care of passing
249+
Windows-specific process creation flags, but that is unrelated to path search.
280250
281-
safer_popen:Callable[...,Popen]
251+
:note:
252+
The current implementation contains a race condition on :attr:`os.environ`.
253+
GitPython isn't thread-safe, but a program using it on one thread should ideally
254+
be able to mutate :attr:`os.environ` on another, without unpredictable results.
255+
See comments in https://github.com/gitpython-developers/GitPython/pull/1650.
256+
"""
257+
# CREATE_NEW_PROCESS_GROUP is needed for some ways of killing it afterwards. See:
258+
# https://docs.python.org/3/library/subprocess.html#subprocess.Popen.send_signal
259+
# https://docs.python.org/3/library/subprocess.html#subprocess.CREATE_NEW_PROCESS_GROUP
260+
creationflags=subprocess.CREATE_NO_WINDOW|subprocess.CREATE_NEW_PROCESS_GROUP
261+
262+
# When using a shell, the shell is the direct subprocess, so the variable must be
263+
# set in its environment, to affect its search behavior. (The "1" can be any value.)
264+
ifshell:
265+
# The original may be immutable or reused by the caller. Make changes in a copy.
266+
env= {}ifenvisNoneelsedict(env)
267+
env["NoDefaultCurrentDirectoryInExePath"]="1"
268+
269+
# When not using a shell, the current process does the search in a CreateProcessW
270+
# API call, so the variable must be set in our environment. With a shell, this is
271+
# unnecessary, in versions where https://github.com/python/cpython/issues/101283 is
272+
# patched. If that is unpatched, then in the rare case the ComSpec environment
273+
# variable is unset, the search for the shell itself is unsafe. Setting
274+
# NoDefaultCurrentDirectoryInExePath in all cases, as is done here, is simpler and
275+
# protects against that. (As above, the "1" can be any value.)
276+
withpatch_env("NoDefaultCurrentDirectoryInExePath","1"):
277+
returnPopen(
278+
command,
279+
shell=shell,
280+
env=env,
281+
creationflags=creationflags,
282+
**kwargs,
283+
)
282284

283-
ifos.name=="nt":
284285
safer_popen=_safer_popen_windows
285286
else:
286287
safer_popen=Popen

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp