Re: [PATCH] sparse-index: copy dir_hash in ensure_full_index()

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Aug 23, 2021, 18:18 UTC
Message-ID: <8058589d-f84f-2fdc-6057-8242cde9df61@jeffhostetler.com>
In-Reply-To: <87h7fgfdah.fsf@evledraar.gmail.com>

On 8/23/21 10:25 AM, Ævar Arnfjörð Bjarmason wrote:

Show 26 quoted lines

> 
> On Mon, Aug 16 2021, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Copy the 'index_state->dir_hash' back to the real istate after expanding
>> a sparse index.
>>
>> A crash was observed in 'git status' during some hashmap lookups with
>> corrupted hashmap entries.  During an index expansion, new cache-entries
>> are added to the 'index_state->name_hash' and the 'dir_hash' in a
>> temporary 'index_state' variable 'full'.  However, only the 'name_hash'
>> hashmap from this temp variable was copied back into the real 'istate'
>> variable.  The original copy of the 'dir_hash' was incorrectly
>> preserved.  If the table in the 'full->dir_hash' hashmap were realloced,
>> the stale version (in 'istate') would be corrupted.
>>
>> The test suite does not operate on index sizes sufficiently large to
>> trigger this reallocation, so they do not cover this behavior.
>> Increasing the test suite to cover such scale is fragile and likely
>> wasteful.
> 
> How large does the index need to be to trigger this? I don't know if a
> test here is useful, but FWIW if we had such a test then the EXPENSIVE
> prereq + GIT_TEST_LONG=true might be a good fit for it.
>

There would need to be enough directories in the repo to cause the dir_hash to grow during an insert into the dir_hash. IIRC the hashmap table is initialized to 64 and auto reallocs when it hits 80% capacity, so somewhere in the area of 50 directories at minimum.

Whether the error is observed would also depend on free() trashing the contents of the memory and/or whether the memory was recycled by something else.

Jeff