Regression in v2.23

Regression in v2.23

[Uwe Kleine-König]

Hello,

With git 2.23.0 I have:

	uwe@taurus:~/tmp/rangediff-segfault$ git init
	Initialized empty Git repository in /home/uwe/tmp/rangediff-segfault/.git/
	uwe@taurus:~/tmp/rangediff-segfault$ echo root > root
	uwe@taurus:~/tmp/rangediff-segfault$ git add root
	uwe@taurus:~/tmp/rangediff-segfault$ git commit -m root
	[master (root-commit) b0feddb2dee8] root
	 1 file changed, 1 insertion(+)
	 create mode 100644 root
	uwe@taurus:~/tmp/rangediff-segfault$ echo content > file
	uwe@taurus:~/tmp/rangediff-segfault$ chmod +x file
	uwe@taurus:~/tmp/rangediff-segfault$ git add file
	uwe@taurus:~/tmp/rangediff-segfault$ git commit -m file
	[master 45b547c57acd] file
	 1 file changed, 1 insertion(+)
	 create mode 100755 file
	uwe@taurus:~/tmp/rangediff-segfault$ chmod -x file
	uwe@taurus:~/tmp/rangediff-segfault$ git add file
	uwe@taurus:~/tmp/rangediff-segfault$ git commit -m 'chmod -x'
	[master eaa5d3b98caa] chmod -x
	 1 file changed, 0 insertions(+), 0 deletions(-)
	 mode change 100755 => 100644 file
	uwe@taurus:~/tmp/rangediff-segfault$ git range-diff @~2..@~ @~2..
	Segmentation fault (core dumped)

Bisecting points to b66885a30cb84fc61986bc4eea805a31fdbea79a, current master
(b744c3af07a15aaeb1b82fab689995fd5528f120) segfaults in the same way.

This is somehow similar to
https://public-inbox.org/git/20190923101929.GA18205@kitsune.suse.cz/ but
the patch by Johannes Schindelin sent in
https://public-inbox.org/git/pull.373.git.gitgitgadget@gmail.com/
doesn't help me.

For me the segfault also happens in

	strbuf_addstr(&buf, patch.new_name);

with patch.new_name being NULL.

The matching backtrace and patch object looks as follows:

	(gdb) bt
	#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
	#1  0x0000555cc448949c in strbuf_addstr (s=<optimized out>, sb=0x7ffcd1d9ef00)
	    at strbuf.h:292
	#2  read_patches (range=range@entry=0x555cc5dc2b70 "@~2..", 
	    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126
	#3  0x0000555cc44898a8 in show_range_diff (range1=0x555cc5dc2b50 "@~2..@~", 
	    range2=0x555cc5dc2b70 "@~2..", creation_factor=60, dual_color=1, 
	    diffopt=diffopt@entry=0x7ffcd1d9f680) at range-diff.c:507
	#4  0x0000555cc4397aa6 in cmd_range_diff (argc=<optimized out>, 
	    argv=<optimized out>, prefix=<optimized out>) at builtin/range-diff.c:80
	#5  0x0000555cc4328494 in run_builtin (argv=<optimized out>, 
	    argc=<optimized out>, p=<optimized out>) at git.c:445
	#6  handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:674
	#7  0x0000555cc4329554 in run_argv (argv=0x7ffcd1d9f9e0, argcp=0x7ffcd1d9f9ec)
	    at git.c:741
	#8  cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:872
	#9  0x0000555cc432803a in main (argc=4, argv=0x7ffcd1d9fc78)
	    at common-main.c:52
	(gdb) up 2
	#2  read_patches (range=range@entry=0x555cc5dc2b70 "@~2..", 
	    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126
	126	range-diff.c: No such file or directory.
	(gdb) print patch
	$1 = {new_name = 0x0, old_name = 0x0, def_name = 0x555cc5dc98c0 "file", 
	  old_mode = 33261, new_mode = 33188, is_new = 0, is_delete = 0, rejected = 0, 
	  ws_rule = 0, lines_added = 0, lines_deleted = 0, score = 0, 
	  extension_linenr = 0, is_toplevel_relative = 0, inaccurate_eof = 0, 
	  is_binary = 0, is_copy = 0, is_rename = 0, recount = 0, 
	  conflicted_threeway = 0, direct_to_threeway = 0, crlf_in_old = 0, 
	  fragments = 0x0, result = 0x0, resultsize = 0, 
	  old_oid_prefix = '\000' <repeats 64 times>, 
	  new_oid_prefix = '\000' <repeats 64 times>, next = 0x0, threeway_stage = {{
	      hash = '\000' <repeats 31 times>}, {hash = '\000' <repeats 31 times>}, {
	      hash = '\000' <repeats 31 times>}}}

