Re: [PATCH] git-fast-import possible memory corruption problem
Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
>> 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;
}
Junio C Hamano <gitster@pobox.com>