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?
> --- 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.
[...]
> --- /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.
> --- 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.
--
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