{"thread":{"id":"38541","subject":"[PATCH] builtin/blame: destroy initialized commit_info only","startedAt":"2015-02-09T21:28:07Z","lastAt":"2015-02-09T23:42:25Z","messageCount":5,"participants":["Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"255855","messageId":"1423517287-8354-1-git-send-email-sunshine@sunshineco.com","threadId":"38541","inReplyTo":null,"subject":"[PATCH] builtin/blame: destroy initialized commit_info only","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-09T21:28:07Z","receivedAt":"2015-02-09T21:28:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),\nfind_alignment() has been invoking commit_info_destroy() on an\nuninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not\nset). commit_info_destroy() calls strbuf_release() for each of\n'commit_info' strbuf member, which randomly invokes free() on whatever\nrandom stack value happens to be reside in strbuf.buf, thus leading to\nperiodic crashes.\n\nReported-by: Dilyan Palauzov \nSigned-off-by: Eric Sunshine \n---\n\nNo test accompanying this fix since I don't know how to formulate one.\n\nDiscussion: http://thread.gmane.org/gmane.comp.version-control.git/263534\n\n builtin/blame.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 303e217..a3cc972 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2085,7 +2085,6 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \n \tfor (e = sb->ent; e; e = e->next) {\n \t\tstruct origin *suspect = e->suspect;\n-\t\tstruct commit_info ci;\n \t\tint num;\n \n \t\tif (compute_auto_abbrev)\n@@ -2096,6 +2095,7 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\tif (longest_file < num)\n \t\t\tlongest_file = num;\n \t\tif (!(suspect->commit->object.flags & METAINFO_SHOWN)) {\n+\t\t\tstruct commit_info ci;\n \t\t\tsuspect->commit->object.flags |= METAINFO_SHOWN;\n \t\t\tget_commit_info(suspect->commit, &ci, 1);\n \t\t\tif (*option & OUTPUT_SHOW_EMAIL)\n@@ -2104,6 +2104,7 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\t\t\tnum = utf8_strwidth(ci.author.buf);\n \t\t\tif (longest_author < num)\n \t\t\t\tlongest_author = num;\n+\t\t\tcommit_info_destroy(&ci);\n \t\t}\n \t\tnum = e->s_lno + e->num_lines;\n \t\tif (longest_src_lines < num)\n@@ -2113,8 +2114,6 @@ static void find_alignment(struct scoreboard *sb, int *option)\n \t\t\tlongest_dst_lines = num;\n \t\tif (largest_score < ent_score(sb, e))\n \t\t\tlargest_score = ent_score(sb, e);\n-\n-\t\tcommit_info_destroy(&ci);\n \t}\n \tmax_orig_digits = decimal_width(longest_src_lines);\n \tmax_digits = decimal_width(longest_dst_lines);\n-- \n2.3.0.rc2.191.g303d43c\n"},{"id":"255856","messageId":"CAPig+cQAmYzzVF5+0j6AgcpEsi6SOVkXyqw4uOMCUjExPSLz_w@mail.gmail.com","threadId":"38541","inReplyTo":"1423517287-8354-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] builtin/blame: destroy initialized commit_info only","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-09T21:33:17Z","receivedAt":"2015-02-09T21:33:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 9, 2015 at 4:28 PM, Eric Sunshine wrote:\n> Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),\n> find_alignment() has been invoking commit_info_destroy() on an\n> uninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not\n> set). commit_info_destroy() calls strbuf_release() for each of\n\ns/each of/each/\n\n...despite several proof-reads (sigh).\n\n> 'commit_info' strbuf member, which randomly invokes free() on whatever\n> random stack value happens to be reside in strbuf.buf, thus leading to\n> periodic crashes.\n>\n> Reported-by: Dilyan Palauzov \n> Signed-off-by: Eric Sunshine \n"},{"id":"255867","messageId":"20150209232435.GB24814@peff.net","threadId":"38541","inReplyTo":"1423517287-8354-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] builtin/blame: destroy initialized commit_info only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-09T23:24:36Z","receivedAt":"2015-02-09T23:24:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 09, 2015 at 04:28:07PM -0500, Eric Sunshine wrote:\n\n> Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),\n> find_alignment() has been invoking commit_info_destroy() on an\n> uninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not\n> set). commit_info_destroy() calls strbuf_release() for each of\n> 'commit_info' strbuf member, which randomly invokes free() on whatever\n> random stack value happens to be reside in strbuf.buf, thus leading to\n> periodic crashes.\n> \n> Reported-by: Dilyan Palauzov \n> Signed-off-by: Eric Sunshine \n> ---\n> \n> No test accompanying this fix since I don't know how to formulate one.\n> \n> Discussion: http://thread.gmane.org/gmane.comp.version-control.git/263534\n\nThanks, good catch. This is almost certainly the source of the reported\nbug. I was curious why we didn't find this until now, and what we could\ndo to detect similar problems. You can stop reading if you're not\ninterested. :)\n\nWe call strbuf_release on each of the uninitialized components. That\nwill only call free(sb->buf) if sb->alloc is non-zero. And if sb->buf is\n0, then that is OK (it is a noop to free NULL). So the bug only shows\nitself when the uninitialized memory is such that both are non-zero.\n\nWe're in a loop here, where we set the METAINFO_SHOWN to avoid\nprocessing commits from the loop twice. So generally the first time\nthrough the loop (when we really do have uninitialized crap in the\nstruct), that flag will not be set, and we will fill in the struct. Then\na later iteration of the loop does have the flag set, and does not fill\nin the struct. But quite often we'll have whatever was left in the\nstruct from the previous loop iteration. Which, because strbuf_release()\nre-initializes the strbufs, is a valid strbuf which it is a noop to\nfree.\n\nOf course, that's entirely up to the compiler. It's reasonable to use\nthe same block for the variable in each iteration of the loop, but it is\nfree to do whatever it likes. It may even come and go with various\noptimization settings.\n\nObviously if we hit a compiler that feeds the uninitialized values to\nfree(), this is pretty easy to detect with valgrind or another\nheap-checking library. But let's assume we have a compiler that does the\n\"reuse\" behavior. There's no bug in the generated code, but it's\ntechnically undefined behavior. Can we detect it?\n\nThe compiler _could_ know statically that this is a use of uninitialized\nmemory, except that it only sees:\n\n commit_info_destroy(&ci);\n\non the uninitialized memory. Since we are passing a pointer to a\nfunction it can't see, it has to assume that it's actually initializing\nthe memory for us (i.e., it looks the same as commit_info_init). So it\ndoesn't catch it. I don't know of a way to annotate the pointers better,\nor if there is a way to get the compiler to see the code in a more\nglobal way.\n\nValgrind cannot detect this. With the \"reuse\" behavior, we never even\ncall malloc, so there's nothing for it to see there. We are accessing\nuninitialized memory, but it's on the stack. Valgrind doesn't even know\nabout it, or that we are looping here.\n\nClang's address sanitizer has compiler support, so it does get to see\nthis memory and could put a canary value in for each loop iteration. But\nit doesn't. Instead, you're supposed to use the \"memory sanitizer\" to\ncatch uninitialized memory.\n\nI tried that, but got overwhelmed with false positives. Like valgrind,\nit has problems accepting that memory written by zlib is actually\ninitialized. But in theory, if we went to the work to annotate some\nfalse positives, it should be able to find this problem.\n\n-Peff\n"},{"id":"255869","messageId":"20150209233707.GA20462@peff.net","threadId":"38541","inReplyTo":"20150209232435.GB24814@peff.net","subject":"Re: [PATCH] builtin/blame: destroy initialized commit_info only","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-09T23:37:08Z","receivedAt":"2015-02-09T23:37:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 09, 2015 at 06:24:35PM -0500, Jeff King wrote:\n\n> Clang's address sanitizer has compiler support, so it does get to see\n> this memory and could put a canary value in for each loop iteration. But\n> it doesn't. Instead, you're supposed to use the \"memory sanitizer\" to\n> catch uninitialized memory.\n> \n> I tried that, but got overwhelmed with false positives. Like valgrind,\n> it has problems accepting that memory written by zlib is actually\n> initialized. But in theory, if we went to the work to annotate some\n> false positives, it should be able to find this problem.\n\nI got rid of the false positives here, through a combination of\ncompiling with NO_OPENSSL (since it otherwise doesn't know that\ngit_SHA1_Final is initializing hashes), and this patch which lets it\nassume that the output of zlib (at least for these cases) is always\ninitialized:\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 30995e6..28c8f84 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1682,6 +1682,7 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \tgit_zstream stream;\n \tint st;\n \n+\tmemset(delta_head, 0, 20);\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = delta_head;\n \tstream.avail_out = sizeof(delta_head);\n@@ -1973,6 +1974,7 @@ static void *unpack_compressed_entry(struct packed_git *p,\n \tbuffer = xmallocz_gently(size);\n \tif (!buffer)\n \t\treturn NULL;\n+\tmemset(buffer, 0, size);\n \tmemset(&stream, 0, sizeof(stream));\n \tstream.next_out = buffer;\n \tstream.avail_out = size + 1;\n\n\nSadly, though, the test case in question runs to completion. It does not\nseem to detect our use of uninitialized memory. :(\n\n-Peff\n"},{"id":"255870","messageId":"CAPig+cQs-W=6K70LfsGPDnxRSQXfcQ5d0voc-KKd4BbeZOe9hA@mail.gmail.com","threadId":"38541","inReplyTo":"CAPig+cQAmYzzVF5+0j6AgcpEsi6SOVkXyqw4uOMCUjExPSLz_w@mail.gmail.com","subject":"Re: [PATCH] builtin/blame: destroy initialized commit_info only","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-02-09T23:42:25Z","receivedAt":"2015-02-09T23:42:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 9, 2015 at 4:33 PM, Eric Sunshine wrote:\n> On Mon, Feb 9, 2015 at 4:28 PM, Eric Sunshine wrote:\n>> Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),\n>> find_alignment() has been invoking commit_info_destroy() on an\n>> uninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not\n>> set). commit_info_destroy() calls strbuf_release() for each of\n>\n> s/each of/each/\n>\n> ...despite several proof-reads (sigh).\n>\n>> 'commit_info' strbuf member, which randomly invokes free() on whatever\n>> random stack value happens to be reside in strbuf.buf, thus leading to\n\ns/to be/to/\n\n(sigh again)\n\n>> periodic crashes.\n>>\n>> Reported-by: Dilyan Palauzov \n>> Signed-off-by: Eric Sunshine \n"}]}