From: Jeff King <peff@peff.net>
Date: Wed, 31 Aug 2011 01:59:36 GMT
Subject: Re: [PATCH] grep: Fix race condition in delta_base_cache
Message-ID: <20110831015936.GB2519@sigill.intra.peff.net>
In-Reply-To: <4E5CE982.7080200@morey-chaisemartin.com>

On Tue, Aug 30, 2011 at 03:45:38PM +0200, Nicolas Morey-Chaisemartin wrote:

> According to gdb the problem originate from release_delta_cash (sha1_file.c:1703)
> 		free(ent->data);
> 
> From my analysis it seems that git grep threads do acquire lock before
> calling read_sha1_file but not before calling
> read_object_with_reference who ends up calling read_sha1_file too.

Yeah, I think this is necessary, and the patch looks good.

I notice there are some other code paths that end up in xmalloc without
locking, too (e.g., load_file, and some strbuf_* calls). Don't those
need locking, too, as malloc may try to release packfile memory?

builtin/pack-objects.c dealt with this already by setting a new
"try_to_free" routine that locks[1], which we should also do. It
probably comes up less frequently, because it only happens when we're
under memory pressure.

-Peff

[1] Actually, it looks like the "try_to_free" routine starts as nothing,
    and then add_packed_git sets it lazily to try_to_free_pack_memory.
    But what builtin/pack-objects tries to do is overwrite that with a
    version of try_to_free_pack_memory that does locking. So it's
    possible that we would not have read any packed objects while
    setting up the threads, and add_packed_git will overwrite our
    careful, locking version of try_to_free_pack_memory.

    I _think_ pack-objects is probably OK, because it will have already
    done the complete "counting objects" phase, which would look in any
    packs. But it may be harder for grep.