[PATCH] git-fast-import possible memory corruption problem
- From
- YONETANI Tomokazu <qhwt+git@les.ath.cx>
- Date
- Dec 14, 2008, 02:08 UTC
- Message-ID
- <20081214020822.GB4121@les.ath.cx>
Hello. While trying to convert NetBSD CVS repository to Git, I've been experiencing 100% reproducible crash of git-fast-import. After poking here and there and I noticed a dubious code fragment in pool_alloc(): :
r = p->next_free;
/* round out to a 'uintmax_t' alignment */
if (len & (sizeof(uintmax_t) - 1))
len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
p->next_free += len;
return r;As the `round out' takes place AFTER it found the room in the mem_pool, there's a small chance of p->next_free being set outside of the chosen area, up to (sizeof(uintmax_t) - 1) bytes. pool_strdup() is one of the functions which can trigger the problem, when pool_alloc() finds a room at the end of a pool entry and the requested length is not multiple of size(uintmax_t). I believe attached patch addresses this problem.
Best regards, YONETANI Tomokazu.
diff --git a/fast-import.c b/fast-import.c index 3c035a5..fb4367a 100644 --- a/fast-import.c +++ b/fast-import.c @@ -549,11 +549,15 @@ static unsigned int hc_str(const char *s, size_t len) return r; } -static void *pool_alloc(size_t len) +static void *_pool_alloc(size_t len, int zero) { struct mem_pool *p; void *r; + /* round out to a 'uintmax_t' alignment */ + if (len & (sizeof(uintmax_t) - 1)) + len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1)); + for (p = mem_pool; p; p = p->next_pool) if ((p->end - p->next_free >= len)) break; @@ -561,7 +565,8 @@ static void *pool_alloc(size_t len) if (!p) { if (len >= (mem_pool_alloc/2)) { total_allocd += len; - return xmalloc(len); + r = xmalloc(len); + goto out; } total_allocd += sizeof(struct mem_pool) + mem_pool_alloc; p = xmalloc(sizeof(struct mem_pool) + mem_pool_alloc); @@ -572,19 +577,22 @@ static void *pool_alloc(size_t len) } r = p->next_free; - /* round out to a 'uintmax_t' alignment */ - if (len & (sizeof(uintmax_t) - 1)) - len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1)); p->next_free += len; +out: + if (zero) + memset(r, 0, len); return r; } +static void *pool_alloc(size_t len) +{ + return _pool_alloc(len, 0); +} + static void *pool_calloc(size_t count, size_t size) { size_t len = count * size; - void *r = pool_alloc(len); - memset(r, 0, len); - return r; + return _pool_alloc(len, 1); } static char *pool_strdup(const char *s)