On Thu, Oct 18, 2018 at 10:57:39PM +0000, Ævar Arnfjörð Bjarmason wrote:
> Add detection for aliasing loops in cases where one of the aliases
> re-invokes git as a shell command. This catches cases like:
>
> [alias]
> foo = !git bar
> bar = !git foo
>
> Before this change running "git {foo,bar}" would create a
> forkbomb. Now using the aliasing loop detection and call history
> reporting added in 82f71d9a5a ("alias: show the call history when an
> alias is looping", 2018-09-16) and c6d75bc17a ("alias: add support for
> aliases of an alias", 2018-09-16) we'll instead report:
>
> fatal: alias loop detected: expansion of 'foo' does not terminate:
> foo <==
> bar ==>
The regular alias expansion can generally assume that there's no
conditional recursion going on, because it's expanding everything
itself. But when we involve multiple processes, things get trickier.
For instance, I could do this:
[alias]
countdown = "!f() { echo \"$@\"; test \"$1\" -gt 0 && git countdown $(($1-1)); }; f"
which works now, but not with your patch.
Now obviously that's a silly toy example, but are there real cases which
might trigger this? Some plausible ones I can think of:
- an alias which handles some special cases, then chains to itself for
the simpler one (or to another alias or script, which ends up
chaining back to the original)
- an alias that runs a git command, which then spawns a hook or other
user-controlled script, which incidentally uses that same alias
I'd guess this sort of thing is pretty rare. But I wonder if we're
crossing the line of trying to assume too much about what the user's
arbitrary code does.
A simple depth counter can limit the fork bomb, and with a high enough
depth would be unlikely to trigger a false positive. It could also
protect non-aliases more reasonably, too (e.g., if you have a 1000-deep
git process hierarchy, there's a good chance you've found an infinite
loop in git itself).
> +static void init_cmd_history(struct strbuf *env, struct string_list *cmd_list)
> +{
> + const char *old = getenv(COMMAND_HISTORY_ENVIRONMENT);
> + struct strbuf **cmd_history, **ptr;
> +
> + if (!old || !*old)
> + return;
> +
> + strbuf_addstr(env, old);
> + strbuf_rtrim(env);
> +
> + cmd_history = strbuf_split_buf(old, strlen(old), ' ', 0);
> + for (ptr = cmd_history; *ptr; ptr++) {
> + strbuf_rtrim(*ptr);
> + string_list_append(cmd_list, (*ptr)->buf);
> + }
> + strbuf_list_free(cmd_history);
Maybe string_list_split() would be a little simpler?
-Peff
