On Fri, Apr 20, 2018 at 5:23 AM, SZEDER Gábor <szeder.dev@xxxxxxxxx> wrote:
> 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?
