Re: [PATCH v3 4/4] commit: rewrite read_graft_line

From: Jeff King <peff@peff.net>
Date: Aug 18, 2017, 11:45 UTC
Message-ID: <20170818114530.hiks5iiljtxeyrha@sigill.intra.peff.net>
In-Reply-To: <CAJfL8+SHSAhgrMY6ONVHLMWEHcT0mhm4oKMmq6D=89SErDKiMA@mail.gmail.com>

On Fri, Aug 18, 2017 at 01:30:23PM +0200, Patryk Obara wrote:

Show 12 quoted lines

> > We'd reject such an input totally (though as an interesting side effect,
> > you can convince the parser to allocate 20x as much RAM as you send it;
> > one oid for each space).
> 
> Grafts are not populated during clone operation, so it really would be user
> making his life miserable. I could allocate FLEXI_ARRAY of size
> min(n, line->len / (GIT_*MIN*_HEXSZ+1)) instead… but I think it's not even
> worth the cost of making the code more complicated (and I don't want
> to reintroduce these size macros in here.
> 
> We _could_ put an artificial limit on graft parents, though (e.g. 10) and
> display an error message urging the user to stop using grafts?

Yeah, sorry, I should have made more clear that this is fine. I always try to read parsing code with my paranoid hat on, but I agree that grafts aren't really exposed to untrusted entities.

In general I'd prefer to avoid artificial limits unless there's a need for them. There are already spots (like receive-pack) where you can ask Git to store bytes in RAM as fast as you can send them. What I found interesting about this one was the 20:1 amplification. :)

> Before sending v3 I tried two other alternative implementations (perhaps I
> should've listed them in the v3 cover letter):

It might even be worth listing them in the commit message. Somebody finding your commit 3 years from via "git log -S" or "git blame" might say "yes, but why didn't they just do it like...". You can respond to them preemptively. :)

-Peff