From: Shawn Pearce Date: Tue, 06 Dec 2011 18:12:54 GMT Subject: Re: [PATCH] Set hard limit on delta chain depth Message-ID: In-Reply-To: On Tue, Dec 6, 2011 at 07:30, Nguyen Thai Ngoc Duy 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. >>  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. > 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.