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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 10, 2011, 21:10 UTC
Message-ID: <20111210211040.GB24817@elie.hsd1.il.comcast.net>
In-Reply-To: <1323521631-24320-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy wrote:

Show 8 quoted lines

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

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

[...]

Show 12 quoted lines

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

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

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.