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

From: Geoffrey Irving <irving@naml.us>
Date: Jul 10, 2008, 14:33 UTC
Message-ID: <7f9d599f0807100733s4435a9bga89749f2f6e10cf@mail.gmail.com>
In-Reply-To: <alpine.DEB.1.00.0807101526380.18205@racer>

On Thu, Jul 10, 2008 at 7:28 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 22 quoted lines

> Hi,
>
> On Thu, 10 Jul 2008, Geoffrey Irving wrote:
>
>> On Wed, Jul 9, 2008 at 8:34 PM, Geoffrey Irving <irving@naml.us> wrote:
>>
>> > Note: there are at least two "holes" in this code.  First, it is
>> > impossible to verify the validity of the entries (this is impossible
>> > to fix).  Second, it is possible to write a malicious patch-id-cache
>> > file that causes git-cherry to go into an infinite loop.  Fixing the
>> > loop requires either traversing every entry on load (bad) or adding a
>> > second loop termination condition to find_helper.  Since looping
>> > forever is better than returning incorrect results, I figured fixing
>> > the weaker hole would just result in a false sense of security.
>>
>> 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.

> 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.

Geoffrey