Re: [PATCH] Set hard limit on delta chain depth
- From
Shawn Pearce <spearce@spearce.org>
- Date
- Dec 6, 2011, 18:12 UTC
- Message-ID
- <CAJo=hJuy27Uagmubbv=RqAOMx03e6JBgZxObkjFLg9oG2x_UqA@mail.gmail.com>
- In-Reply-To
- <CACsJy8Btwn0=PGS+PJV-6DqYBmzOp7LTB2=R_kCx4SJHA2YDRw@mail.gmail.com>
On Tue, Dec 6, 2011 at 07:30, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> index-pack is called at server side as part of a push. So in theory > the sending side can generate very long delta chains and bring down > the server side.
It is also called at client side during fetch. So in theory the server can produce very long delta chains and take down a client.
>> Because if not, then the "creator" had better never be configured >> to use a chain depth that the "reader" cannot handle. > > Normal creators (i.e. C Git) use default depth 50 so we should be safe.
JGit is also a "normal creator", and it sometimes produces chains deeper than 50. Junio identified a 255 deep chain a week or two ago. Some people have repacked their repositories very aggressively with deeper chains when they are trying to optimize for space and don't access historical revisions very often. I doubt anyone has packed deeper than 120ish intentionally... but we shouldn't assume that in the code.
Show 10 quoted lines
>> This in turn >> imply that there should be a common limit that is supported by all git >> clients and is a documented part of the protocol. (Or the code has to >> be rewritten to use an explicit stack instead of recursion.) > > It's the implementation limitation, not the protocol. If the server > propagates error messages from index-pack back to client (I'm not > sure), then users can adjust --depth to be accepted by server. We > could negotiate the limit over the protocol but not sure we would want > to go that route.
What about clients fetching from a server? The client can't change the depth the server sends it.
Show 5 quoted lines
> The troubled code could be rewritten to avoid recursion. However long > delta chains may degrade performance, I'd rather have support to split > long chains (which can be done with --depth at client already) instead > of just recursion elimination. This patch is simpler, so it would be > easier to back port it if someone wants to do so.
JGit long ago changed its IndexPack routine to use a manually managed heap based stack instead of recursion, making it immune from overrunning the stack due to a long delta chain. That is probably the cleaner route. But you also have to fix sha1_file.c and its recursion based unpacking of a delta chain... again which JGit fixed a long time ago.