On 2013-01-23, at 23:07, Jeff King <p...@peff.net> wrote:

> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:
> 
>> Several areas of code would free buffers for object structs that
>> contained them ("struct tree" and "struct commit"), but without clearing
>> the "parsed" flag. parse_object would clear the flag for "struct tree",
>> but commits would remain in an invalid state (marked as parsed but with
>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
>> invalid objects stay around and can lead to bad behavior.
>> 
>> In particular, running pickaxe on all refs in a repository with a cached
>> textconv could segfault. If the textconv cache ref was evaluated first
>> by cmd_log_walk, a subsequent notes_cache_match_validity call would
>> dereference NULL.
> 
> Just to be sure I understand, what is going on is something like this?
> 
> Log reads all refs, including notes. After showing the notes commit,
> log frees the buffer to save memory.  Later, doing the diff on a
> commit causes us to use the notes_cache mechanism. That will look at
> the commit at the tip of the notes ref, which log has already
> processed. It will call parse_commit, but that will do nothing, as the
> parsed flag is already set, but the commit buffer remains NULL.
> 
> I wonder if this could be the source of the segfault here, too:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355

Indeed, that description matches what I see. The other segfault seems to be in 
the same place too.

The segfault hits with the following stack (optimization off):

0   git-log         parse_commit_header + 39 (pretty.c:738)
1   git-log         format_commit_one + 3102 (pretty.c:1138)
2   git-log         format_commit_item + 177 (pretty.c:1203)
3   git-log         strbuf_expand + 172 (strbuf.c:247)
4   git-log         format_commit_message + 196 (pretty.c:1263)
5   git-log         notes_cache_match_validity + 215 (notes-cache.c:22)
6   git-log         notes_cache_init + 212 (notes-cache.c:41)
7   git-log         userdiff_get_textconv + 176 (userdiff.c:301)
8   git-log         get_textconv + 66 (diff.c:2233)
9   git-log         has_changes + 53 (diffcore-pickaxe.c:205)
10  git-log         pickaxe + 299 (diffcore-pickaxe.c:40)
11  git-log         diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)
12  git-log         diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)
13  git-log         diffcore_std + 162 (diff.c:4673)
14  git-log         log_tree_diff_flush + 43 (log-tree.c:721)
15  git-log         log_tree_diff + 566 (log-tree.c:826)
16  git-log         log_tree_commit + 63 (log-tree.c:847)
17  git-log         cmd_log_walk + 172 (log.c:301)
18  git-log         cmd_log + 186 (log.c:568)
19  git-log         run_builtin + 475 (git.c:273)
20  git-log         handle_internal_command + 199 (git.c:434)
21  git-log         main + 149 (git.c:523)
22  libdyld.dylib   start + 1

commit->message was freed and nulled earlier in a call to cmd_log_walk. This 
test reproduces it reliably on my machine:

diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces 
correct results' '
        test_cmp expect actual
'

+cat >expect <<EOF
+./helper other (refs/notes/textconv/magic)
+one
+EOF
+# add empty commit on master to make bug more reproducible
+test_expect_success 'pickaxe with cached textconv' '
+       git commit --allow-empty -m another &&
+       git log -S other --pretty=tformat:%s%d --all >actual &&
+       test_cmp expect actual
+'
+
test_done




Jonathon Mah
m...@jonathonmah.com


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to