[PATCH] builtin/blame: destroy initialized commit_info only

[PATCH] builtin/blame: destroy initialized commit_info only

[Eric Sunshine]

Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),
find_alignment() has been invoking commit_info_destroy() on an
uninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not
set). commit_info_destroy() calls strbuf_release() for each of
'commit_info' strbuf member, which randomly invokes free() on whatever
random stack value happens to be reside in strbuf.buf, thus leading to
periodic crashes.

Reported-by: Dilyan Palauzov <dilyan.palauzov@aegee.org>
Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

No test accompanying this fix since I don't know how to formulate one.

Discussion: http://thread.gmane.org/gmane.comp.version-control.git/263534

 builtin/blame.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 303e217..a3cc972 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2085,7 +2085,6 @@ static void find_alignment(struct scoreboard *sb, int *option)
 
 	for (e = sb->ent; e; e = e->next) {
 		struct origin *suspect = e->suspect;
-		struct commit_info ci;
 		int num;
 
 		if (compute_auto_abbrev)
@@ -2096,6 +2095,7 @@ static void find_alignment(struct scoreboard *sb, int *option)
 		if (longest_file < num)
 			longest_file = num;
 		if (!(suspect->commit->object.flags & METAINFO_SHOWN)) {
+			struct commit_info ci;
 			suspect->commit->object.flags |= METAINFO_SHOWN;
 			get_commit_info(suspect->commit, &ci, 1);
 			if (*option & OUTPUT_SHOW_EMAIL)
@@ -2104,6 +2104,7 @@ static void find_alignment(struct scoreboard *sb, int *option)
 				num = utf8_strwidth(ci.author.buf);
 			if (longest_author < num)
 				longest_author = num;
+			commit_info_destroy(&ci);
 		}
 		num = e->s_lno + e->num_lines;
 		if (longest_src_lines < num)
@@ -2113,8 +2114,6 @@ static void find_alignment(struct scoreboard *sb, int *option)
 			longest_dst_lines = num;
 		if (largest_score < ent_score(sb, e))
 			largest_score = ent_score(sb, e);
-
-		commit_info_destroy(&ci);
 	}
 	max_orig_digits = decimal_width(longest_src_lines);
 	max_digits = decimal_width(longest_dst_lines);
-- 
2.3.0.rc2.191.g303d43c

Re: [PATCH] builtin/blame: destroy initialized commit_info only

[Eric Sunshine]

On Mon, Feb 9, 2015 at 4:28 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),
> find_alignment() has been invoking commit_info_destroy() on an
> uninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not
> set). commit_info_destroy() calls strbuf_release() for each of

s/each of/each/

...despite several proof-reads (sigh).

> 'commit_info' strbuf member, which randomly invokes free() on whatever
> random stack value happens to be reside in strbuf.buf, thus leading to
> periodic crashes.
>
> Reported-by: Dilyan Palauzov <dilyan.palauzov@aegee.org>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

Re: [PATCH] builtin/blame: destroy initialized commit_info only

[Eric Sunshine]

On Mon, Feb 9, 2015 at 4:33 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, Feb 9, 2015 at 4:28 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),
>> find_alignment() has been invoking commit_info_destroy() on an
>> uninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not
>> set). commit_info_destroy() calls strbuf_release() for each of
>
> s/each of/each/
>
> ...despite several proof-reads (sigh).
>
>> 'commit_info' strbuf member, which randomly invokes free() on whatever
>> random stack value happens to be reside in strbuf.buf, thus leading to

s/to be/to/

(sigh again)

>> periodic crashes.
>>
>> Reported-by: Dilyan Palauzov <dilyan.palauzov@aegee.org>
>> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

Re: [PATCH] builtin/blame: destroy initialized commit_info only

[Jeff King]

On Mon, Feb 09, 2015 at 04:28:07PM -0500, Eric Sunshine wrote:

> Since ea02ffa3 (mailmap: simplify map_user() interface, 2013-01-05),
> find_alignment() has been invoking commit_info_destroy() on an
> uninitialized auto 'struct commit_info' (when METAINFO_SHOWN is not
> set). commit_info_destroy() calls strbuf_release() for each of
> 'commit_info' strbuf member, which randomly invokes free() on whatever
> random stack value happens to be reside in strbuf.buf, thus leading to
> periodic crashes.
> 
> Reported-by: Dilyan Palauzov <dilyan.palauzov@aegee.org>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
> 
> No test accompanying this fix since I don't know how to formulate one.
> 
> Discussion: http://thread.gmane.org/gmane.comp.version-control.git/263534

