From f716ad6d54ac74f9445b869f3fdd2b46256d1259 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Thu, 13 Mar 2014 12:45:57 -0700 Subject: [PATCH] Fix StackOverflowError in RevCommit.carryFlags on deep side graphs Copying flags through a graph with deep side branches can cause StackOverflowError. The recursive step to visit the 2nd parent of a merge commit can overflow the stack if these are themselves very deep histories with many branches. Rewrite the loop to iterate up to 500 recursive steps deep before unwinding the stack and running the remaining parts of the graph using a dynamically allocated FIFORevQueue. This approach still allows simple graphs that mostly merge short lived topic branches into master to copy flags with no dynamic memory allocation, relying only on temporary stack extensions. Larger more complex graphs only pay the allocation penalities if copying has to extend outwards "very far" in the graph, which is unlikely for many coloring based algorithms. Change-Id: I1882e6832c916e27dd5f6b7602d9caf66fb39c84 --- .../org/eclipse/jgit/revwalk/RevCommit.java | 79 +++++++++++++++---- 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java index 93d28aaf5..fac4f809f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/RevCommit.java @@ -63,6 +63,8 @@ import org.eclipse.jgit.util.StringUtils; /** A commit reference to a commit in the DAG. */ public class RevCommit extends RevObject { + private static final int STACK_DEPTH = 500; + /** * Parse a commit from its canonical format. * @@ -213,27 +215,72 @@ public class RevCommit extends RevObject { return Constants.OBJ_COMMIT; } - static void carryFlags(RevCommit c, final int carry) { - 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 ((p.flags & carry) == carry) - continue; - p.flags |= carry; - carryFlags(p, carry); + static void carryFlags(RevCommit c, int carry) { + FIFORevQueue q = carryFlags1(c, carry, 0); + if (q != null) + slowCarryFlags(q, carry); + } + + private static FIFORevQueue carryFlags1(RevCommit c, int carry, int depth) { + for(;;) { + RevCommit[] pList = c.parents; + if (pList == null || pList.length == 0) + return null; + if (pList.length != 1) { + if (depth == STACK_DEPTH) + return defer(c); + for (int i = 1; i < pList.length; i++) { + RevCommit p = pList[i]; + if ((p.flags & carry) == carry) + continue; + p.flags |= carry; + FIFORevQueue q = carryFlags1(c, carry, depth + 1); + if (q != null) + return defer(q, carry, pList, i + 1); + } } c = pList[0]; if ((c.flags & carry) == carry) - return; + return null; + c.flags |= carry; + } + } + + private static FIFORevQueue defer(RevCommit c) { + FIFORevQueue q = new FIFORevQueue(); + q.add(c); + return q; + } + + private static FIFORevQueue defer(FIFORevQueue q, int carry, + RevCommit[] pList, int i) { + // In normal case the caller will run pList[0] in a tail recursive + // fashion by updating the variable. However the caller is unwinding + // the stack and will skip that pList[0] execution step. + carryOneStep(q, carry, pList[0]); + + // Remaining parents (if any) need to have flags checked and be + // enqueued if they have ancestors. + for (; i < pList.length; i++) + carryOneStep(q, carry, pList[i]); + return q; + } + + private static void slowCarryFlags(FIFORevQueue q, int carry) { + // Commits in q have non-null parent arrays and have set all + // flags in carry. This loop finishes copying over the graph. + for (RevCommit c; (c = q.next()) != null;) { + for (RevCommit p : c.parents) + carryOneStep(q, carry, p); + } + } + + private static void carryOneStep(FIFORevQueue q, int carry, RevCommit c) { + if ((c.flags & carry) != carry) { c.flags |= carry; + if (c.parents != null) + q.add(c); } }