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