From: Derrick Stolee Date: Fri, 13 Oct 2017 13:39:14 GMT Subject: Re: git-clone causes out of memory Message-ID: In-Reply-To: On 10/13/2017 9:15 AM, Derrick Stolee wrote: > On 10/13/2017 8:44 AM, Jeff King wrote: >> On Fri, Oct 13, 2017 at 03:12:43PM +0300, Constantine wrote: >> >>> On 13.10.2017 15:04, Junio C Hamano wrote: >>>> Christian Couder writes: >>>> >>>>> Yeah, but perhaps Git could be smarter when rev-listing too and avoid >>>>> processing files or directories it has already seen? >>>> Aren't you suggesting to optimize for a wrong case? >>>> >>> Anything that is possible with a software should be considered as a >>> possible >>> usecase. It's in fact a DoS attack. Imagine there's a server that >>> using git >>> to process something, and now there's a way to knock down this >>> server. It's >>> also bad from a promotional stand point. >> But the point is that you'd have the same problem with any repository >> that had 10^7 files in it. Yes, it's convenient for the attacker that >> there are only 9 objects, but fundamentally it's pretty easy for an >> attacker to construct repositories that have large trees (or very deep >> trees -- that's what causes stack exhaustion in some cases). >> >> Note too that this attack almost always comes down to the diff code >> (which is why it kicks in for pathspec limiting) which has to actually >> expand the tree. Most "normal" server-side operations (like accepting >> pushes or serving fetches) operate only on the object graph and _do_ >> avoid processing already-seen objects. >> >> As soon as servers start trying to checkout or diff, though, the attack >> surface gets quite large. And you really need to start thinking about >> having resource limits and quotas for CPU and memory use of each process >> (and group by requesting user, IP, repo, etc). >> >> I think the main thing Git could be doing here is to limit the size of >> the tree (both width and depth). But arbitrary limits like that have a >> way of being annoying, and I think it just pushes resource-exhaustion >> attacks off a little (e.g., can you construct a blob that behaves badly >> with the "--patch"?). >> >> -Peff > > I'm particularly interested in why `git rev-list HEAD -- [path]` gets > slower in this case, because I wrote the history algorithm used by > VSTS. In our algorithm, we only walk the list of objects from commit > to the tree containing the path item. For example, in the path > d0/d0/d0, we would only walk: > >     commit --root--> tree --d0--> tree --d0--> tree [parse oid for d0 > entry] > > From this, we can determine the TREESAME relationship by parsing four > objects without parsing all contents below d0/d0/d0. > > The reason we have exponential behavior in `git rev-list` is because > we are calling diff_tree_oid() in tree-diff.c recursively without > short-circuiting on equal OIDs. > > I will prepare a patch that adds this OID-equal short-circuit to avoid > this exponential behavior. I'll model my patch against a similar patch > in master: > >     commit d12a8cf0af18804c2000efc7a0224da631e04cd1 unpack-trees: > avoid duplicate ODB lookups during checkout > > It will also significantly speed up rev-list calls for short paths in > deep repositories. It will not be very measurable in the git or Linux > repos because their shallow folder structure. > > Thanks, > -Stolee Since I don't understand enough about the consumers to diff_tree_oid() (and the fact that the recursive behavior may be wanted in some cases), I think we can fix this in builtin/rev-list.c with this simple diff: --- diff --git a/builtin/rev-list.c b/builtin/rev-list.c index ded1577424..b2e8e02cc8 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -285,6 +285,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)         git_config(git_default_config, NULL);         init_revisions(&revs, prefix); + +       revs.pruning.flags = revs.pruning.flags & ~DIFF_OPT_RECURSIVE; +         revs.abbrev = DEFAULT_ABBREV;         revs.commit_format = CMIT_FMT_UNSPECIFIED;         argc = setup_revisions(argc, argv, &revs, NULL);