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
