Re: [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects

From: Johan Herland <johan@herland.net>
Date: May 14, 2011, 13:17 UTC
Message-ID: <201105141517.45324.johan@herland.net>
In-Reply-To: <BANLkTik_taBK+=nh+0WEUjp3AV_fC7e_dg@mail.gmail.com>

On Saturday 14 May 2011, Shawn Pearce wrote:

Show 5 quoted lines

> I wonder... should we instead export the objectCountLimit as part of
> the advertisement to the client, and teach send-pack to look at this
> and pass it down to pack-objects? If pack-objects winds up with more
> than this limit, it aborts and the client doesn't even transmit data.
> Newer clients would abort cleanly with a nice error.

Good idea (although it grows the scope from the quick-fix I initially intended it to be...)

I'm planning to add a new capability collection/namespace, called "limit-*", where the server can communicate capabilities to the client, like so:

  limit-object-count_100000
  limit-commit-count_1000
  limit-pack-size_500000000

(I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a capability may not contain "=" or ":")

However, you say:

> For older clients that don't know this new advertised capability, they
> should fail hard and not transfer all of this data.

AFAICS this is not the case. If a client does not understand a capability, it simply ignores it, and carries on doing its usual thing.

IINM there are only two ways to prevent an older client from transferring all the data:

1. Change the pack protocol in an incompatible way, that causes older client 
to abort with a pack format error prior to transmitting the pack.

2. (as in initial patch) Abort receive-pack when the server detects a limit 
violation, leaving the client with a broken pipe. I haven't read the pack 
protocol closely, but I wouldn't be surprised if this behavior is strictly 
in violation of the protocol.

Show 6 quoted lines

> In my experience
> when a user gets these strange errors from his Git client, he contacts
> his server administrator with the screen output. At which point the
> administrator can see the Counting objects line, check the repository
> configuration, and tell the user what the problem is... and encourage
> them to upgrade their client to a newer version.

Hmm... Not ideal, but I guess we can live with that. At least we should warn the server administrator of this in the documentation of the config variable(s).

Otherwise, I agree with everything you wrote.

Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net