Re: [PATCH 3/4] Guard memory overwriting in resolve_ref's static buffer
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 13, 2011, 00:37 UTC
- Message-ID
- <7v8vmhz4ql.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1323688832-32016-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 14 quoted lines
> 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.
Show 24 quoted lines
> 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.
Show 25 quoted lines
> 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.