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

[YONETANI Tomokazu <qhwt+git@les.ath.cx>]

On Sat, Dec 13, 2008 at 09:53:55PM -0800, Junio C Hamano wrote:
> 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
[snip]
Yes, that was what it originally looked like, but after fixing the crash
I encountered another problem, and thought that I had to clear rounded up
area too.  It turned out that the second problem has nothing to do with
[the pool allocator, but the data fed to git-fast-import.  After fixing
the data, the git-fast-import with above patch finished successfully,
no more crashes.