2011/12/11 Jonathan Nieder <jrnieder@xxxxxxxxx>:
>> - 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.
Clobbered value may be carried around for some time before the code
detects wrong value. It'd be hard to track back where the root cause
is.
>> - 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.)
No. I wish we had a performance test suite. Which use cases should I test?
> [...]
>> Also introduce a new target, "make memcheck", that runs tests with this
>> flag on.
>
> Neat. Did it catch any bugs?
No, otherwise I would have sent more patches ;)
>> --- 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.
Will change to __FILE__ then
>> --- 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?
No idea (I'm on 32-bit). I don't see any reasons why it would not
work. But that function may cause unaligned access, I think.
> 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.
from coredump.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html