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

[Jeff King <peff@peff.net>]

On Sat, Oct 20, 2018 at 01:14:28PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > 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 agree it's probably quite rare, if it exists at all. But I also wonder
how important looping alias protection is. It's also rare, and the
outcome is usually "gee, I wonder why this is taking so long? ^C".

At least that's my instinct. I don't remember having run into this at
all myself (though certainly I have written my fair share of infinite
loops in other systems, like bash aliases, and that is what happened).

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

Would we print a long error message? I'd assume that we'd just recurse
for longer and print one error message that says:

  fatal: woah, you're 1000-levels deep in Git commands!

That doesn't help the user find the recursion, but re-running with
GIT_TRACE=1 would make it pretty clear, I'd think.

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

Right, I'm not too happy about that annoyance. But it seems clear that I
think the loop protection is way less important than you do, so I'm
willing to sacrifice (or more accurately, risk the possibility of
sacrificing) a lot less for it. :)

I dunno. I doubt it is likely to help or hinder that many people either
way.

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

I cheated before writing that and confirmed that it does seem to work. ;)

Here's the patch in case it is useful. IMHO we should be trying to get
rid of strbuf_split, because it's a pretty crappy interface.

diff --git a/git.c b/git.c
index cba242836c..9d1b66a1fa 100644
--- a/git.c
+++ b/git.c
@@ -675,7 +675,6 @@ static void execv_dashed_external(const char **argv)
 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;
@@ -683,12 +682,7 @@ static void init_cmd_history(struct strbuf *env, struct string_list *cmd_list)
 	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);
+	string_list_split(cmd_list, env->buf, ' ', -1);
 }
 
 static void add_cmd_history(struct strbuf *env, struct string_list *cmd_list,

-Peff