On 2013-01-23, at 23:07, Jeff King <[email protected]> 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
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
