Re: Regression in v2.23

[Thomas Gummerer <t.gummerer@gmail.com>]

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