From d377a885a98d4a6281b3edd0aa6a0c610cf5e1a6 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 29 Apr 2017 08:27:41 -0700 Subject: [PATCH] Fix stack overflow in MergeBaseGenerator Some repository topologies can cause carryOntoHistory to overflow the thread stack, due to its strategy of recursing into the 2nd+ parents of a merge commit. This can easily happen if a project maintains a local fork, and frequently pulls from the upstream repository, which itself may have a branchy history. Rewrite the carryOntoHistory algorithm to use a fixed amount of thread stack, pushing the save points onto the heap. By using heap space the thread stack depth is no longer a concern. Repositories are instead limited by available memory. The algorithm is now structured as two loops: carryOntoHistory: This outer loop pops saved commits off the top of the stack, allowing the inner loop algorithm to dive down that path and carry bits onto commits along that part of the graph. The loop ends when there are no more stack elements. carryOntoHistoryInner: The inner loop walks along a single path of the graph. For a string of pearls (commits with one parent each) r <- s <- t <- u the algorithm walks backwards from u to r by iteratively updating its local variable 'c'. This avoids heap allocation along a simple path that does not require remembering state. The inner loop breaks in the HAVE_ALL case, when all bits have been found to be previously set on the commit. This occurs when a prior iteration of the outer loop (carryOntoHistory) explored a different path to this same commit, and copied the bits onto it. When the inner loop encounters a merge commit, it pushes all parents onto the heap based stack by allocating individual CarryStack elements for each parent. Parents are pushed in order, allowing side branches to be explored first. A small optimization is taken for the last parent, avoiding pushing it and instead updating 'c', allowing the side branch to be entered without allocating a CarryStack. Change-Id: Ib7b67d90f141c497fbdc61a31b0caa832e4b3c04 --- .../jgit/revwalk/MergeBaseGenerator.java | 90 ++++++++++++------- 1 file changed, 59 insertions(+), 31 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/MergeBaseGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/MergeBaseGenerator.java index 5d7e72dd2..73ce9854e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/MergeBaseGenerator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/MergeBaseGenerator.java @@ -69,26 +69,21 @@ import org.eclipse.jgit.internal.JGitText; */ class MergeBaseGenerator extends Generator { private static final int PARSED = RevWalk.PARSED; - private static final int IN_PENDING = RevWalk.SEEN; - private static final int POPPED = RevWalk.TEMP_MARK; - private static final int MERGE_BASE = RevWalk.REWRITE; private final RevWalk walker; - private final DateRevQueue pending; private int branchMask; - private int recarryTest; - private int recarryMask; - private int mergeBaseAncestor = -1; private LinkedList ret = new LinkedList<>(); + private CarryStack stack; + MergeBaseGenerator(final RevWalk w) { walker = w; pending = new DateRevQueue(); @@ -202,29 +197,56 @@ class MergeBaseGenerator extends Generator { return null; } - private void carryOntoHistory(RevCommit c, final int carry) { + private void carryOntoHistory(RevCommit c, int carry) { + stack = null; for (;;) { - final RevCommit[] pList = c.parents; - if (pList == null) - return; - final int n = pList.length; - if (n == 0) - return; - - for (int i = 1; i < n; i++) { - final RevCommit p = pList[i]; - if (!carryOntoOne(p, carry)) - carryOntoHistory(p, carry); + carryOntoHistoryInnerLoop(c, carry); + if (stack == null) { + break; + } + c = stack.c; + carry = stack.carry; + stack = stack.prev; + } + } + + private void carryOntoHistoryInnerLoop(RevCommit c, int carry) { + for (;;) { + RevCommit[] parents = c.parents; + if (parents == null || parents.length == 0) { + break; } - c = pList[0]; - if (carryOntoOne(c, carry)) + int e = parents.length - 1; + for (int i = 0; i < e; i++) { + RevCommit p = parents[i]; + if (carryOntoOne(p, carry) == CONTINUE) { + // Walking p will be required, buffer p on stack. + stack = new CarryStack(stack, p, carry); + } + // For other results from carryOntoOne: + // HAVE_ALL: p has all bits, do nothing to skip that path. + // CONTINUE_ON_STACK: callee pushed StackElement for p. + } + + c = parents[e]; + if (carryOntoOne(c, carry) != CONTINUE) { break; + } } } - private boolean carryOntoOne(final RevCommit p, final int carry) { - final boolean haveAll = (p.flags & carry) == carry; + private static final int CONTINUE = 0; + private static final int HAVE_ALL = 1; + private static final int CONTINUE_ON_STACK = 2; + + private int carryOntoOne(RevCommit p, int carry) { + // If we already had all carried flags, our parents do too. + // Return HAVE_ALL to stop caller from running down this leg + // of the revision graph any further. + // + // Otherwise return CONTINUE to ask the caller to walk history. + int rc = (p.flags & carry) == carry ? HAVE_ALL : CONTINUE; p.flags |= carry; if ((p.flags & recarryMask) == recarryTest) { @@ -232,17 +254,23 @@ class MergeBaseGenerator extends Generator { // voted to be one. Inject ourselves back at the front of the // pending queue and tell all of our ancestors they are within // the merge base now. - // p.flags &= ~POPPED; pending.add(p); - carryOntoHistory(p, branchMask | MERGE_BASE); - return true; + stack = new CarryStack(stack, p, branchMask | MERGE_BASE); + return CONTINUE_ON_STACK; } + return rc; + } - // If we already had all carried flags, our parents do too. - // Return true to stop the caller from running down this leg - // of the revision graph any further. - // - return haveAll; + private static class CarryStack { + final CarryStack prev; + final RevCommit c; + final int carry; + + CarryStack(CarryStack prev, RevCommit c, int carry) { + this.prev = prev; + this.c = c; + this.carry = carry; + } } }