git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] fix segfault with git log -c --follow

From
Clemens Buchacher <drizzd@aon.at>
Date
May 27, 2013, 22:49 UTC
Message-ID
<20130527224957.GA7492@ecki>

In diff_tree_combined we make a copy of diffopts. In try_to_follow_renames, called via diff_tree_sha1, we free and re-initialize diffopts->pathspec->items. Since we did not make a deep copy of diffopts in diff_tree_combined, the original diffopts does not get the update. By the time we return from diff_tree_combined, rev->diffopt->pathspec->items points to an invalid memory address. We get a segfault next time we try to access that pathspec.

Instead, along with the copy of diffopts, make a copy pathspec->items as well.

We would also have to make a copy of pathspec->raw to keep it consistent with pathspec->items, but nobody seems to rely on that.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

I wonder why I get a segfault from this so reliably, since it's not actually a null-pointer dereference. Maybe this is gcc 4.8 doing something different from previous versions?

Also, I have absolutely no confidence in my understanding of this code. This is the first solution that came to mind, and could be totally wrong. I just figured a patch is better than no patch.

Clemens
 combine-diff.c |  3 +++
 t/t4202-log.sh | 14 ++++++++++++++
 2 files changed, 17 insertions(+)
diff --git a/combine-diff.c b/combine-diff.c
index 77d7872..8825604 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1302,6 +1302,7 @@ void diff_tree_combined(const unsigned char *sha1,
 	int i, num_paths, needsep, show_log_first, num_parent = parents->nr;
 
 	diffopts = *opt;
+	diff_tree_setup_paths(diffopts.pathspec.raw, &diffopts);
 	diffopts.output_format = DIFF_FORMAT_NO_OUTPUT;
 	DIFF_OPT_SET(&diffopts, RECURSIVE);
 	DIFF_OPT_CLR(&diffopts, ALLOW_EXTERNAL);
@@ -1372,6 +1373,8 @@ void diff_tree_combined(const unsigned char *sha1,
 		paths = paths->next;
 		free(tmp);
 	}
+
+	diff_tree_release_paths(&diffopts);
 }
 
 void diff_tree_combined_merge(const struct commit *commit, int dense,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 9243a97..cb03d28 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -530,6 +530,20 @@ test_expect_success 'show added path under "--follow -M"' '
 	)
 '
 
+test_expect_success 'git log -c --follow' '
+	test_create_repo follow-c &&
+	(
+		cd follow-c &&
+		test_commit initial file original &&
+		git rm file &&
+		test_commit rename file2 original &&
+		git reset --hard initial &&
+		test_commit modify file foo &&
+		git merge -m merge rename &&
+		git log -c --follow file2
+	)
+'
+
 cat >expect <<\EOF
 *   commit COMMIT_OBJECT_NAME
 |\  Merge: MERGE_PARENTS
-- 
1.8.2.3
Next: Junio C Hamano
Message 1 of 4 in “fix segfault with git log -c --follow”
  1. fix segfault with git log -c --followClemens Buchacher, May 27, 2013
  2. Junio C HamanoMay 28, 2013
  3. Clemens BuchacherMay 28, 2013
  4. Junio C HamanoMay 28, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.