Re: [PATCH v10 33/36] merge-recursive: fix was_tracked() to quit lying with some renamed paths

From: Elijah Newren <newren@gmail.com>
Date: Apr 20, 2018, 15:23 UTC
Message-ID: <CABPp-BHZ-agTY77iqe9BxLC2ijrcKO+UPk83Bn+0cckA3fJFYg@mail.gmail.com>
In-Reply-To: <20180420122355.21416-1-szeder.dev@gmail.com>

On Fri, Apr 20, 2018 at 5:23 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

Show 9 quoted lines

> This patch causes memory corruption when the split index feature is in
> use, making several tests fail.  Now, while the split index feature
> sure has its own set of problems, AFAIK those are not that bad to
> cause memory corruption, they "only" tend to cause transient test
> failures due to a variant of the classic racy git issue [1].
>
> Here is a test failure:
>
>   $ GIT_TEST_SPLIT_INDEX=DareISayYes ./t3030-merge-recursive.sh

Running under valgrind shows that merge-recursive.c's add_cacheinfo (which calls add_cache_entry()) results in data used by o->orig_index getting free()'d. That means that anything trying to use that memory (whether a later call to discard_index, or just a call to was_dirty() or was_tracked()) will be access'ing free'd memory. (The exact same tests run valgrind clean when GIT_TEST_SPLIT_INDEX is not turned on.)

The fact that add_cacheinfo() frees data used by o->orig_index surprises me. add_cacheinfo is only supposed to modify the_index. Are o->orig_index and the_index sharing data somehow? Did I do something wrong or incomplete for the split index case when swapping indexes? My swapping logic, as shown in this patch was:

    /*
     * Update the_index to match the new results, AFTER saving a copy
     * in o->orig_index.  Update src_index to point to the saved copy.
     * (verify_uptodate() checks src_index, and the original index is
     * the one that had the necessary modification timestamps.)
     */
    o->orig_index = the_index;
    the_index = tmp_index;
    o->unpack_opts.src_index = &o->orig_index;

Do I need to do more?