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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Oct 19 2018, Jeff King wrote:

> 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).

I don't think this edge case you're describing is very plausible, and I
doubt it exists in the wild.

But going by my personal incredulity and a git release breaking code in
the wild would suck, so agree that I need to re-roll this to anticipate
that.

I don't have time to write it now, but what do you think about a version
of this where we introduce a core.recursionLimit setting, and by default
set it to "1" (for one recursion), so by default die just as we do now,
but with some advice() saying that we've bailed out early because this
looks crazy, but you can set it to e.g. "1000" if you think you know
what you're doing, or "0" for no limit.

The reason I'd like to do that is because I think it's *way* more common
to do this accidentally than intentionally, and by having a default
limit of 1000 we'd print a really long error message, or alternatively
would have to get into the mess of de-duplicating the callstack as we
print the error.

It also has the advantage that if people in the wild really use this
they'll chime in about this new annoying core.recursionLimit=1 setting,
at the cost of me having annoyed them all by breaking their working
code.

>> +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?

Yeah looks like it. I cargo-culted this from elsewhere without looking
at that API. I'll look into it.