From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 23 Aug 2021 19:27:57 GMT
Subject: Re: [PATCH] sparse-index: copy dir_hash in ensure_full_index()
Message-ID: <2431b77d-0d10-0fd6-5ccd-8250db9f7998@gmail.com>
In-Reply-To: <8058589d-f84f-2fdc-6057-8242cde9df61@jeffhostetler.com>

On 8/23/2021 2:18 PM, Jeff Hostetler wrote:
> On 8/23/21 10:25 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> 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.

Further complicating this is that the sparse index size matters
relative to the full index size, since the dir_hash has a given
size going into ensure_full_index() that depends on the sparse-index
size and the reallocation patterns.
 
> 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.

Issues like this cause this to be hard to reproduce. We did not
catch it in the performance test p2000-sparse-operations.sh, which
aims to measure how the index works at scale.

This makes me incredibly pessimistic that such a test will be
effective at preventing a regression, let alone catching similar
issues in the future.

Thanks,
-Stolee