Re: [PATCH v3] receive-pack: optionally deny case clone refs

From: David Turner <dturner@twopensource.com>
Date: Jun 6, 2014, 03:01 UTC
Message-ID: <1402023675.18134.192.camel@stross>
In-Reply-To: <CACsJy8ALeYR1+5ox2GE2Rz3TRM5wqd9fxSCAnHoabo9fmuSdXQ@mail.gmail.com>

On Fri, 2014-06-06 at 08:37 +0700, Duy Nguyen wrote:

Show 13 quoted lines

> On Fri, Jun 6, 2014 at 7:52 AM, David Turner <dturner@twopensource.com> wrote:
> > Create the option receive.denycaseclonerefs, which checks pushed
> > refs to ensure that they are not case clones of an existing
> > ref.  This setting is turned on by default if core.ignorecase is
> > set, but not otherwise.
> 
> Just thinking out loud as I haven't had time to read this patch
> carefully, but I wonder if there's a potential performance problem
> with ref_is_denied_case_clone(). What if the receiver repo has 60k
> refs? Should we use name-hash (or a variant of it)? What if the sender
> pushes 60k refs in one go, will ref_is_denied_case_clone check against
> existing refs only, or it will cover more and more refs from the 60k
> input?

Good news! The sender can't push 60k refs in one go because that would require a command-line bigger than ARG_MAX. (Well, OK, this is not really good news, since of course someone could hack around this, and since this is minor bug in git that we should probably fix).

I guess this does present a bit of a DOS attack. When I discussed the previous version of this patch with Junio, I did not think it was a huge problem, since the the ordinary number of refs pushed is small. But when I think of it as a DOS, it sounds much worse.

I don't like the idea of creating a hashmap just for this, because really we need to be storing packed refs in an better on-disk format -- one that allows O(log n) or better access (as has been discussed).

I'll give this some more thought and see if I can come up with a better solution.