Re: [PATCH] cherry: cache patch-ids to avoid repeating work

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 11, 2008, 06:54 UTC
Message-ID: <7v3amglxmb.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7f9d599f0807100733s4435a9bga89749f2f6e10cf@mail.gmail.com>

"Geoffrey Irving" <irving@naml.us> writes:

Show 9 quoted lines

>>> Oops: avoiding the infinite loop only requires reading expected O(1)
>>> entries on load, so I can fix that if you like.  It would only be all of
>>> them if it actually did detect the infinite loop.
>>
>> I have to admit that you lost me there.  AFAIR the patch-id cache is a
>> simple commit->patch_id store, right?  Then there should be no way to get
>> an infinite loop.
>
> If every entry is nonnull, find_helper loops forever.

Isn't it sufficient to make this part check the condition as well?

+ if (cache->count >= cache->size) + { + warning("%s is corrupt: count %"PRIu32" >= size %"PRIu32, + filename, cache->count, cache->size); + goto empty; + }

At runtime you keep the invariants that hashtable always has at most 3/4 full and whoever wrote the file you are reading must have honored that as well, or there is something fishy going on.

Show 5 quoted lines

>> Besides, this is a purely local cache, no?  Never to be transmitted...  So
>> not much chance of a malicious attack, except if you allow write access to
>> your local repository, in which case you are endangered no matter what.
>
> Yep, that's why it's only a hole in quotes, and why I didn't fix it.

You might want to protect yourself against file corruption, though. Checksumming the whole file and checking it at opening defeats the point of mmap'ed access, but at least the header may want to be checksummed?