I guess you are able to work out the details with this information. If you need
more input, please Cc: me on replies.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: Regression in v2.23

[Thomas Gummerer]

On 10/07, Uwe Kleine-König wrote:
> Hello,
> 
> With git 2.23.0 I have:
> 
> 	uwe@taurus:~/tmp/rangediff-segfault$ git init
> 	Initialized empty Git repository in /home/uwe/tmp/rangediff-segfault/.git/
> 	uwe@taurus:~/tmp/rangediff-segfault$ echo root > root
> 	uwe@taurus:~/tmp/rangediff-segfault$ git add root
> 	uwe@taurus:~/tmp/rangediff-segfault$ git commit -m root
> 	[master (root-commit) b0feddb2dee8] root
> 	 1 file changed, 1 insertion(+)
> 	 create mode 100644 root
> 	uwe@taurus:~/tmp/rangediff-segfault$ echo content > file
> 	uwe@taurus:~/tmp/rangediff-segfault$ chmod +x file
> 	uwe@taurus:~/tmp/rangediff-segfault$ git add file
> 	uwe@taurus:~/tmp/rangediff-segfault$ git commit -m file
> 	[master 45b547c57acd] file
> 	 1 file changed, 1 insertion(+)
> 	 create mode 100755 file
> 	uwe@taurus:~/tmp/rangediff-segfault$ chmod -x file
> 	uwe@taurus:~/tmp/rangediff-segfault$ git add file
> 	uwe@taurus:~/tmp/rangediff-segfault$ git commit -m 'chmod -x'
> 	[master eaa5d3b98caa] chmod -x
> 	 1 file changed, 0 insertions(+), 0 deletions(-)
> 	 mode change 100755 => 100644 file
> 	uwe@taurus:~/tmp/rangediff-segfault$ git range-diff @~2..@~ @~2..
> 	Segmentation fault (core dumped)
> 
> Bisecting points to b66885a30cb84fc61986bc4eea805a31fdbea79a, current master
> (b744c3af07a15aaeb1b82fab689995fd5528f120) segfaults in the same way.
> 
> This is somehow similar to
> https://public-inbox.org/git/20190923101929.GA18205@kitsune.suse.cz/ but
> the patch by Johannes Schindelin sent in
> https://public-inbox.org/git/pull.373.git.gitgitgadget@gmail.com/
> doesn't help me.

Thanks for the report, and testing if those patches help.

> For me the segfault also happens in
> 
> 	strbuf_addstr(&buf, patch.new_name);
> 
> with patch.new_name being NULL.
> 
> The matching backtrace and patch object looks as follows:
> 
> 	(gdb) bt
> 	#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
> 	#1  0x0000555cc448949c in strbuf_addstr (s=<optimized out>, sb=0x7ffcd1d9ef00)
> 	    at strbuf.h:292
> 	#2  read_patches (range=range@entry=0x555cc5dc2b70 "@~2..", 
> 	    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126
> 	#3  0x0000555cc44898a8 in show_range_diff (range1=0x555cc5dc2b50 "@~2..@~", 
> 	    range2=0x555cc5dc2b70 "@~2..", creation_factor=60, dual_color=1, 
> 	    diffopt=diffopt@entry=0x7ffcd1d9f680) at range-diff.c:507
> 	#4  0x0000555cc4397aa6 in cmd_range_diff (argc=<optimized out>, 
> 	    argv=<optimized out>, prefix=<optimized out>) at builtin/range-diff.c:80
> 	#5  0x0000555cc4328494 in run_builtin (argv=<optimized out>, 
> 	    argc=<optimized out>, p=<optimized out>) at git.c:445
> 	#6  handle_builtin (argc=<optimized out>, argv=<optimized out>) at git.c:674
> 	#7  0x0000555cc4329554 in run_argv (argv=0x7ffcd1d9f9e0, argcp=0x7ffcd1d9f9ec)
> 	    at git.c:741
> 	#8  cmd_main (argc=<optimized out>, argv=<optimized out>) at git.c:872
> 	#9  0x0000555cc432803a in main (argc=4, argv=0x7ffcd1d9fc78)
> 	    at common-main.c:52
> 	(gdb) up 2
> 	#2  read_patches (range=range@entry=0x555cc5dc2b70 "@~2..", 
> 	    list=list@entry=0x7ffcd1d9f280) at range-diff.c:126
> 	126	range-diff.c: No such file or directory.
> 	(gdb) print patch
> 	$1 = {new_name = 0x0, old_name = 0x0, def_name = 0x555cc5dc98c0 "file", 
> 	  old_mode = 33261, new_mode = 33188, is_new = 0, is_delete = 0, rejected = 0, 
> 	  ws_rule = 0, lines_added = 0, lines_deleted = 0, score = 0, 
> 	  extension_linenr = 0, is_toplevel_relative = 0, inaccurate_eof = 0, 
> 	  is_binary = 0, is_copy = 0, is_rename = 0, recount = 0, 
> 	  conflicted_threeway = 0, direct_to_threeway = 0, crlf_in_old = 0, 
> 	  fragments = 0x0, result = 0x0, resultsize = 0, 
> 	  old_oid_prefix = '\000' <repeats 64 times>, 
> 	  new_oid_prefix = '\000' <repeats 64 times>, next = 0x0, threeway_stage = {{
> 	      hash = '\000' <repeats 31 times>}, {hash = '\000' <repeats 31 times>}, {
> 	      hash = '\000' <repeats 31 times>}}}
> 
> I guess you are able to work out the details with this information. If you need
> more input, please Cc: me on replies.

