git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Tim Schumacher <timschumi@gmx.de>, Duy Nguyen <pclouds@gmail.com>
Subject: Re: [PATCH] alias: detect loops in mixed execution mode
Date: Sat, 20 Oct 2018 21:18:21 +0200	[thread overview]
Message-ID: <87efckctqa.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20181020185852.GA6234@sigill.intra.peff.net>


On Sat, Oct 20 2018, Jeff King wrote:

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

Yeah the reason I'd like the core.recursionLimit=1 setting by default is
so that we can also print the same pretty and easy to grok error message
we do now for non-! aliases by default without spewing out ~3-4k lines
of mostly duplicate output (with a default limit of 1000).

We didn't support chained aliases at all before, so I think the odds
that people will run into this now will increase as they add "!" to
existing aliases, and I'd like to have git's UI friendly enough to tell
users what went wrong by default, and not have to resort to the likes of
GIT_TRACE=1 which really should be left to powerusers.

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

Thanks! Will squash it.

  reply	other threads:[~2018-10-20 19:18 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-05  8:54 [RFC PATCH v2] Allow aliases that include other aliases Tim Schumacher
2018-09-05 15:48 ` Duy Nguyen
2018-09-05 19:02   ` Tim Schumacher
2018-09-05 17:12 ` Junio C Hamano
2018-09-05 19:12   ` Tim Schumacher
2018-09-05 17:34 ` Jeff King
2018-09-05 20:02   ` Tim Schumacher
2018-09-06 13:38     ` Ævar Arnfjörð Bjarmason
2018-09-06 14:17     ` Ævar Arnfjörð Bjarmason
2018-10-18 22:57       ` [PATCH] alias: detect loops in mixed execution mode Ævar Arnfjörð Bjarmason
2018-10-19  8:28         ` Ævar Arnfjörð Bjarmason
2018-10-19 22:09           ` Jeff King
2018-10-20 10:52             ` Ævar Arnfjörð Bjarmason
2018-10-19 22:07         ` Jeff King
2018-10-20 11:14           ` Ævar Arnfjörð Bjarmason
2018-10-20 18:58             ` Jeff King
2018-10-20 19:18               ` Ævar Arnfjörð Bjarmason [this message]
2018-10-22 21:15                 ` Jeff King
2018-10-22 21:28                   ` Ævar Arnfjörð Bjarmason
2018-10-22  1:23               ` Junio C Hamano
2018-10-26  8:39               ` Jeff King
2018-10-26 12:44                 ` Ævar Arnfjörð Bjarmason
2018-10-29  3:44                 ` Junio C Hamano
2018-10-29 14:17                   ` Jeff King
2018-09-05 21:51   ` [RFC PATCH v2] Allow aliases that include other aliases Junio C Hamano
2018-09-06 10:16 ` [PATCH v3] " Tim Schumacher
2018-09-06 14:01   ` Ævar Arnfjörð Bjarmason
2018-09-06 14:57     ` Jeff King
2018-09-06 15:10       ` Ævar Arnfjörð Bjarmason
2018-09-06 16:18         ` Jeff King
2018-09-06 19:05       ` Tim Schumacher
2018-09-06 19:17         ` Jeff King
2018-09-06 14:59   ` Jeff King
2018-09-06 18:40     ` Junio C Hamano
2018-09-06 19:05       ` Jeff King
2018-09-06 19:31       ` Tim Schumacher
2018-09-07 22:44 ` [RFC PATCH v4 1/3] Add support for nested aliases Tim Schumacher
2018-09-07 22:44   ` [RFC PATCH v4 2/3] Show the call history when an alias is looping Tim Schumacher
2018-09-08 13:34     ` Duy Nguyen
2018-09-08 16:29       ` Jeff King
2018-09-07 22:44   ` [RFC PATCH v4 3/3] t0014: Introduce alias testing suite Tim Schumacher
2018-09-07 23:38     ` Eric Sunshine
2018-09-14 23:12       ` Tim Schumacher
2018-09-16  7:21         ` Eric Sunshine
2018-09-08 13:28   ` [RFC PATCH v4 1/3] Add support for nested aliases Duy Nguyen
2018-09-16  7:46     ` Tim Schumacher
2018-09-17 15:37       ` Junio C Hamano
2018-09-21 12:45         ` Tim Schumacher
2018-09-21 15:59           ` Junio C Hamano
2018-09-16  7:50   ` [PATCH v5 " Tim Schumacher
2018-09-16  7:50     ` [PATCH v5 2/3] Show the call history when an alias is looping Tim Schumacher
2018-09-16  7:50     ` [PATCH v5 3/3] t0014: Introduce an alias testing suite Tim Schumacher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87efckctqa.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=peff@peff.net \
    --cc=timschumi@gmx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).