[1/1] stash: fix segmentation fault when files were added with intent

Commit Message

Bruce Perry via GitGitGadget Jan. 18, 2019, 9:50 a.m. UTC

From: Matthew Kraai <[e-mail]>

After `git add -N <file>`, the index is in a special state. A state for
which the built-in stash was not prepared, as it failed to initialize
the `rev` structure in that case before using `&rev.pending`.

Detailed explanation: If `reset_tree()` returns a non-zero value,
`stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.
If `rev` is not initialized, this causes a segmentation fault.

Prevent this by initializing `rev` before calling `reset_tree()`.

This fixes https://github.com/git-for-windows/git/issues/2006.

[jes: modified the commit message in preparation for sending this patch
to the Git mailing list.]

Signed-off-by: Matthew Kraai <[e-mail]>
Signed-off-by: Johannes Schindelin <[e-mail]>
---
 builtin/stash.c  | 3 ++-
 t/t3903-stash.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Jan. 18, 2019, 8:42 p.m. UTC | #1

"Matthew Kraai via GitGitGadget" <[e-mail]> writes:

> From: Matthew Kraai <[e-mail]>
>
> After `git add -N <file>`, the index is in a special state. A state for
> which the built-in stash was not prepared, as it failed to initialize
> the `rev` structure in that case before using `&rev.pending`.
>
> Detailed explanation: If `reset_tree()` returns a non-zero value,
> `stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.
> If `rev` is not initialized, this causes a segmentation fault.

It is a bit strange that the paragraph for "detailed explanation" is
shorter than the paragraph it attempts to clarify.

Dropping those two words "Detailed explanation:" easily fixes
awkwardness ;-).

> +test_expect_success 'stash --intent-to-add file' '
> +	git reset --hard &&
> +	echo new >file4 &&
> +	git add --intent-to-add file4 &&
> +	test_when_finished "git rm -f file4" &&
> +	test_must_fail git stash
> +'

This still must fail because an index with an I-T-A cannot be
included in a stash, but test_must_fail will make sure that the
command does not suffer an uncontrolled crash.  Good.

Johannes Schindelin Jan. 21, 2019, 3:14 p.m. UTC | #2

Hi Junio,

On Fri, 18 Jan 2019, Junio C Hamano wrote:

> "Matthew Kraai via GitGitGadget" <[e-mail]> writes:
> 
> > From: Matthew Kraai <[e-mail]>
> >
> > After `git add -N <file>`, the index is in a special state. A state for
> > which the built-in stash was not prepared, as it failed to initialize
> > the `rev` structure in that case before using `&rev.pending`.
> >
> > Detailed explanation: If `reset_tree()` returns a non-zero value,
> > `stash_working_tree()` calls `object_array_clear()` with `&rev.pending`.
> > If `rev` is not initialized, this causes a segmentation fault.
> 
> It is a bit strange that the paragraph for "detailed explanation" is
> shorter than the paragraph it attempts to clarify.
> 
> Dropping those two words "Detailed explanation:" easily fixes
> awkwardness ;-).

If you must.

For me, it is not the quantity, but the quality of the words that makes it
a detailed explanation. You see, as a mathematician, I can give you a very
condensed, super detailed, complicated proof for many a lemma which is
substantially shorter than the understandable, English explanation that
I would want to give to non-mathematicians.

Likewise, if you take a step back, and try to forget for a moment that you
are very familiar with Git's source code, you will without any doubt
*have* to admit that the detailed explanation requires a *lot* of
knowledge already, while the paragraph before that does not.

So if you find that awkward, I respectfully disagree. And if you insist on
removing those "two words" (to make the paragraph *even* shorter, which is
apparently something you took exception with), I have no tools to stop
you.

Just let it be known that it is against the wish of the author of those
lines.

> > +test_expect_success 'stash --intent-to-add file' '
> > +	git reset --hard &&
> > +	echo new >file4 &&
> > +	git add --intent-to-add file4 &&
> > +	test_when_finished "git rm -f file4" &&
> > +	test_must_fail git stash
> > +'
> 
> This still must fail because an index with an I-T-A cannot be
> included in a stash, but test_must_fail will make sure that the
> command does not suffer an uncontrolled crash.  Good.

Indeed. And Matthew even adopted your preferred strategy of combining the
demonstration of the bug with the fix. In the meantime, I have found the
totally intuitive (and equally documented) command line that I can use
when I want to see whether a given branch is buggy and when I cannot
simply `git cherry-pick <commit-demonstrating-a-bug>`:

	git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
	git checkout HEAD^ -- :^/t/

Ciao,
Johannes

Junio C Hamano Jan. 22, 2019, 6:33 p.m. UTC | #3

Johannes Schindelin <[e-mail]> writes:

> when I want to see whether a given branch is buggy and when I cannot
> simply `git cherry-pick <commit-demonstrating-a-bug>`:
>
> 	git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
> 	git checkout HEAD^ -- :^/t/

Yup.  It is easy to just apply the t/ part to grab the test update
to see breakage (which I already said when I told you to have a fix
and test protecting the future breakage of the fix in a single patch
long time ago).

Johannes Schindelin Jan. 23, 2019, 11:38 a.m. UTC | #4

Hi Junio,

On Tue, 22 Jan 2019, Junio C Hamano wrote:

> Johannes Schindelin <[e-mail]> writes:
> 
> > when I want to see whether a given branch is buggy and when I cannot
> > simply `git cherry-pick <commit-demonstrating-a-bug>`:
> >
> > 	git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
> > 	git checkout HEAD^ -- :^/t/
> 
> Yup.  It is easy to just apply the t/ part to grab the test update
> to see breakage (which I already said when I told you to have a fix
> and test protecting the future breakage of the fix in a single patch
> long time ago).

Sorry, that was not my point.

My point is that

	git cherry-pick <commit-fixing-the-bug-and-adding-a-test>
	git checkout HEAD^ -- :^/t/

is *ridiculously* less intuitive than

	git cherry-pick <commit-demonstrating-a-bug>

and I would rather you stop promoting the former over the latter. After
all, Git's purpose in life is to make things easier and quicker and less
error-prone, rathern than slower, more complicated and unintuitive.

And I am sure you agree with me on that goal, so I do not understand why
you promote that a bit more.

Ciao,
Dscho

Patch

diff --git a/builtin/stash.c b/builtin/stash.c
index 3ee8a41cda..74e6ff62b5 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -1048,6 +1048,8 @@  static int stash_working_tree(struct stash_info *info, struct pathspec ps)
 	struct strbuf diff_output = STRBUF_INIT;
 	struct index_state istate = { NULL };
 
+	init_revisions(&rev, NULL);
+
 	set_alternate_index_output(stash_index_path.buf);
 	if (reset_tree(&info->i_tree, 0, 0)) {
 		ret = -1;
@@ -1055,7 +1057,6 @@  static int stash_working_tree(struct stash_info *info, struct pathspec ps)
 	}
 	set_alternate_index_output(NULL);
 
-	init_revisions(&rev, NULL);
 	rev.prune_data = ps;
 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = add_diff_to_buf;
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index b67d7a1120..7dfa3a8038 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -287,6 +287,14 @@  test_expect_success 'stash an added file' '
 	test new = "$(cat file3)"
 '
 
+test_expect_success 'stash --intent-to-add file' '
+	git reset --hard &&
+	echo new >file4 &&
+	git add --intent-to-add file4 &&
+	test_when_finished "git rm -f file4" &&
+	test_must_fail git stash
+'
+
 test_expect_success 'stash rm then recreate' '
 	git reset --hard &&
 	git rm file &&