Indeed, I was able to figure out what's going wrong from the
backtrace.  Here's a patch.  It's not ready for inclusion, as I still
need to write some tests, and make sure I'm not breaking something
else.

I can hopefully do some more testing and write automated tests later
today or tomorrow.  In the meantime it would be awesome if you could
confirm if this patch fixes the problem you were seeing.

I have pushed this to GitHub as well if you prefer that to applying
the patch yourself: https://github.com/tgummerer/git tg/range-diff-mode-only-change

--- >8 ---
Subject: [PATCH] range-diff: don't segfault with mode-only changes

If we don't have a new file, deleted file or renamed file in a diff,
we currently add 'patch.new_name' to the range-diff header.  This
works well for files that are changed.  However if we have a pure mode
change, 'patch.new_name' is NULL, and thus range-diff segfaults.

We can however rely on 'patch.def_name' in that case, which is
extracted from the 'diff --git' line and should be equal to
'patch.new_name'.  Use that instead to avoid the segfault.

Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
 range-diff.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index ba1e9a4265..d8d906b3c6 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -116,20 +116,20 @@ static int read_patches(const char *range, struct string_list *list)
 			if (len < 0)
 				die(_("could not parse git header '%.*s'"), (int)len, line);
 			strbuf_addstr(&buf, " ## ");
-			if (patch.is_new > 0)
+			free(current_filename);
+			if (patch.is_new > 0) {
 				strbuf_addf(&buf, "%s (new)", patch.new_name);
-			else if (patch.is_delete > 0)
+				current_filename = xstrdup(patch.new_name);
+			} else if (patch.is_delete > 0) {
 				strbuf_addf(&buf, "%s (deleted)", patch.old_name);
-			else if (patch.is_rename)
-				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
-			else
-				strbuf_addstr(&buf, patch.new_name);
-
-			free(current_filename);
-			if (patch.is_delete > 0)
 				current_filename = xstrdup(patch.old_name);
-			else
+			} else if (patch.is_rename) {
+				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
 				current_filename = xstrdup(patch.new_name);
+			} else {
+				strbuf_addstr(&buf, patch.def_name);
+				current_filename = xstrdup(patch.def_name);
+			}
 
 			if (patch.new_mode && patch.old_mode &&
 			    patch.old_mode != patch.new_mode)
-- 
2.23.0.501.gb744c3af07

Re: Regression in v2.23

[Junio C Hamano]

Thomas Gummerer <t.gummerer@gmail.com> writes:

> We can however rely on 'patch.def_name' in that case, which is
> extracted from the 'diff --git' line and should be equal to
> 'patch.new_name'.  Use that instead to avoid the segfault.

This patch makes the way this function calls parse_git_diff_header()
more in line with the way how it is used by its original caller in
apply.c::find_header(), but not quite.

I have to wonder if we want to move a bit of code around so that
callers of parse_git_diff_header() do not have to worry about
def_name and can rely on new_name and old_name fields correctly
filled.

There was only one caller of the parse_git_diff_header() function
before range-diff.  The division of labour between find_header() and
parse_git_diff_header() did not make any difference to the consumers
of the new/old_name fields.  They only cared that they do not have
to worry about def_name.  But by calling parse_git_diff_header()
that forces the caller to worry about def_name (which is done by
find_header() to free its callers from doing so), range-diff took
responsibility of caring, which was suboptimal.  The interface could
have been a bit more cleaned up before we started to reuse it in the
new caller, and as this bug shows, it may be time to do so now, no?

