René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Git.execute's kill_after_timeout callback assumes procps","articleBody":"### Background\r\n\r\nCalling `Git.execute`—whether directly, or indirectly by calling the dynamic attributes of a `Git` instance—and passing `kill_after_timeout` with a non-`None` value, creates a timer on a separate thread that calls the local [`kill_process`](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L1008-L1036) function. This callback function uses `os.kill` to kill the process. Before killing the process, it enumerates the process's *direct children*. If sending the first signal succeeds (basically, if the parent process still existed), it also attempts to kill the child processes.\r\n\r\nThe children are enumerated with `ps --ppid`:\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L1010-L1014\r\n\r\n### The problem\r\n\r\nThe `--ppid` option is [not POSIX](https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/ps.html). Most GNU/Linux systems have [procps](https://gitlab.com/procps-ng/procps), whose `ps` implementation supports `--ppid`. I am unsure if *any* other implementations of `ps` support it. The procps tools generally run only on Linux-based systems, because they use the `/proc` filesystem (and assume it is laid out as in Linux). Although they can run on any such system, Alpine Linux and some minimal GNU/Linux environments do not ship them, defaulting to `ps` from [busybox](https://busybox.net/) instead.\r\n\r\nAs demonstrated below, [macOS](https://ss64.com/osx/ps.html) `ps` does not support `--ppid`. Nor do [FreeBSD](https://man.freebsd.org/cgi/man.cgi?query=ps\u0026apropos=0\u0026sektion=0\u0026manpath=FreeBSD+14.0-RELEASE+and+Ports\u0026arch=default\u0026format=html), [NetBSD](https://man.netbsd.org/ps.1), [OpenBSD](https://man.openbsd.org/ps), or [DragonFly](https://leaf.dragonflybsd.org/cgi/web-man?command=ps\u0026section=ANY). [AIX](https://www.ibm.com/docs/en/aix/7.3?topic=p-ps-command) does not have `--ppid`. [illumos](https://illumos.org/man/1/ps) does not have `--ppid`; nor does [Solaris](https://docs.oracle.com/cd/E88353_01/html/E37839/ps-1.html), though `-ppid` (with one `-`) can be used in 11.4.27 or higher. Although Cygwin mimics Linux where feasible, its `/proc` filesystem is different, and its `ps` does not support `--ppid` either (nor even some important POSIX options like `-o`).\r\n\r\nThe callback parses stdout from that `ps` command, but does not examine the exit status or stderr. The effect is that an error message on a system without procps (or another `ps` supporting `--ppid`, if there is one) is printed, and the parent process is still sent `SIGKILL`, but its children are never found or sent signals.\r\n\r\n*As detailed below, although the `fetch`, `pull`, and `push` methods of the `Remote` class accept a `kill_after_timeout` argument, they do not use `Git.execute`, so they are unaffected by this bug.*\r\n\r\n### Steps to reproduce\r\n\r\nOn macOS 13 (on a GitHub Actions CI runner with [tmate](https://github.com/marketplace/actions/debugging-with-tmate)), I created this script in a directory in `$PATH`, named it `git-sleep`, and marked it executable:\r\n\r\n```sh\r\n#!/bin/sh\r\nsleep \"$@\"\r\n```\r\n\r\nThen I called `sleep` on a `Git` instance with a `kill_after_timeout` argument specifying a shorter duration than the sleep:\r\n\r\n```text\r\nbash-3.2$ python\r\nPython 3.12.0 (v3.12.0:0fb18b02c8, Oct  2 2023, 09:45:56) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin\r\nType \"help\", \"copyright\", \"credits\" or \"license\" for more information.\r\n\u003e\u003e\u003e from git import Git\r\n\u003e\u003e\u003e Git().sleep(10, kill_after_timeout=5)\r\nps: illegal option -- -\r\nusage: ps [-AaCcEefhjlMmrSTvwXx] [-O fmt | -o fmt] [-G gid[,gid...]]\r\n          [-g grp[,grp...]] [-u [uid,uid...]]\r\n          [-p pid[,pid...]] [-t tty[,tty...]] [-U user[,user...]]\r\n       ps [-L]\r\nTraceback (most recent call last):\r\n  File \"\u003cstdin\u003e\", line 1, in \u003cmodule\u003e\r\n  File \"/Users/runner/work/GitPython/GitPython/git/cmd.py\", line 741, in \u003clambda\u003e\r\n    return lambda *args, **kwargs: self._call_process(name, *args, **kwargs)\r\n                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\r\n  File \"/Users/runner/work/GitPython/GitPython/git/cmd.py\", line 1320, in _call_process\r\n    return self.execute(call, **exec_kwargs)\r\n           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\r\n  File \"/Users/runner/work/GitPython/GitPython/git/cmd.py\", line 1117, in execute\r\n    raise GitCommandError(redacted_command, status, stderr_value, stdout_value)\r\ngit.exc.GitCommandError: Cmd('git') failed due to: exit code(-9)\r\n  cmdline: git sleep 10\r\n  stderr: 'Timeout: the command \"git sleep 10\" did not complete in 5 secs.'\r\n\u003e\u003e\u003e\r\n```\r\n\r\n### Impact\r\n\r\n#### 1. The *other* `kill_after_timeout` is unaffected\r\n\r\nThere are two callables defined in `git/cmd.py` that accept an optional `kill_after_timeout` argument: the \"internal\" top-level [`handle_process_output`](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L96-L108) function that is not listed in `__all__` but is used throughout GitPython, and the public [`Git.execute`](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L820-L837) method (also used when dynamic `Git` methods are called). The meaning of this argument is subtly different, and the associated implementations completely different.\r\n\r\n**This bug affects only the one in the `Git` class.** Thus it does not affect common uses of timeouts in interacting with remotes: the [`Remote.fetch`](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/remote.py#L956-L965), [`Remote.push`](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/remote.py#L1073-L1081), and [`Remote.pull`](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/remote.py#L1032-L1040) methods accept `kill_after_timeout` arguments, but they forward them to `handle_process_output`.\r\n\r\n#### 2. But this one *should* work on all Unix-like systems\r\n\r\nFrom context, I think it is unintended not to support common Unix-like systems such as macOS. The [`Git.execute` docstring](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L878-L886) says \"This feature is not supported on Windows\" and makes no other claims about compatibility, from which I think readers will reasonably infer that other platforms are believed supported. When called on a native Windows system (not Cygwin) with a non-`None` value for `kill_after_timeout`, it raises a `GitCommandError`. Other systems, including Cygwin, raise no exception and register the `kill_process` callback. `kill_after_timeout` is thus *in effect* documented to work on all systems except native Windows.\r\n\r\n#### 3. What happens if the child processes aren't sent `SIGKILL`?\r\n\r\nI don't know how much of a problem it is for `SIGKILL` to be sent only to the parent and not to its direct children. I am not confident I know why that is being done, as opposed to killing only the parent process, or attempting to kill its entire process tree. My *guess* is that this is because many `git` commands use a subprocess to do their work. If so, then it may in practice be important—in situations where people pass `kill_after_timeout`—that the child processes are killed as well.\r\n\r\nHowever, `git` subprocesses do sometimes use their own subprocesses:\r\n\r\n```text\r\nek@Glub:~$ pstree -a\r\ninit(Ubuntu)\r\n  ├─SessionLeader\r\n  │   └─Relay(9)\r\n  │       ├─bash\r\n  │       │   └─git clone https://github.com/huggingface/transformers.git\r\n  │       │       └─git remote-https origin https://github.com/huggingface/transformers.git\r\n  │       │           └─git-remote-http origin https://github.com/huggingface/transformers.git\r\n...\r\n```\r\n\r\nIn that example, the `git-remote-http` process may not receive `SIGKILL`. I am unsure how much this matters, but if it is a problem, then the more severe it is, the *less* severe this bug is, because the intended behavior wouldn't help anyway. Likewise, in situations where killing the parent process is sufficient, this bug also does not cause a problem.\r\n\r\nThat lower descendants are not killed has been reported as #895. That was observed in GitPython 2.0.2, which had the current approach of [killing just the direct child processes](https://github.com/gitpython-developers/GitPython/blob/2.0.2/git/cmd.py#L635-L639).\r\n\r\n### A minor race condition…\r\n\r\nOne thing I'm a little worried about is a race condition that is currently present, and that I think may not be possible to fix, *but that I worry finding child processes in a more portable way may exacerbate*. Unless it can be solved or mitigated more deeply, it is a reason, unrelated to performance, to prefer that a portable substitute for the existing use of `ps --ppid` *not be too much slower* than the current way. (I likewise worry that if the approach were changed to kill all descendants, then the added time to traverse the whole subtree might exacerbate this race condition.)\r\n\r\nSuppose we plan to kill a process P and all its direct child processes including Q, and we find the PID of Q, but before killing Q, all the following happen:\r\n\r\n1. Q dies.\r\n2. Q is reaped. That is, it is [wait(2)](https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/wait.html)ed by its parent--which is either its original parent P or, if P has died, then `init`--causing its entry in the process table to be removed and its PID to be available for use by a future process.\r\n3. A new process, R, is created and assigned Q's old PID.\r\n\r\nThen when when we try to kill Q, we kill R.\r\n\r\n**This situation is rare**, because in practice the time between when a process is reaped and when a new process is given its PID is only short [when the process table is nearly full](https://unix.stackexchange.com/a/414974/11938) so the kernel has no less recently relinquished PIDs to give out. But I think it would be best to avoid increasing the risk of it.\r\n\r\nThere may be other related race conditions, but this is the one that seems it could be worsened by replacing the existing unportable use of `ps --ppid` with some other technique, if that other technique is markedly slower.\r\n\r\n### Finding/killing the the subprocesses portably\r\n\r\nI am unsure if this should be done, because it is not clear to me that killing the parent process and its direct child processes, as is currently attempted (generally successfully on GNU/Linux and unsuccessfully elsewhere), is necessarily what should happen. Doing anything else might risk incompatibility for some existing use cases on some systems, so I would want to be cautious about doing something altogether different, but I think it should still be considered before proceeding.\r\n\r\nHowever, assuming the current approach of killing the child processes should be preserved, I think there are three cases:\r\n\r\n1. Systems with `pgrep`/`pkill`.\r\n2. Systems whose `ps` is POSIX-compliant, or at least supports `-A` and `-o`.\r\n3. Cygwin. (And the like, e.g., MSYS2. But `sys.platform == \"cygwin\"` still covers that.)\r\n\r\nCase 1 could be folded into case 2 if a speed regression is acceptable (but see above on the race condition), or if testing reveals using `pgrep` or `pkill` is not significantly faster. Case 3 could be dropped in lieu of modifying the docstring to document that `kill_after_timeout` is less effective on Cygwin, if reducing complexity is regarded as more important than covering it.\r\n\r\nWhether to cover case 3 or not is more a matter of code complexity than time to write and review the code. With or without it, I think most of the time and effort would be on the *tests*. Currently none cover passing `kill_after_timeout` to `Git.execute` or to a dynamic method of a `Git` object. Only the *other* `kill_after_timeout`—of `handle_process_output`—has test coverage. Because this project has CI on Cygwin, I don't think the tests have to do much to accommodate it—its challenges are ready-made.\r\n\r\n(An alternative to dealing with these details is to use [psutil](https://pypi.org/project/psutil/), but I'm unsure if the impact of this issue is sufficient to justify adding it as a dependency. It doesn't support all systems, but systems it doesn't support are rare. I think it could be made conditional on the systems it is installable on, and the features that use it be documented as unavailable on other systems. I think this is probably not worth doing just for this, but if it turns out it would help in various other places, and increasing rather than decreasing OS compatibility—as it would here—then it might make sense to consider it. On the other hand, one benefit of GitPython is that it has very few dependencies.)\r\n\r\n#### 1. If we have `pgrep`/`pkill`\r\n\r\n`pgrep` and `pkill` are not POSIX, but they are available on many more systems than `ps --ppid`. Furthermore, it is likely that all systems that support `ps --ppid` also have `pgrep` and `pkill`, because not only are they very common, but procps (which provides the only `ps` with `--ppid` I can find, as discussed above) [includes an implementation](https://gitlab.com/procps-ng/procps/-/blob/master/src/pgrep.c?ref_type=heads) of them. Of course, it's possible (odd, but possible) for a distribution to use procps for `ps` but not include `pgrep` and `pkill`. Whether `pkill` can be used to consolidate the steps, or `pgrep` must be used together with something what is already there, is a design decision that should be influenced by a decision about the best order for sending `SIGKILL`.\r\n\r\nIf it's acceptable to send `SIGKILL` to the child processes first, then instead of running `[\"ps\", \"--ppid\", str(pid)]` *and most of what comes after it*, one option is to run `[\"pkill\", \"-P\", str(pid)]` and then:\r\n\r\n- If it succeeds, immediately call `os.kill(pid, signal.SIGKILL)` and `kill_check.set()`.\r\n- If it fails due to `pkill` not existing—or if it was checked first and found absent—proceed to case 2 (using `ps`).\r\n\r\nIf it's not acceptable to send `SIGKILL` to the child processes first, or if either order is acceptable but it is desirable to share more code with the fallback case 2, then instead of running `[\"ps\", \"--ppid\", str(pid)]`, run `[\"pgrep\", \"-P\", str(pid)]`, then:\r\n\r\n- If it failed due to `pgrep` not existing—or if it was checked first and found absent—proceed to case 2.\r\n- Treat an exit status of 0 or 1 as success; 1 is when there were no children. (This seems to hold across different implementations, but I'll want to look into it further, since these tools are not POSIX or [XSI](https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/V1_chap02.html#tag_02_01_04).)\r\n- Parse the output—each line is just a PID, with no headers, no other columns—into `child_pids`.\r\n- Continue with the rest of the `kill_process` function as it already exists.\r\n\r\n#### 2. If `ps` supports `-A` and `-o`\r\n\r\nThis is almost every Unix-like system used today; [POSIX requires these options](https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/ps.html).\r\n\r\nInstead of running `[\"ps\", \"--ppid\", str(pid)]`, run `[\"ps\", \"-A\", \"-o\", \"pid,ppid\"]`, then:\r\n\r\n- Check that the first row is `PID` and `PPID` to safeguard against unexpectedly nonstandard `ps`.\r\n- Each remaining row should have child and parent process IDs. Filter for the rows where the parent (second column) is what we passed, and populate `child_pids` with the PIDs from the first column.\r\n- Continue with the rest of the `kill_process` function as it already exists.\r\n\r\nIf using this is as fast a `pkill`/`pgrep`, or slower but not by a lot, or code simplicity is considered more important than the small worsening of the rare race condition, then this could be used on all systems except Cygwin. The truth is that it is only out of fear of worsening things in weird situations on GNU/Linux systems with procps that I have even proposed case 1. This is the portable way to do it (except Cygwin).\r\n\r\nIt may be possible to optimize this with `-U` to filter the real user ID to `os.getuid()`, or `-u` to filter the effective user ID to `os.geteuid()`, though `-u` seems to be an XSI extension. I don't know if this would actually make things faster. I don't know if the added complexity, though modest, would be worthwhile even if it does. When doing this, `-A` would not also be passed.\r\n\r\nThe reason not to simply omit `-A` without replacing it, which gets processes that share the caller's EUID, is that it also only shows processes with the same controlling terminal. The reason not to use `-a` instead is that it doesn't show processes not associated with any terminal. The reason I prefer `-A` to its synonym `-e` is that `-e` seems to be an XSI extension.\r\n\r\n#### 3. Cygwin\r\n\r\nRunning `ps` on Cygwin gives output that looks like:\r\n\r\n```text\r\n      PID    PPID    PGID     WINPID   TTY         UID    STIME COMMAND\r\n     1641       1    1641      27868  ?         197609   Nov 14 /usr/bin/ssh-agent\r\n     2201       1    2201      27112  ?         197609 02:35:26 /usr/bin/mintty\r\n     2202    2201    2202      41336  pty0      197609 02:35:26 /usr/bin/bash\r\n     2304    2202    2304      47276  pty0      197609 14:47:12 /usr/bin/ps\r\n```\r\n\r\nThis can be modified by various options, but `-o` is not supported. (`-A` is not supported either, but it is not needed.)\r\n\r\nInstead of running `[\"ps\", \"--ppid\", str(pid)]`, we can run `[\"ps\"]`, then:\r\n\r\n- Check the first row headers, or at least the leading `PID` and `PPID` headers that we are going to use, to safeguard against unexpectedly non-Cygwin `ps` or future changes to Cygwin `ps`.\r\n- Continue as in case 2 after the check, making sure to use only the first two fields.\r\n\r\n### Perspective\r\n\r\nI think the *what* is more important than the *how*, because:\r\n\r\n#### Test coverage\r\n\r\n[Unlike](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/test/test_remote.py#L1006-L1019) the other `kill_after_timeout` (in `handle_process_output`), the code path where `Git.execute` is passed `kill_after_timeout` has no test coverage. It would be good to test it even if this bug is not fixed. But at the root of both is figuring out if killing the parent process and its direct child processes are what is wanted.\r\n\r\n#### Maintainability\r\n\r\nThe `kill_process` callback is never called on (native) Windows, where calling `Git.execute` with a non-`None` value for `kill_after_timeout` [raises](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L960-L964) `GitCommandError`. But it contains what seem to be the remains of an attempt to support Windows: it [passes](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L1013) `PROC_CREATIONFLAGS` (which is 0 [except](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L231-L236) on Windows) when running `ps`, and it [falls back](https://github.com/gitpython-developers/GitPython/blob/fe082ad5e297119a3073dab74fa604eaf547ecc7/git/cmd.py#L1023-L1024) to `signal.SIGTERM` when `signal.SIGKILL` is absent (which it is on Windows).\r\n\r\nI discovered this whole issue because I want to remove that code, which I think could lead to future bugs, and I was looking into whether there is any reason not to. A possible reason not to is if `kill_process` can be easily modified to support Windows—which it could, if it is acceptable to kill either only the parent process, or the whole process tree, though whether it *should* is another question. Because figuring out what to do about this issue entails figuring that out too, it would open `kill_process` up to that improvement—dropping its vestigial Windows code if it is not going to support Windows—and possibly others.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-12-03T23:04:39.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":10},"url":"https://github.com/1756/GitPython/issues/1756"}