Re: [PATCH] alias: detect loops in mixed execution mode

[Jeff King <peff@peff.net>]

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