Thanks, good catch. This is almost certainly the source of the reported
bug. I was curious why we didn't find this until now, and what we could
do to detect similar problems. You can stop reading if you're not
interested. :)

We call strbuf_release on each of the uninitialized components. That
will only call free(sb->buf) if sb->alloc is non-zero. And if sb->buf is
0, then that is OK (it is a noop to free NULL). So the bug only shows
itself when the uninitialized memory is such that both are non-zero.

We're in a loop here, where we set the METAINFO_SHOWN to avoid
processing commits from the loop twice. So generally the first time
through the loop (when we really do have uninitialized crap in the
struct), that flag will not be set, and we will fill in the struct. Then
a later iteration of the loop does have the flag set, and does not fill
in the struct. But quite often we'll have whatever was left in the
struct from the previous loop iteration. Which, because strbuf_release()
re-initializes the strbufs, is a valid strbuf which it is a noop to
free.

Of course, that's entirely up to the compiler. It's reasonable to use
the same block for the variable in each iteration of the loop, but it is
free to do whatever it likes. It may even come and go with various
optimization settings.

Obviously if we hit a compiler that feeds the uninitialized values to
free(), this is pretty easy to detect with valgrind or another
heap-checking library. But let's assume we have a compiler that does the
"reuse" behavior. There's no bug in the generated code, but it's
technically undefined behavior. Can we detect it?

The compiler _could_ know statically that this is a use of uninitialized
memory, except that it only sees:

    commit_info_destroy(&ci);

on the uninitialized memory. Since we are passing a pointer to a
function it can't see, it has to assume that it's actually initializing
the memory for us (i.e., it looks the same as commit_info_init). So it
doesn't catch it. I don't know of a way to annotate the pointers better,
or if there is a way to get the compiler to see the code in a more
global way.

Valgrind cannot detect this. With the "reuse" behavior, we never even
call malloc, so there's nothing for it to see there. We are accessing
uninitialized memory, but it's on the stack. Valgrind doesn't even know
about it, or that we are looping here.

Clang's address sanitizer has compiler support, so it does get to see
this memory and could put a canary value in for each loop iteration. But
it doesn't. Instead, you're supposed to use the "memory sanitizer" to
catch uninitialized memory.

I tried that, but got overwhelmed with false positives. Like valgrind,
it has problems accepting that memory written by zlib is actually
initialized. But in theory, if we went to the work to annotate some
false positives, it should be able to find this problem.

-Peff

Re: [PATCH] builtin/blame: destroy initialized commit_info only

[Jeff King]

On Mon, Feb 09, 2015 at 06:24:35PM -0500, Jeff King wrote:

> Clang's address sanitizer has compiler support, so it does get to see
> this memory and could put a canary value in for each loop iteration. But
> it doesn't. Instead, you're supposed to use the "memory sanitizer" to
> catch uninitialized memory.
> 
> I tried that, but got overwhelmed with false positives. Like valgrind,
> it has problems accepting that memory written by zlib is actually
> initialized. But in theory, if we went to the work to annotate some
> false positives, it should be able to find this problem.

I got rid of the false positives here, through a combination of
compiling with NO_OPENSSL (since it otherwise doesn't know that
git_SHA1_Final is initializing hashes), and this patch which lets it
assume that the output of zlib (at least for these cases) is always
initialized:

diff --git a/sha1_file.c b/sha1_file.c
index 30995e6..28c8f84 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1682,6 +1682,7 @@ unsigned long get_size_from_delta(struct packed_git *p,
 	git_zstream stream;
 	int st;
 
+	memset(delta_head, 0, 20);
 	memset(&stream, 0, sizeof(stream));
 	stream.next_out = delta_head;
 	stream.avail_out = sizeof(delta_head);
@@ -1973,6 +1974,7 @@ static void *unpack_compressed_entry(struct packed_git *p,
 	buffer = xmallocz_gently(size);
 	if (!buffer)
 		return NULL;
+	memset(buffer, 0, size);
 	memset(&stream, 0, sizeof(stream));
 	stream.next_out = buffer;
 	stream.avail_out = size + 1;


Sadly, though, the test case in question runs to completion. It does not
seem to detect our use of uninitialized memory. :(

-Peff