From: Clemens Buchacher <drizzd@aon.at>
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] fix segfault with git log -c --follow
Date: Tue, 28 May 2013 00:49:57 +0200 [thread overview]
Message-ID: <20130527224957.GA7492@ecki> (raw)
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 reply other threads:[~2013-05-27 23:22 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-27 22:49 Clemens Buchacher [this message]
2013-05-28 17:22 ` [PATCH] fix segfault with git log -c --follow Junio C Hamano
2013-05-28 22:54 ` Clemens Buchacher
2013-05-28 23:24 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130527224957.GA7492@ecki \
--to=drizzd@aon.at \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).