Re: [PATCH] git-fast-import possible memory corruption problem

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

Junio C Hamano <gitster@pobox.com> writes:

>> 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.
>
> Thanks -- do you mean your reproducible crash does not reproduce with the
> patch anymore?
>
> I think your change to move the "round up" logic up in the codepath makes
> perfect sense.  But your patch seems to conflate totally unrelated change
> to move memzero from the caller to callee into it, and I do not see the
> reason why it should be that way.  If the caller asked 10 bytes to calloc
> from the pool, and the underlying pool allocator gives you a 16-byte
> block, you only have to guarantee that the first 10 bytes are cleared, and
> can leave the trailing padding 6 bytes at the end untouched.

That is, something like this...

 fast-import.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git c/fast-import.c w/fast-import.c
index 3c035a5..3276d5d 100644
--- c/fast-import.c
+++ w/fast-import.c
@@ -554,6 +554,10 @@ static void *pool_alloc(size_t len)
 	struct mem_pool *p;
 	void *r;
 
+	/* round up 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;
@@ -572,9 +576,6 @@ 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;
 	return r;
 }