Re: [PATCH 2/3] Guard memory overwriting in resolve_ref's static buffer
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:36
Nguyễn Thái Ngọc Duy wrote:
Michael Haggerty has an idea [1] that, instead of passing the same static buffer to caller every time the function is called, we free the old buffer and allocate the new one. This way access to the old (now invalid) buffer may be caught. This patch applies the same principle for resolve_ref() with a few modifications:
[...]
- Rely on mmap/mprotect to catch illegal access. We need valgrind or some other memory tracking tool to reliably catch this in Michael's approach.
I wonder if the lower-tech approach would be so bad in practice. On systems using glibc, if MALLOC_PERTURB_ is set, then the value will always be clobbered on free(). It would be possible to do the same explicitly in resolve_ref() for platforms without such a feature.
- Because mprotect is used instead of munmap, we definitely leak memory. Hopefully callers will not put resolve_ref() in a loop that runs 1 million times.
Have you measured the performance impact when GIT_DEBUG_MEMCHECK is not set? (I don't expect major problems, but sometimes code surprises me.) [...]
Also introduce a new target, "make memcheck", that runs tests with this flag on.
Neat. Did it catch any bugs?
quoted hunk ↗ jump to hunk
--- a/cache.h +++ b/cache.h@@ -865,7 +865,8 @@ extern int read_ref(const char *filename, unsigned char *sha1); * * errno is sometimes set on errors, but not always. */ -extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag); +#define resolve_ref(ref, sha1, reading, flag) resolve_ref_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)
__FUNCTION__ is nonstandard, though it's probably supported pretty widely and we can do #ifndef __FUNCTION__ #define __FUNCTION__ something-else #endif in git-compat-util.h when we discover a platform that doesn't support it if needed. __func__ was introduced in C99. __LINE__ and __FILE__ should work everywhere. [...]
quoted hunk ↗ jump to hunk
--- /dev/null +++ b/t/t0071-memcheck.sh@@ -0,0 +1,12 @@ +#!/bin/sh + +test_description='test that GIT_DEBUG_MEMCHECK works correctly' +. ./test-lib.sh + +test_expect_success 'test-resolve-ref must crash' ' + GIT_DEBUG_MEMCHECK=1 test-resolve-ref + exit_code=$? && + test $exit_code -eq 139
Micronit: something like ( GIT_DEBUG_MEMCHECK=1 && export GIT_DEBUG_MEMCHECK && test_expect_code 139 test-resolve-ref ) would fit better in an &&-list.
quoted hunk ↗ jump to hunk
--- a/wrapper.c +++ b/wrapper.c@@ -60,6 +60,28 @@ void *xmallocz(size_t size) return ret; } +void *xmalloc_mmap(size_t size, const char *file, int line) +{ + int *ret; + size += sizeof(int*) * 3; + ret = mmap(NULL, size, PROT_READ | PROT_WRITE, + MAP_ANONYMOUS | MAP_PRIVATE, -1, 0); + if (ret == (int*)-1) + die_errno("unable to mmap %lu bytes anonymously", + (unsigned long)size); + + ret[0] = (int)file; + ret[1] = line; + ret[2] = size; + return ret + 3;
Would this work on 64-bit platforms? How does one retrieve the line and file number? I guess one is expected to retrieve them from the core dump, but a few words of advice in Documentation/technical would be helpful.