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

2011/12/11 Jonathan Nieder [off-list ref]:

quoted

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

quoted

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

[...]

quoted

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

quoted

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

quoted

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