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

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?

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

[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