Re: [RFC PATCH v10 32.5/36] unpack_trees: fix memory corruption with split_index when src != dst

From: Elijah Newren <newren@gmail.com>
Date: Apr 23, 2018, 17:09 UTC
Message-ID: <CABPp-BFuuywXrJuzfb4V1bRLb9eVZpLKiCDwQWetHk8bymC6EQ@mail.gmail.com>
In-Reply-To: <CACsJy8D4OYphLG5vStr+M9qkiqyH_LR517M-JOPS2wJeTFNGRg@mail.gmail.com>

Hi,

On Sun, Apr 22, 2018 at 5:38 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 12 quoted lines

>>   - there's a better, more performant fix or there is some way to actually
>>     share a split_index between two independent index_state objects.
>
> A cleaner way of doing this would be something to the line [1]
>
>     move_index_extensions(&o->result, o->dst_index);
>
> near the end of this function. This could be where we compare the
> result index with the source index's shared file and see if it's worth
> keeping the shared index or not. Shared index is designed to work with
> huge index files though, any operations that go through all index
> entries will usually not be cheap. But at least it's safer.

Yeah, it looks like move_index_extensions() currently has no logic for the split_index. Adding it sounds to me like a patch series of its own, and I'm keen to limit additional changes since my patch series already broke things pretty badly once already.

Show 6 quoted lines

>> However, with this fix, all the tests pass both normally and under
>> GIT_TEST_SPLIT_INDEX=DareISayYes.  Without this patch, when
>> GIT_TEST_SPLIT_INDEX is set, my directory rename detection series will fail
>> several tests, as reported by SZEDER.
>
> Yes, the change looks good.

Great, thanks for looking over it.

Show 5 quoted lines

> [1] To me the second parameter should be src_index, not dst_index.
> We're copying entries from _source_ index to "result" and we should
> also copy extensions from the source index. That line happens to work
> only when dst_index is the same as src_index, which is the common use
> case so far.

That makes sense; this sounds like another fix that should be submitted. Did you want to submit a patch making that change? Do you want me to?

Elijah