[v2] range-diff: don't segfault with mode-only changes
diff mbox series

Message ID 20191008173843.GC74671@cat
State New
Headers show
Series
  • [v2] range-diff: don't segfault with mode-only changes
Related show

Commit Message

Thomas Gummerer Oct. 8, 2019, 5:38 p.m. UTC
In ef283b3699 ("apply: make parse_git_diff_header public", 2019-07-11)
the 'parse_git_diff_header' function was made public and useable by
callers outside of apply.c.

However it was missed that its (then) only caller, 'find_header' did
some error handling, and completing 'struct patch' appropriately.

range-diff then started using this function, and tried to handle this
appropriately itself, but fell short in some cases.  This in turn
would lead to range-diff segfaulting when there are mode-only changes
in a range.

Move the error handling and completing of the struct into the
'parse_git_diff_header' function, so other callers can take advantage
of it.  This fixes the segfault in 'git range-diff'.

Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---

Thanks Junio and Dscho for your reviews.  I decided to lift the whole
error handling behaviour from find_header into parse_git_diff_header,
instead of just filling the two names with xstrdup(def_name) if
(!old_name && !new_name && !!def_name).  I think the additional
information presented there can be useful.  For example we would have
gotten some "error: git diff header lacks filename information"
instead of a segfault for the problem described in
https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/T/#me576615d7a151cf2ed46186c482fbd88f9959914.

Dscho, I didn't re-use your test case here as I had already written
one, and think what I have is slightly nicer in that it follows what
most other range-diff tests do in using the fast-exported history.  It
also expands the test coverage slightly, as we currently don't have
any coverage of the mode-change header, but will with this test.

The downside is of course that the fast export script is harder to
understand than the test you had, at least for me, but I think the
tradeoff of having the additional test coverage, and having it similar
to the rest of the test script is worth it.  If you strongly prefer
your test though I'm not going to be unhappy to use that :)

 apply.c                | 43 +++++++++++++++++++++---------------------
 t/t3206-range-diff.sh  | 40 +++++++++++++++++++++++++++++++++++++++
 t/t3206/history.export | 31 +++++++++++++++++++++++++++++-
 3 files changed, 92 insertions(+), 22 deletions(-)

Comments

Johannes Schindelin Oct. 8, 2019, 7:44 p.m. UTC | #1
Hi Thomas,

On Tue, 8 Oct 2019, Thomas Gummerer wrote:

> In ef283b3699 ("apply: make parse_git_diff_header public", 2019-07-11)
> the 'parse_git_diff_header' function was made public and useable by
> callers outside of apply.c.
>
> However it was missed that its (then) only caller, 'find_header' did
> some error handling, and completing 'struct patch' appropriately.
>
> range-diff then started using this function, and tried to handle this
> appropriately itself, but fell short in some cases.  This in turn
> would lead to range-diff segfaulting when there are mode-only changes
> in a range.
>
> Move the error handling and completing of the struct into the
> 'parse_git_diff_header' function, so other callers can take advantage
> of it.  This fixes the segfault in 'git range-diff'.
>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Ciao,
Dscho

> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>
> Thanks Junio and Dscho for your reviews.  I decided to lift the whole
> error handling behaviour from find_header into parse_git_diff_header,
> instead of just filling the two names with xstrdup(def_name) if
> (!old_name && !new_name && !!def_name).  I think the additional
> information presented there can be useful.  For example we would have
> gotten some "error: git diff header lacks filename information"
> instead of a segfault for the problem described in
> https://public-inbox.org/git/20191002141615.GB17916@kitsune.suse.cz/T/#me576615d7a151cf2ed46186c482fbd88f9959914.
>
> Dscho, I didn't re-use your test case here as I had already written
> one, and think what I have is slightly nicer in that it follows what
> most other range-diff tests do in using the fast-exported history.  It
> also expands the test coverage slightly, as we currently don't have
> any coverage of the mode-change header, but will with this test.
>
> The downside is of course that the fast export script is harder to
> understand than the test you had, at least for me, but I think the
> tradeoff of having the additional test coverage, and having it similar
> to the rest of the test script is worth it.  If you strongly prefer
> your test though I'm not going to be unhappy to use that :)
>
>  apply.c                | 43 +++++++++++++++++++++---------------------
>  t/t3206-range-diff.sh  | 40 +++++++++++++++++++++++++++++++++++++++
>  t/t3206/history.export | 31 +++++++++++++++++++++++++++++-
>  3 files changed, 92 insertions(+), 22 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 57a61f2881..f8a046a6a5 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1361,11 +1361,32 @@ int parse_git_diff_header(struct strbuf *root,
>  			if (check_header_line(*linenr, patch))
>  				return -1;
>  			if (res > 0)
> -				return offset;
> +				goto done;
>  			break;
>  		}
>  	}
>
> +done:
> +	if (!patch->old_name && !patch->new_name) {
> +		if (!patch->def_name) {
> +			error(Q_("git diff header lacks filename information when removing "
> +				 "%d leading pathname component (line %d)",
> +				 "git diff header lacks filename information when removing "
> +				 "%d leading pathname components (line %d)",
> +				 parse_hdr_state.p_value),
> +			      parse_hdr_state.p_value, *linenr);
> +			return -128;
> +		}
> +		patch->old_name = xstrdup(patch->def_name);
> +		patch->new_name = xstrdup(patch->def_name);
> +	}
> +	if ((!patch->new_name && !patch->is_delete) ||
> +	    (!patch->old_name && !patch->is_new)) {
> +		error(_("git diff header lacks filename information "
> +			"(line %d)"), *linenr);
> +		return -128;
> +	}
> +	patch->is_toplevel_relative = 1;
>  	return offset;
>  }
>
> @@ -1546,26 +1567,6 @@ static int find_header(struct apply_state *state,
>  				return -128;
>  			if (git_hdr_len <= len)
>  				continue;
> -			if (!patch->old_name && !patch->new_name) {
> -				if (!patch->def_name) {
> -					error(Q_("git diff header lacks filename information when removing "
> -							"%d leading pathname component (line %d)",
> -							"git diff header lacks filename information when removing "
> -							"%d leading pathname components (line %d)",
> -							state->p_value),
> -						     state->p_value, state->linenr);
> -					return -128;
> -				}
> -				patch->old_name = xstrdup(patch->def_name);
> -				patch->new_name = xstrdup(patch->def_name);
> -			}
> -			if ((!patch->new_name && !patch->is_delete) ||
> -			    (!patch->old_name && !patch->is_new)) {
> -				error(_("git diff header lacks filename information "
> -					     "(line %d)"), state->linenr);
> -				return -128;
> -			}
> -			patch->is_toplevel_relative = 1;
>  			*hdrsize = git_hdr_len;
>  			return offset;
>  		}
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index ec548654ce..5b87fead2e 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -226,6 +226,46 @@ test_expect_success 'renamed file' '
>  	test_cmp expected actual
>  '
>
> +test_expect_success 'file with mode only change' '
> +	git range-diff --no-color --submodule=log topic...mode-only-change >actual &&
> +	sed s/Z/\ /g >expected <<-EOF &&
> +	1:  fccce22 ! 1:  4d39cb3 s/4/A/
> +	    @@ Metadata
> +	    ZAuthor: Thomas Rast <trast@inf.ethz.ch>
> +	    Z
> +	    Z ## Commit message ##
> +	    -    s/4/A/
> +	    +    s/4/A/ + add other-file
> +	    Z
> +	    Z ## file ##
> +	    Z@@
> +	    @@ file
> +	    Z A
> +	    Z 6
> +	    Z 7
> +	    +
> +	    + ## other-file (new) ##
> +	2:  147e64e ! 2:  26c107f s/11/B/
> +	    @@ Metadata
> +	    ZAuthor: Thomas Rast <trast@inf.ethz.ch>
> +	    Z
> +	    Z ## Commit message ##
> +	    -    s/11/B/
> +	    +    s/11/B/ + mode change other-file
> +	    Z
> +	    Z ## file ##
> +	    Z@@ file: A
> +	    @@ file: A
> +	    Z 12
> +	    Z 13
> +	    Z 14
> +	    +
> +	    + ## other-file (mode change 100644 => 100755) ##
> +	3:  a63e992 = 3:  4c1e0f5 s/12/B/
> +	EOF
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'file added and later removed' '
>  	git range-diff --no-color --submodule=log topic...added-removed >actual &&
>  	sed s/Z/\ /g >expected <<-EOF &&
> diff --git a/t/t3206/history.export b/t/t3206/history.export
> index 7bb3814962..4c808e5b3b 100644
> --- a/t/t3206/history.export
> +++ b/t/t3206/history.export
> @@ -55,7 +55,7 @@ A
>  19
>  20
>
> -commit refs/heads/topic
> +commit refs/heads/mode-only-change
>  mark :4
>  author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
>  committer Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
> @@ -678,3 +678,32 @@ s/12/B/
>  from :55
>  M 100644 :9 renamed-file
>
> +commit refs/heads/mode-only-change
> +mark :57
> +author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473767 +0100
> +data 24
> +s/4/A/ + add other-file
> +from :4
> +M 100644 :5 file
> +M 100644 :49 other-file
> +
> +commit refs/heads/mode-only-change
> +mark :58
> +author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100
> +data 33
> +s/11/B/ + mode change other-file
> +from :57
> +M 100644 :7 file
> +M 100755 :49 other-file
> +
> +commit refs/heads/mode-only-change
> +mark :59
> +author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200
> +committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100
> +data 8
> +s/12/B/
> +from :58
> +M 100644 :9 file
> +
> --
> 2.23.0.501.gb744c3af07
>
>
Uwe Kleine-König Oct. 9, 2019, 7:42 a.m. UTC | #2
On Tue, Oct 08, 2019 at 06:38:43PM +0100, Thomas Gummerer wrote:
> In ef283b3699 ("apply: make parse_git_diff_header public", 2019-07-11)
> the 'parse_git_diff_header' function was made public and useable by
> callers outside of apply.c.
> 
> However it was missed that its (then) only caller, 'find_header' did
> some error handling, and completing 'struct patch' appropriately.
> 
> range-diff then started using this function, and tried to handle this
> appropriately itself, but fell short in some cases.  This in turn
> would lead to range-diff segfaulting when there are mode-only changes
> in a range.
> 
> Move the error handling and completing of the struct into the
> 'parse_git_diff_header' function, so other callers can take advantage
> of it.  This fixes the segfault in 'git range-diff'.
> 
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>