Perhaps before returing, parse_git_diff_header() should fill the two
names with xstrdup() of def_name if (!old_name && !new_name &&
!!def_name); all other cases the existing caller and this new caller
would work unchanged correctly, no?

Re: Regression in v2.23

[Uwe Kleine-König]

Hello Thomas,

On Mon, Oct 07, 2019 at 02:48:31PM +0100, Thomas Gummerer wrote:
> I can hopefully do some more testing and write automated tests later
> today or tomorrow.  In the meantime it would be awesome if you could
> confirm if this patch fixes the problem you were seeing.

I assume you already tested on my constructed example from the report. I
just tested on the original and there is works fine, too.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: Regression in v2.23

[Johannes Schindelin]

Hi Junio,

On Tue, 8 Oct 2019, Junio C Hamano wrote:

> Thomas Gummerer <t.gummerer@gmail.com> writes:
>
> > We can however rely on 'patch.def_name' in that case, which is
> > extracted from the 'diff --git' line and should be equal to
> > 'patch.new_name'.  Use that instead to avoid the segfault.
>
> This patch makes the way this function calls parse_git_diff_header()
> more in line with the way how it is used by its original caller in
> apply.c::find_header(), but not quite.
>
> I have to wonder if we want to move a bit of code around so that
> callers of parse_git_diff_header() do not have to worry about
> def_name and can rely on new_name and old_name fields correctly
> filled.
>
> There was only one caller of the parse_git_diff_header() function
> before range-diff.  The division of labour between find_header() and
> parse_git_diff_header() did not make any difference to the consumers
> of the new/old_name fields.  They only cared that they do not have
> to worry about def_name.  But by calling parse_git_diff_header()
> that forces the caller to worry about def_name (which is done by
> find_header() to free its callers from doing so), range-diff took
> responsibility of caring, which was suboptimal.  The interface could
> have been a bit more cleaned up before we started to reuse it in the
> new caller, and as this bug shows, it may be time to do so now, no?
>
> Perhaps before returing, parse_git_diff_header() should fill the two
> names with xstrdup() of def_name if (!old_name && !new_name &&
> !!def_name); all other cases the existing caller and this new caller
> would work unchanged correctly, no?

FWIW I totally agree.

Ciao,
Dscho

Re: Regression in v2.23

[Johannes Schindelin]

Hi Thomas,

On Mon, 7 Oct 2019, Thomas Gummerer wrote:

> Subject: [PATCH] range-diff: don't segfault with mode-only changes
>
> If we don't have a new file, deleted file or renamed file in a diff,
> we currently add 'patch.new_name' to the range-diff header.  This
> works well for files that are changed.  However if we have a pure mode
> change, 'patch.new_name' is NULL, and thus range-diff segfaults.
>
> We can however rely on 'patch.def_name' in that case, which is
> extracted from the 'diff --git' line and should be equal to
> 'patch.new_name'.  Use that instead to avoid the segfault.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
>  range-diff.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index ba1e9a4265..d8d906b3c6 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -116,20 +116,20 @@ static int read_patches(const char *range, struct string_list *list)
>  			if (len < 0)
>  				die(_("could not parse git header '%.*s'"), (int)len, line);
>  			strbuf_addstr(&buf, " ## ");
> -			if (patch.is_new > 0)
> +			free(current_filename);
> +			if (patch.is_new > 0) {
>  				strbuf_addf(&buf, "%s (new)", patch.new_name);
> -			else if (patch.is_delete > 0)
> +				current_filename = xstrdup(patch.new_name);
> +			} else if (patch.is_delete > 0) {
>  				strbuf_addf(&buf, "%s (deleted)", patch.old_name);
> -			else if (patch.is_rename)
> -				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
> -			else
> -				strbuf_addstr(&buf, patch.new_name);
> -
> -			free(current_filename);
> -			if (patch.is_delete > 0)
>  				current_filename = xstrdup(patch.old_name);
> -			else
> +			} else if (patch.is_rename) {
> +				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
>  				current_filename = xstrdup(patch.new_name);
> +			} else {
> +				strbuf_addstr(&buf, patch.def_name);
> +				current_filename = xstrdup(patch.def_name);
> +			}
>
>  			if (patch.new_mode && patch.old_mode &&
>  			    patch.old_mode != patch.new_mode)
> --

