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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Thu, 10 Jul 2008, Geoffrey Irving wrote:

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

Ah, that is because you did not use that part of my implementation.  My 
hash did not wrap.

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

Then it is not a hole.

Ciao,
Dscho