Re: [PATCH] grep: Fix race condition in delta_base_cache

From: Jeff King <peff@peff.net>
Date: Aug 31, 2011, 01:59 UTC
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:

Show 6 quoted lines

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