Bug: git bus error (stack blowout)

Bug: git bus error (stack blowout)

[Venkatesh Srinivas]

Hi,

When using git 1.7.6.3 from NetBSD's pkgsrc, on this git tree:
http://gitweb.dragonflybsd.org/pkgsrcv2.git, I got a bus error when
switching from the pkgsrc-2011q3 branch to the master branch. I have a
core file and the git binary if it'd be helpful; it looks like
mark_parents_uninteresting() was called recursively entirely too many
times (>60,000), originally from prepare_revision_walk(), from
stat_tracking_info(), from format_tracking_info(),
update_revs_for_switch(), from cmd_checkout().

Thanks,
-- vs;

Re: Bug: git bus error (stack blowout)

[Nguyen Thai Ngoc Duy]

On Sun, Dec 04, 2011 at 05:50:19PM -0500, Venkatesh Srinivas wrote:
> Hi,
> 
> When using git 1.7.6.3 from NetBSD's pkgsrc, on this git tree:
> http://gitweb.dragonflybsd.org/pkgsrcv2.git, I got a bus error when
> switching from the pkgsrc-2011q3 branch to the master branch. I have a
> core file and the git binary if it'd be helpful; it looks like
> mark_parents_uninteresting() was called recursively entirely too many
> times (>60,000), originally from prepare_revision_walk(), from
> stat_tracking_info(), from format_tracking_info(),
> update_revs_for_switch(), from cmd_checkout().

This patch may fix it for you, although it'd be interesting to
understand how you get into this (I'm still cloning pkgsrcv2.git).

-- 8< --
Subject: [PATCH] Eliminate recursion in setting/clearing marks in commit list

Recursion in a DAG is generally a bad idea because it could be very
deep. Be defensive and avoid recursion in mark_parents_uninteresting()
and clear_commit_marks().

mark_parents_uninteresting() learns a trick from clear_commit_marks()
to avoid malloc() in (dorminant) single-parent case.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 commit.c   |   31 ++++++++++++++++++++-----------
 revision.c |   45 +++++++++++++++++++++++++++++----------------
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/commit.c b/commit.c
index 0775eec..cd19a69 100644
--- a/commit.c
+++ b/commit.c
@@ -423,22 +423,31 @@ struct commit *pop_most_recent_commit(struct commit_list **list,
 
 void clear_commit_marks(struct commit *commit, unsigned int mark)
 {
-	while (commit) {
-		struct commit_list *parents;
+	struct commit_list *list = NULL, *l;
+	commit_list_insert(commit, &list);
+	while (list) {
+		commit = list->item;
+		l = list;
+		list = list->next;
+		free(l);
 
-		if (!(mark & commit->object.flags))
-			return;
+		while (commit) {
+			struct commit_list *parents;
 
-		commit->object.flags &= ~mark;
+			if (!(mark & commit->object.flags))
+				break;
 
-		parents = commit->parents;
-		if (!parents)
-			return;
+			commit->object.flags &= ~mark;
+
+			parents = commit->parents;
+			if (!parents)
+				break;
 
-		while ((parents = parents->next))
-			clear_commit_marks(parents->item, mark);
+			while ((parents = parents->next))
+				commit_list_insert(parents->item, &list);
 
-		commit = commit->parents->item;
+			commit = commit->parents->item;
+		}
 	}
 }
 
diff --git a/revision.c b/revision.c
index 0aa3638..8d4069e 100644
--- a/revision.c
+++ b/revision.c
@@ -139,11 +139,32 @@ void mark_tree_uninteresting(struct tree *tree)
 
 void mark_parents_uninteresting(struct commit *commit)
 {
-	struct commit_list *parents = commit->parents;
+	struct commit_list *parents = NULL, *l;
+
+	for (l = commit->parents; l; l = l->next)
+		commit_list_insert(l->item, &parents);
 
 	while (parents) {
 		struct commit *commit = parents->item;
-		if (!(commit->object.flags & UNINTERESTING)) {
+		l = parents;
+		parents = parents->next;
+		free(l);
+
+		while (commit) {
+			/*
+			 * A missing commit is ok iff its parent is marked
+			 * uninteresting.
+			 *
+			 * We just mark such a thing parsed, so that when
+			 * it is popped next time around, we won't be trying
+			 * to parse it and get an error.
+			 */
+			if (!has_sha1_file(commit->object.sha1))
+				commit->object.parsed = 1;
+
+			if (commit->object.flags & UNINTERESTING)
+				break;
+
 			commit->object.flags |= UNINTERESTING;
 
 			/*
@@ -154,21 +175,13 @@ void mark_parents_uninteresting(struct commit *commit)
 			 * wasn't uninteresting), in which case we need
 			 * to mark its parents recursively too..
 			 */
-			if (commit->parents)
-				mark_parents_uninteresting(commit);
-		}
+			if (!commit->parents)
+				break;
 
-		/*
-		 * A missing commit is ok iff its parent is marked
-		 * uninteresting.
-		 *
-		 * We just mark such a thing parsed, so that when
-		 * it is popped next time around, we won't be trying
-		 * to parse it and get an error.
-		 */
-		if (!has_sha1_file(commit->object.sha1))
-			commit->object.parsed = 1;
-		parents = parents->next;
+			for (l = commit->parents->next; l; l = l->next)
+				commit_list_insert(l->item, &parents);
+			commit = commit->parents->item;
+		}
 	}
 }
 
-- 
1.7.8.36.g69ee2
-- 8< --
-- 
Duy