[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] grep: Fix race condition in delta_base_cache
From:       Nicolas Morey-Chaisemartin <devel-git () morey-chaisemartin ! com>
Date:       2011-08-31 19:13:28
Message-ID: 4E5E87D8.3080108 () morey-chaisemartin ! com
[Download RAW message or body]

On 08/31/2011 03:59 AM, Jeff King wrote:
> 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.
> 

After some more looking around, I'd say the best way to fix this is provide a lock to \
the try_to_free function from sha1_file and provide access to this lock for \
pack-objects and grep to replace respectively the read_mutex and read_sha1_mutex. So \
we can simplify the problem by having a single lock to avoid all the cache/free \
issues (and reusable elsewhere if needed in the future), whether it's shared through \
direct access or API (I'm not sure what's git policy about that). And this way, there \
is no need to duplicate what pack-objects is achieving and it gives some peace of \
mind about the fact that the try_to _free function won't be overwritten in our backs.

Nicolas Morey-Chaisemartin

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]