final git bisect step leads to: "fatal: you want to use way too much memory"

final git bisect step leads to: "fatal: you want to use way too much memory"

[Markus Trippelsdorf]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

To reproduce the issue, just run:

markus@x4 ~ % git clone git://gcc.gnu.org/git/gcc.git
markus@x4 gcc % git checkout -b gcc-6 origin/gcc-6-branch 
markus@x4 gcc % git bisect bad 23240454adf1
You need to start by "git bisect start"
Do you want me to do it for you [Y/n]? y
markus@x4 gcc % git bisect good 558525d0cf3
Bisecting: 45 revisions left to test after this (roughly 6 steps)
[c4727bc59ecad9e8879aed135624781fabae16b5]      PR c++/71372    *
cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression   is
INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS
and TREE_THIS_VOLATILE flags.  For ARRAY_REF and ARRAY_RANGE_REF, copy
over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags
to the newly built tree.
markus@x4 gcc % git bisect bad
Bisecting: 22 revisions left to test after this (roughly 5 steps)
[6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31  Richard Biener
<rguenther@suse.de>
markus@x4 gcc % git bisect bad
Bisecting: 10 revisions left to test after this (roughly 4 steps)
[909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in
get_def_bb_for_const"
markus@x4 gcc % git bisect bad
Bisecting: 5 revisions left to test after this (roughly 3 steps)
[f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/
markus@x4 gcc % git bisect bad
Bisecting: 2 revisions left to test after this (roughly 1 step)
[745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback
implementation of nonexistent_path
markus@x4 gcc % git bisect good 
Bisecting: 0 revisions left to test after this (roughly 1 step)
[a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26  Jerry DeLisle
<jvdelisle@gcc.gnu.org>
markus@x4 gcc % git bisect good
f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit
commit f216419e5c4c41df70dbe00a6ea1faea46484dc8
fatal: you want to use way too much memory
markus@x4 gcc % 

-- 
Markus

[-- Attachment #2: bisect_log --]
[-- Type: text/plain, Size: 1496 bytes --]

git bisect start
# bad: [23240454adf160862453a3a850451a304867c29b] 	Backported from mainline 	2016-06-04  Jakub Jelinek  <jakub@redhat.com>
git bisect bad 23240454adf160862453a3a850451a304867c29b
# good: [558525d0cf372a48ae12b1dc27201dd47ab7878c] Daily bump.
git bisect good 558525d0cf372a48ae12b1dc27201dd47ab7878c
# bad: [c4727bc59ecad9e8879aed135624781fabae16b5] 	PR c++/71372 	* cp-gimplify.c (cp_fold): For INDIRECT_REF, if the folded expression 	is INDIRECT_REF or MEM_REF, copy over TREE_READONLY, TREE_SIDE_EFFECTS 	and TREE_THIS_VOLATILE flags.  For ARRAY_REF and ARRAY_RANGE_REF, copy 	over TREE_READONLY, TREE_SIDE_EFFECTS and TREE_THIS_VOLATILE flags 	to the newly built tree.
git bisect bad c4727bc59ecad9e8879aed135624781fabae16b5
# bad: [6935372a7f032acccf7876e7f54ddf494e101e5e] 2016-05-31  Richard Biener  <rguenther@suse.de>
git bisect bad 6935372a7f032acccf7876e7f54ddf494e101e5e
# bad: [909ed6a89604db5baa55b62ad253a9bcd5d89c03] backport "Remove assert in get_def_bb_for_const"
git bisect bad 909ed6a89604db5baa55b62ad253a9bcd5d89c03
# bad: [f216419e5c4c41df70dbe00a6ea1faea46484dc8] gcc/
git bisect bad f216419e5c4c41df70dbe00a6ea1faea46484dc8
# good: [745d4ecd59a3430f7c7b3bf33db1083d529a018b] libstdc++/70762 fix fallback implementation of nonexistent_path
git bisect good 745d4ecd59a3430f7c7b3bf33db1083d529a018b
# good: [a64301f7ac47b9d8f4f816c5e935cda4c92716b7] 2016-05-26  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
git bisect good a64301f7ac47b9d8f4f816c5e935cda4c92716b7

Re: final git bisect step leads to: "fatal: you want to use way too much memory"

[Markus Trippelsdorf]

On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:
> To reproduce the issue, just run:

Forget to mention:

markus@x4 ~ % git --version
git version 2.9.0

git-2.8.4 is fine.


-- 
Markus

Re: final git bisect step leads to: "fatal: you want to use way too much memory"

[Markus Trippelsdorf]

On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:
> markus@x4 gcc % git bisect good
> f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit
> commit f216419e5c4c41df70dbe00a6ea1faea46484dc8
> fatal: you want to use way too much memory
> markus@x4 gcc % 

The issue started with:

commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023
Author: Junio C Hamano <gitster@pobox.com>
Date:   Tue Mar 29 16:05:39 2016 -0700

    pretty: allow tweaking tabwidth in --expand-tabs


-- 
Markus

Re: final git bisect step leads to: "fatal: you want to use way too much memory"

[Jeff King]

On Thu, Jun 16, 2016 at 03:29:52PM +0200, Markus Trippelsdorf wrote:

> On 2016.06.16 at 14:53 +0200, Markus Trippelsdorf wrote:
> > markus@x4 gcc % git bisect good
> > f216419e5c4c41df70dbe00a6ea1faea46484dc8 is the first bad commit
> > commit f216419e5c4c41df70dbe00a6ea1faea46484dc8
> > fatal: you want to use way too much memory
> > markus@x4 gcc % 
> 
> The issue started with:
> 
> commit fe37a9c586a65943e1bca327a1bbe1ca4a3d3023
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Tue Mar 29 16:05:39 2016 -0700
> 
>     pretty: allow tweaking tabwidth in --expand-tabs

Interesting. But `git show` on the commit in question (f216419e5) does
not have any problems. It looks like bisect's internal "show the commit"
code does not properly call setup_revisions() to finalize the "struct
rev_info". That leaves the expand_tabs_in_log flag as "-1", which then
ends up cast to an unsigned of 2^64 when we use it in a size
computation.

And who knows what other bugs have been lurking there over the years;
there are other flags that should be finalized by setup_revision(), too.

This patch should fix it.

diff --git a/bisect.c b/bisect.c
index 6d93edb..dc13319 100644
--- a/bisect.c
+++ b/bisect.c
@@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 	if (!opt.diffopt.output_format)
 		opt.diffopt.output_format = DIFF_FORMAT_RAW;
 
+	setup_revisions(0, NULL, &opt, NULL);
 	log_tree_commit(&opt, commit);
 }
 

Re: final git bisect step leads to: "fatal: you want to use way too much memory"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Interesting. But `git show` on the commit in question (f216419e5) does
> not have any problems. It looks like bisect's internal "show the commit"
> code does not properly call setup_revisions() to finalize the "struct
> rev_info". That leaves the expand_tabs_in_log flag as "-1", which then
> ends up cast to an unsigned of 2^64 when we use it in a size
> computation.

Yuck

> And who knows what other bugs have been lurking there over the years;
> there are other flags that should be finalized by setup_revision(), too.
>
> This patch should fix it.

Looks sensible.

> diff --git a/bisect.c b/bisect.c
> index 6d93edb..dc13319 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>  	if (!opt.diffopt.output_format)
>  		opt.diffopt.output_format = DIFF_FORMAT_RAW;
>  
> +	setup_revisions(0, NULL, &opt, NULL);
>  	log_tree_commit(&opt, commit);
>  }
>  

[PATCH] bisect: always call setup_revisions after init_revisions

[Jeff King]

The former initializes the rev_info struct to default
values, and the latter parsers any command-line arguments
and finalizes the struct.

In e22278c (bisect: display first bad commit without forking
a new process, 2009-05-28), a show_diff_tree() was added
that calls the former but not the latter. It doesn't have
any arguments to parse, but it still should do the
finalizing step.

This may have caused other minor bugs over the years, but it
became much more prominent after fe37a9c (pretty: allow
tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves
the expected tab width as "-1", rather than the true default
of "8". When we see a commit with tabs to be expanded, we
end up trying to add (size_t)-1 spaces to a strbuf, which
complains about the integer overflow.

The fix is easy: just call setup_revisions() with no
arguments.

Signed-off-by: Jeff King <peff@peff.net>
---
Same patch as earlier, now with 100% more commit message.

I didn't add a test, as it seemed weirdly specific to be checking "can
bisect show a commit with tabs in it". I.e., it's not likely to actually
regress in this specific way again.

 bisect.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/bisect.c b/bisect.c
index 6d93edb..dc13319 100644
--- a/bisect.c
+++ b/bisect.c
@@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 	if (!opt.diffopt.output_format)
 		opt.diffopt.output_format = DIFF_FORMAT_RAW;
 
+	setup_revisions(0, NULL, &opt, NULL);
 	log_tree_commit(&opt, commit);
 }
 
-- 
2.9.0.165.g4aacdc3

Re: [PATCH] bisect: always call setup_revisions after init_revisions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The former initializes the rev_info struct to default
> values, and the latter parsers any command-line arguments
> and finalizes the struct.

The former refers to init and the latter setup?

> In e22278c (bisect: display first bad commit without forking
> a new process, 2009-05-28), a show_diff_tree() was added
> that calls the former but not the latter. It doesn't have
> any arguments to parse, but it still should do the
> finalizing step.
>
> This may have caused other minor bugs over the years, but it
> became much more prominent after fe37a9c (pretty: allow
> tweaking tabwidth in --expand-tabs, 2016-03-29). That leaves
> the expected tab width as "-1", rather than the true default
> of "8". When we see a commit with tabs to be expanded, we
> end up trying to add (size_t)-1 spaces to a strbuf, which
> complains about the integer overflow.
>
> The fix is easy: just call setup_revisions() with no
> arguments.

Thanks.

I wonder if we can make it even harder to make the same mistake
again somehow.  I notice that run_diff_files() and run_diff_index()
in diff-lib.c share the ideal name for such an easy-to-use helper
and run_diff_tree(), which does not exist yet, could sit alongside
with them, but the actual implementation of the former two do not
address this issue either.  I guess that the diversity of the set of
pre-packaged options that various callers want to use are so graet
that we need a rather unpleasntly large API refactoring before we
could even contemplate doing so?

In any case, this is a strict improvement.  Let's queue it for the
first maintenance release.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Same patch as earlier, now with 100% more commit message.
>
> I didn't add a test, as it seemed weirdly specific to be checking "can
> bisect show a commit with tabs in it". I.e., it's not likely to actually
> regress in this specific way again.
>
>  bisect.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/bisect.c b/bisect.c
> index 6d93edb..dc13319 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -890,6 +890,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>  	if (!opt.diffopt.output_format)
>  		opt.diffopt.output_format = DIFF_FORMAT_RAW;
>  
> +	setup_revisions(0, NULL, &opt, NULL);
>  	log_tree_commit(&opt, commit);
>  }

Re: [PATCH] bisect: always call setup_revisions after init_revisions

[Jeff King]

On Thu, Jun 16, 2016 at 05:03:53PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The former initializes the rev_info struct to default
> > values, and the latter parsers any command-line arguments
> > and finalizes the struct.
> 
> The former refers to init and the latter setup?

Yeah, sorry, I guess I was reaching back to the subject line.

Maybe (also fixing a typo):

  init_revisions() initializes the rev_info struct to default values,
  and setup_revisions() parses any command-line arguments and finalizes
  the struct.

> I wonder if we can make it even harder to make the same mistake
> again somehow.  I notice that run_diff_files() and run_diff_index()
> in diff-lib.c share the ideal name for such an easy-to-use helper
> and run_diff_tree(), which does not exist yet, could sit alongside
> with them, but the actual implementation of the former two do not
> address this issue either.  I guess that the diversity of the set of
> pre-packaged options that various callers want to use are so graet
> that we need a rather unpleasntly large API refactoring before we
> could even contemplate doing so?
> 
> In any case, this is a strict improvement.  Let's queue it for the
> first maintenance release.

I wondered about something like the patch below, to detect such problems
consistently (and not just blow up on some corner case that isn't hit in
the test suite).

But it doesn't cover every way somebody might use a "struct rev_info",
so we'd have to sprinkle more "check" functions around. And a bunch of
stuff fails in the test suite (though it looks like it's mostly rebase
stuff, so it's probably all one or two plumbing call-sites).

I do notice some sites, like builtin/pull, use init_revisions() coupled
with diff_setup_done(). That's OK if you're just doing a diff, though
I'd argue they should use setup_revisions() to be on the safe side.

---
diff --git a/log-tree.c b/log-tree.c
index 78a5381..8303e64 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -538,6 +538,12 @@ static void show_mergetag(struct rev_info *opt, struct commit *commit)
 	for_each_mergetag(show_one_mergetag, commit, opt);
 }
 
+static void check_rev_info(struct rev_info *opt)
+{
+	if (!opt->setup_finished)
+		die("BUG: init_revisions called without setup_revisions");
+}
+
 void show_log(struct rev_info *opt)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
@@ -547,6 +553,8 @@ void show_log(struct rev_info *opt)
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
 
+	check_rev_info(opt);
+
 	opt->loginfo = NULL;
 	if (!opt->verbose_header) {
 		graph_show_commit(opt->graph);
@@ -799,6 +807,8 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 	struct commit_list *parents;
 	struct object_id *oid;
 
+	check_rev_info(opt);
+
 	if (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS))
 		return 0;
 
diff --git a/revision.c b/revision.c
index d30d1c4..2677b2e 100644
--- a/revision.c
+++ b/revision.c
@@ -2341,6 +2341,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
 
+	revs->setup_finished = 1;
+
 	return left;
 }
 
diff --git a/revision.h b/revision.h
index 9fac1a6..2dc6ecb 100644
--- a/revision.h
+++ b/revision.h
@@ -213,6 +213,8 @@ struct rev_info {
 
 	struct commit_list *previous_parents;
 	const char *break_bar;
+
+	unsigned setup_finished;
 };
 
 extern int ref_excluded(struct string_list *, const char *path);