This patch also makes git work again for the originally problematic
usecase.

Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for your quick reaction to my bug report,
Uwe Kleine-König

Patch
diff mbox series

diff --git a/apply.c b/apply.c
index 57a61f2881..f8a046a6a5 100644
--- a/apply.c
+++ b/apply.c
@@ -1361,11 +1361,32 @@  int parse_git_diff_header(struct strbuf *root,
 			if (check_header_line(*linenr, patch))
 				return -1;
 			if (res > 0)
-				return offset;
+				goto done;
 			break;
 		}
 	}
 
+done:
+	if (!patch->old_name && !patch->new_name) {
+		if (!patch->def_name) {
+			error(Q_("git diff header lacks filename information when removing "
+				 "%d leading pathname component (line %d)",
+				 "git diff header lacks filename information when removing "
+				 "%d leading pathname components (line %d)",
+				 parse_hdr_state.p_value),
+			      parse_hdr_state.p_value, *linenr);
+			return -128;
+		}
+		patch->old_name = xstrdup(patch->def_name);
+		patch->new_name = xstrdup(patch->def_name);
+	}
+	if ((!patch->new_name && !patch->is_delete) ||
+	    (!patch->old_name && !patch->is_new)) {
+		error(_("git diff header lacks filename information "
+			"(line %d)"), *linenr);
+		return -128;
+	}
+	patch->is_toplevel_relative = 1;
 	return offset;
 }
 
