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

From: Nicolas Morey-Chaisemartin <devel-git@morey-chaisemartin.com>
Date: Aug 31, 2011, 06:32 UTC
Message-ID: <4E5DD592.5090608@morey-chaisemartin.com>
In-Reply-To: <20110831015936.GB2519@sigill.intra.peff.net>

On 08/31/2011 03:59 AM, Jeff King wrote:

Show 5 quoted lines

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

After some consideration I think they do. Grep threads definitly get the try_to_free function registered while the main one does at least some xreallock out of sha1_file.c (so not owning the lock). I guess it requires the same kind of lock pack-objects.c uses (meaning we need to set the try_to_free function in grep.c too).

Show 12 quoted lines

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

I'm not expert enough in git pathways to be sure about pack_objects but I agree it looks "risky". Maybe add_packed_git should check whether there already is a free routine (other than do_nothing) instead of simply setting it up the first time.

Nicolas Morey-Chaisemartin