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

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

YONETANI Tomokazu <qhwt+git@les.ath.cx> writes:

> 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.

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.