@@ -1546,26 +1567,6 @@  static int find_header(struct apply_state *state,
 				return -128;
 			if (git_hdr_len <= len)
 				continue;
-			if (!patch->old_name && !patch->new_name) {
-				if (!patch->def_name) {
-					error(Q_("git diff header lacks filename information when removing "
-							"%d leading pathname component (line %d)",
-							"git diff header lacks filename information when removing "
-							"%d leading pathname components (line %d)",
-							state->p_value),
-						     state->p_value, state->linenr);
-					return -128;
-				}
-				patch->old_name = xstrdup(patch->def_name);
-				patch->new_name = xstrdup(patch->def_name);
-			}
-			if ((!patch->new_name && !patch->is_delete) ||
-			    (!patch->old_name && !patch->is_new)) {
-				error(_("git diff header lacks filename information "
-					     "(line %d)"), state->linenr);
-				return -128;
-			}
-			patch->is_toplevel_relative = 1;
 			*hdrsize = git_hdr_len;
 			return offset;
 		}
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ec548654ce..5b87fead2e 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -226,6 +226,46 @@  test_expect_success 'renamed file' '
 	test_cmp expected actual
 '
 
+test_expect_success 'file with mode only change' '
+	git range-diff --no-color --submodule=log topic...mode-only-change >actual &&
+	sed s/Z/\ /g >expected <<-EOF &&
+	1:  fccce22 ! 1:  4d39cb3 s/4/A/
+	    @@ Metadata
+	    ZAuthor: Thomas Rast <trast@inf.ethz.ch>
+	    Z
+	    Z ## Commit message ##
+	    -    s/4/A/
+	    +    s/4/A/ + add other-file
+	    Z
+	    Z ## file ##
+	    Z@@
+	    @@ file
+	    Z A
+	    Z 6
+	    Z 7
+	    +
+	    + ## other-file (new) ##
+	2:  147e64e ! 2:  26c107f s/11/B/
+	    @@ Metadata
+	    ZAuthor: Thomas Rast <trast@inf.ethz.ch>
+	    Z
+	    Z ## Commit message ##
+	    -    s/11/B/
+	    +    s/11/B/ + mode change other-file
+	    Z
+	    Z ## file ##
+	    Z@@ file: A
+	    @@ file: A
+	    Z 12
+	    Z 13
+	    Z 14
+	    +
+	    + ## other-file (mode change 100644 => 100755) ##
+	3:  a63e992 = 3:  4c1e0f5 s/12/B/
+	EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'file added and later removed' '
 	git range-diff --no-color --submodule=log topic...added-removed >actual &&
 	sed s/Z/\ /g >expected <<-EOF &&
diff --git a/t/t3206/history.export b/t/t3206/history.export
index 7bb3814962..4c808e5b3b 100644
--- a/t/t3206/history.export
+++ b/t/t3206/history.export
@@ -55,7 +55,7 @@  A
 19
 20
 
-commit refs/heads/topic
+commit refs/heads/mode-only-change
 mark :4
 author Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
 committer Thomas Rast <trast@inf.ethz.ch> 1374485014 +0200
@@ -678,3 +678,32 @@  s/12/B/
 from :55
 M 100644 :9 renamed-file
 
+commit refs/heads/mode-only-change
+mark :57
+author Thomas Rast <trast@inf.ethz.ch> 1374485024 +0200
+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473767 +0100
+data 24
+s/4/A/ + add other-file
+from :4
+M 100644 :5 file
+M 100644 :49 other-file
+
+commit refs/heads/mode-only-change
+mark :58
+author Thomas Rast <trast@inf.ethz.ch> 1374485036 +0200
+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100
+data 33
+s/11/B/ + mode change other-file
+from :57
+M 100644 :7 file
+M 100755 :49 other-file
+
+commit refs/heads/mode-only-change
+mark :59
+author Thomas Rast <trast@inf.ethz.ch> 1374485044 +0200
+committer Thomas Gummerer <t.gummerer@gmail.com> 1570473768 +0100
+data 8
+s/12/B/
+from :58
+M 100644 :9 file
+