I am not quite sure that this fixes it... Here is my regression test case:

-- snipsnap --
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ec548654ce1..6aca7f5a5b1 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -354,4 +354,18 @@ test_expect_success 'format-patch --range-diff as commentary' '
 	grep "> 1: .* new message" 0001-*
 '

+test_expect_success 'range-diff and mode-only changes' '
+	git switch -c mode-only &&
+
+	test_commit mode-only &&
+
+	: pretend it is executable &&
+	git add --chmod=+x mode-only.t &&
+	chmod a+x mode-only.t &&
+	test_tick &&
+	git commit -m mode-only &&
+
+	git range-diff @^...
+'
+
 test_done

Re: Regression in v2.23

[Johannes Schindelin]

Hi Thomas,

On Tue, 8 Oct 2019, Johannes Schindelin wrote:

> On Mon, 7 Oct 2019, Thomas Gummerer wrote:
>
> > Subject: [PATCH] range-diff: don't segfault with mode-only changes
> >
> > If we don't have a new file, deleted file or renamed file in a diff,
> > we currently add 'patch.new_name' to the range-diff header.  This
> > works well for files that are changed.  However if we have a pure mode
> > change, 'patch.new_name' is NULL, and thus range-diff segfaults.
> >
> > We can however rely on 'patch.def_name' in that case, which is
> > extracted from the 'diff --git' line and should be equal to
> > 'patch.new_name'.  Use that instead to avoid the segfault.
> >
> > Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> > ---
> >  range-diff.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/range-diff.c b/range-diff.c
> > index ba1e9a4265..d8d906b3c6 100644
> > --- a/range-diff.c
> > +++ b/range-diff.c
> > @@ -116,20 +116,20 @@ static int read_patches(const char *range, struct string_list *list)
> >  			if (len < 0)
> >  				die(_("could not parse git header '%.*s'"), (int)len, line);
> >  			strbuf_addstr(&buf, " ## ");
> > -			if (patch.is_new > 0)
> > +			free(current_filename);
> > +			if (patch.is_new > 0) {
> >  				strbuf_addf(&buf, "%s (new)", patch.new_name);
> > -			else if (patch.is_delete > 0)
> > +				current_filename = xstrdup(patch.new_name);
> > +			} else if (patch.is_delete > 0) {
> >  				strbuf_addf(&buf, "%s (deleted)", patch.old_name);
> > -			else if (patch.is_rename)
> > -				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
> > -			else
> > -				strbuf_addstr(&buf, patch.new_name);
> > -
> > -			free(current_filename);
> > -			if (patch.is_delete > 0)
> >  				current_filename = xstrdup(patch.old_name);
> > -			else
> > +			} else if (patch.is_rename) {
> > +				strbuf_addf(&buf, "%s => %s", patch.old_name, patch.new_name);
> >  				current_filename = xstrdup(patch.new_name);
> > +			} else {
> > +				strbuf_addstr(&buf, patch.def_name);
> > +				current_filename = xstrdup(patch.def_name);
> > +			}
> >
> >  			if (patch.new_mode && patch.old_mode &&
> >  			    patch.old_mode != patch.new_mode)
> > --
>
> I am not quite sure that this fixes it...

Whoops. I should learn to distrust `git apply` claiming success when
running in `t/`. (I tried to apply your patch, but nothing was actually
applied before I ran `make`.)

So it totally fixes the issue (feel free to just pick up the regression
test case).

Having said that, I would agree with Junio that it'd be nicer to make
`parse_git_diff_header()` more useful to all of its callers, including
future ones.

Sorry for the misreport, and thanks for all the patch,
Dscho

> Here is my regression test case:
>
> -- snipsnap --
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index ec548654ce1..6aca7f5a5b1 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -354,4 +354,18 @@ test_expect_success 'format-patch --range-diff as commentary' '
>  	grep "> 1: .* new message" 0001-*
>  '
>
> +test_expect_success 'range-diff and mode-only changes' '
> +	git switch -c mode-only &&
> +
> +	test_commit mode-only &&
> +
> +	: pretend it is executable &&
> +	git add --chmod=+x mode-only.t &&
> +	chmod a+x mode-only.t &&
> +	test_tick &&
> +	git commit -m mode-only &&
> +
> +	git range-diff @^...
> +'
> +
>  test_done
>
>
>

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

[Thomas Gummerer]

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(-)

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

Re: [PATCH v2] range-diff: don't segfault with mode-only changes

[Johannes Schindelin]

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

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

Re: [PATCH v2] range-diff: don't segfault with mode-only changes

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |