Re: [PATCH] Set hard limit on delta chain depth

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Dec 6, 2011, 15:30 UTC
Message-ID: <CACsJy8Btwn0=PGS+PJV-6DqYBmzOp7LTB2=R_kCx4SJHA2YDRw@mail.gmail.com>
In-Reply-To: <4EDE2C95.5040804@alum.mit.edu>

2011/12/6 Michael Haggerty <mhagger@alum.mit.edu>:

Show 21 quoted lines

> On 12/06/2011 01:17 PM, Erik Faye-Lund wrote:
>> 2011/12/5 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>>> Too deep delta chains can cause stack overflow in get_base_data(). Set
>>> a hard limit so that index-pack does not run out of stack. Also stop
>>> people from producing such a long delta chains using "pack-object
>>> --depth=<too large>"
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  I used to make very long delta chains and triggered this in index-pack.
>>>  I did not care reporting because it's my fault anyway. Think again,
>>>  index-pack is called at server side and a malicious client can
>>>  trigger this. This patch does not improve the situation much, but at
>>>  least we won't get sigsegv at server side.
>>
>> Wouldn't it make more sense to make the limit a config option rather
>> than a hard-coded value of 128 (which seems arbitrary to me)? After
>> all, different platforms have different stack-limitations...
>
> I'm confused: is the data only ever read by the same host that generated
> it?

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.

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

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

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.

-- 
Duy