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

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> diff --git a/cache.h b/cache.h
> index 4887a3e..ba5e911 100644
> --- 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, __FILE__, __LINE__)
> +extern const char *resolve_ref_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);
>  extern char *resolve_refdup(const char *ref, unsigned char *sha1, int reading, int *flag);
>  
>  extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);

Eek.

> diff --git a/wrapper.c b/wrapper.c
> index 85f09df..02b6c81 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -60,6 +60,33 @@ void *xmallocz(size_t size)
>  	return ret;
>  }
>  
> +void *xmalloc_mmap(size_t size, const char *file, int line)
> +{
> +	struct alloc_header *block;
> +	size += offsetof(struct alloc_header,buf);
> +	block = mmap(NULL, size, PROT_READ | PROT_WRITE,
> +		   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> +	if (block == (struct alloc_header*)-1)
> +		die_errno("unable to mmap %lu bytes anonymously",
> +			  (unsigned long)size);
> +
> +	block->file = file;
> +	block->line = line;
> +	block->size = size;
> +	return block->buf;
> +}
> +

Double eek. A refname is ordinarily way too small than a page and we spend
a full page every time resolve_ref_unsafe() is called. That is acceptable
only for debugging build, but then having to patch the main codepath like
the following, renaming the "real" implementation of a rather important
function is not acceptable in a non-debugging build.

> diff --git a/refs.c b/refs.c
> index 8ffb32f..cf8dfcc 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -497,12 +497,21 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)
>  	return -1;
>  }
>  
> -const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)
> +const char *resolve_ref_real(const char *ref, unsigned char *sha1,
> +			     int reading, int *flag, const char *file, int line)
>  {
>  	int depth = MAXDEPTH;
>  	ssize_t len;
>  	char buffer[256];
> -	static char ref_buffer[256];
> +	static char real_ref_buffer[256];
> +	static char *ref_buffer;
> +
> +	if (!ref_buffer && !getenv("GIT_DEBUG_MEMCHECK"))
> +		ref_buffer = real_ref_buffer;
> +	if (ref_buffer != real_ref_buffer) {
> +		xfree_mmap(ref_buffer);
> +		ref_buffer = xmalloc_mmap(256, file, line);
> +	}

I'll drop 3/4 from the series, adjust 4/4, and queue the result as a
three-patch series for now.

Thanks.