Re: [PATCH 2/3] Guard memory overwriting in resolve_ref's static buffer

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Dec 11, 2011, 09:22 UTC
Message-ID: <CACsJy8BRs2Xh5AgibyW=X41cSDVAzMDo3xXSjSpkm5wyKeYN=A@mail.gmail.com>
In-Reply-To: <20111210211040.GB24817@elie.hsd1.il.comcast.net>

2011/12/11 Jonathan Nieder <jrnieder@gmail.com>:

Show 8 quoted lines

>>  - 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.

Show 6 quoted lines

>>  - 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?

Show 5 quoted lines

> [...]
>> 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 ;)

Show 19 quoted lines

>> --- 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

Show 22 quoted lines

>> --- 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