Segmentation fault git read-tree

i pushed the branches necessary to reproduce this right before it happened, so you should be able to pull them and try it yourself.

thanks,
-Len

[lenb@t61 acpi (test)]$ git push lenb [email protected]'s password:
Counting objects: 723, done.
Compressing objects: 100% (237/237), done.
Writing objects: 100% (492/492), 82.22 KiB, done.
Total 492 (delta 389), reused 337 (delta 249)
To master.kernel.org:/pub/scm/linux/kernel/git/lenb/linux-acpi-2.6.git

   93d7446..dba92d3  linus -> linus
   93d7446..dba92d3  release -> release
   93d7446..c68a500  suspend -> suspend
   93d7446..5dceb6d  test -> test

[lenb@t61 acpi (test)]$ git merge test suspend
Already up-to-date with 5dceb6d3377c3cbd06f7b3282fec0e201273c302 Trying simple merge with c68a5009ab9938af22af668e0e2d646d2482c866
/home/lenb/bin/git-merge-octopus: line 52: 24287 Segmentation fault git read-tree -u -m --aggressive $common $MRT $SHA1
Merge with strategy octopus failed.
[lenb@t61 acpi (test)]$ git merge test suspend
fatal: unable to create '.git/index.lock': File exists Already up-to-date with 5dceb6d3377c3cbd06f7b3282fec0e201273c302 Trying simple merge with c68a5009ab9938af22af668e0e2d646d2482c866
fatal: unable to create '.git/index.lock': File exists Merge with strategy octopus failed.
[lenb@t61 acpi (test)]$ git --version
git version 1.5.4.4.537.gb75aa
[lenb@t61 acpi (test)]$

Ok, I can reproduce this, I'm on it like a maggot on a two-week-dead baby donkey.

Linus

Ooh, interesting. Compiling with debugging makes the SIGSEGV go away, and replaces it with an endless loop.

The SIGSEGV when non-debugging seems to be due to a corrupt "info->prev" pointer chain, but this was less obvious than I hoped it would be.

Still looking.

Linus

Ok, that was embarrassing.

This should fix it. Spot the stupid stack corruption..

Linus

---
 unpack-trees.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index be89d52..b62b054 100644
@@ -8,6 +8,8 @@
 #include "progress.h"
 #include "refs.h"
 
+#define MAX_UNPACK_TREES 4
+
 static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 	unsigned int set, unsigned int clear)
 {

@@ -123,7 +125,7 @@ static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_option
 int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long df_conflicts, struct name_entry *names, struct traverse_info *info)
 {
 	int i;
-	struct tree_desc t[3];
+	struct tree_desc t[MAX_UNPACK_TREES];
 	struct traverse_info newinfo;
 	struct name_entry *p;
 

@@ -327,7 +329,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 {
 	static struct cache_entry *dfc;
 
-	if (len > 4)
+	if (len > MAX_UNPACK_TREES)
 		die("unpack_trees takes at most four trees");
 	memset(&state, 0, sizeof(state));
 	state.base_dir = "";

Thanks for the fix.

Let's do this right, as the two limits should be in sync.

-- >8 --
read-tree() and unpack_trees(): use consistent limit

read-tree -m can read up to MAX_TREES, which was arbitrarily set to 8 since August 2007 (4 is needed to deal with 2 merge-base case).

However, the updated unpack_trees() code had an advertised limit of 4 (which it enforced). In reality the code was prepared to take only 3 trees and giving 4 caused it to stomp on its stack. Rename the MAX_TREES constant to MAX_UNPACK_TREES, move it to the unpack-trees.h common header file, and use it from both places to avoid future confusion.

---
 builtin-read-tree.c |    9 ++++-----
 unpack-trees.c      |    6 +++---
 unpack-trees.h      |    2 ++
 3 files changed, 9 insertions(+), 8 deletions(-)

index 160456d..e9cfd2b 100644
@@ -13,16 +13,15 @@
 #include "dir.h"
 #include "builtin.h"
 
-#define MAX_TREES 8
 static int nr_trees;
-static struct tree *trees[MAX_TREES];
+static struct tree *trees[MAX_UNPACK_TREES];
 
 static int list_tree(unsigned char *sha1)
 {
 	struct tree *tree;
 
-	if (nr_trees >= MAX_TREES)
-		die("I cannot read more than %d trees", MAX_TREES);
+	if (nr_trees >= MAX_UNPACK_TREES)
+		die("I cannot read more than %d trees", MAX_UNPACK_TREES);
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		return -1;

@@ -97,7 +96,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 {
 	int i, newfd, stage = 0;
 	unsigned char sha1[20];
-	struct tree_desc t[MAX_TREES];
+	struct tree_desc t[MAX_UNPACK_TREES];
 	struct unpack_trees_options opts;
 
 	memset(&opts, 0, sizeof(opts));

index be89d52..91649f3 100644
@@ -123,7 +123,7 @@ static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_option
 int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long df_conflicts, struct name_entry *names, struct traverse_info *info)
 {
 	int i;
-	struct tree_desc t[3];
+	struct tree_desc t[MAX_UNPACK_TREES];
 	struct traverse_info newinfo;
 	struct name_entry *p;
 

@@ -327,8 +327,8 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 {
 	static struct cache_entry *dfc;
 
-	if (len > 4)
-		die("unpack_trees takes at most four trees");
+	if (len > MAX_UNPACK_TREES)
+		die("unpack_trees takes at most %d trees", MAX_UNPACK_TREES);
 	memset(&state, 0, sizeof(state));
 	state.base_dir = "";
 	state.force = 1;

index e8abbcd..50453ed 100644
@@ -1,6 +1,8 @@
 #ifndef UNPACK_TREES_H
 #define UNPACK_TREES_H
 
+#define MAX_UNPACK_TREES 8
+
 struct unpack_trees_options;
 
 typedef int (*merge_fn_t)(struct cache_entry **src,

confirmed fixed.

thanks guys, for the great support!

I'm pleased that I can (almost fearlessly) update git on a daily basis. On the rare occasion I run into issues, you kill 'em quick.

-Len

[lenb@t61 acpi (test)]$ git merge test suspend
Already up-to-date with 5dceb6d3377c3cbd06f7b3282fec0e201273c302 Trying simple merge with c68a5009ab9938af22af668e0e2d646d2482c866 Merge made by octopus.

 arch/frv/kernel/pm.c            |    8 --
 arch/mips/au1000/common/power.c |   35 +-------
 arch/x86/kernel/apm_32.c        |   15 ---
 kernel/power/Kconfig            |   10 --
 kernel/power/Makefile           |    1 -
 kernel/power/pm.c               |  205 ---------------------------------------
 6 files changed, 1 insertions(+), 273 deletions(-)
 delete mode 100644 kernel/power/pm.c

[lenb@t61 acpi (test)]$ git version
git version 1.5.4.4.551.g1658

Yeah. I think we don't actually have any test for more than three trees. If we really are supposed to do eight trees, maybe we should test for it.

Linus

Again?