Re: Something weird is happening...

* Ingo Molnar [off-list ref] wrote:

a simple 'git fsck' still reports a ton of dangling and missing objects. 
(see it below)

amongst them is the object above:

  missing blob af0e01d4c663a101f48614e40d006ed6272d5c36

but this does not seem to cause problems with git version 1.6.0.6 that 
i'm using now.

hm - maybe it's unrelated, but yesterday, a few hours before the incident 
i did a higher-order Octopus merge with 1.6.1, that segfaulted.

I didnt think much of it - git often crashes with our crazy -tip tree when 
i get above the magic 20 branches limit. The crash left a .git/index.lock 
file around which i removed - and then forgot about the incident. I wanted 
to report those crashes before but procrastinated it.

I used git/maint snapshots because new versions of Git are much better at 
doing Octopus merges.

I've written a reproducer for git-1.6.1.1-1.fc11.i386. See the crash log 
below.

To reproduce, pick up the -tip tree as a remote:

   http://people.redhat.com/mingo/tip.git/README

(you can run the README file as a script)

Then do this:

   git checkout -b tmp.crash v2.6.29-rc3

   git merge  tip/x86/asm        tip/x86/cleanups     tip/x86/cpudetect    \
              tip/x86/debug      tip/x86/doc          tip/x86/header-fixes \
              tip/x86/mm         tip/x86/paravirt     tip/x86/pat          \
              tip/x86/setup-v2   tip/x86/subarch      tip/x86/uaccess      \
              tip/x86/urgent     tip/core/percpu

and you should see the segfault.

interestingly i did not reproduce with the sha1's hardcoded:

  git checkout -b tmp.crash 18e352e4a73465349711a9324767e1b2453383e2

 git merge 2d4d57db692ea790e185656516e6ebe8791f1788 a448720ca3248e8a7a426336885549d6e923fd8e b38b0665905538e76e26f2a4c686179abb1f69f6 d5e397cb49b53381e4c99a064ca733c665646de8 e56d0cfe7790fd3218ae4f6aae1335547fea8763 dbca1df48e89d8aa59254fdc10ef16c16e73d94e fb746d0e1365b7472ccc4c3d5b0672b34a092d0b 6522869c34664dd5f05a0a327e93915b1281c90d d639bab8da86d330493487e8c0fea8ca31f53427 042cbaf88ab48e11afb725541e3c2cbf5b483680 5662a2f8e7313f78d6b17ab383f3e4f04971c335 3b4b75700a245d0d48fc52a4d2f67d3155812aba bf3647c44bc76c43c4b2ebb4c37a559e899ac70e 4369f1fb7cd4cf777312f43e1cb9aa5504fc4125

	Ingo

-------------------->
earth4:~/tip> git merge x86/asm x86/cleanups x86/cpudetect x86/debug 
x86/doc x86/header-fixes x86/mm x86/paravirt x86/pat x86/setup-v2 
x86/subarch x86/uaccess x86/urgent core/percpu
Trying simple merge with 2d4d57db692ea790e185656516e6ebe8791f1788
Trying simple merge with a448720ca3248e8a7a426336885549d6e923fd8e
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/include/asm/io.h
Auto-merging arch/x86/include/asm/spinlock.h
Auto-merging arch/x86/kernel/mpparse.c
Auto-merging arch/x86/kernel/setup_percpu.c
Auto-merging arch/x86/mm/init_32.c
Trying simple merge with b38b0665905538e76e26f2a4c686179abb1f69f6
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/kernel/cpu/common.c
Auto-merging arch/x86/kernel/cpu/intel.c
Auto-merging arch/x86/mm/pat.c
Trying simple merge with d5e397cb49b53381e4c99a064ca733c665646de8
Trying simple merge with e56d0cfe7790fd3218ae4f6aae1335547fea8763
Trying simple merge with dbca1df48e89d8aa59254fdc10ef16c16e73d94e
Trying simple merge with fb746d0e1365b7472ccc4c3d5b0672b34a092d0b
Trying simple merge with 6522869c34664dd5f05a0a327e93915b1281c90d
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/include/asm/paravirt.h
Trying simple merge with d639bab8da86d330493487e8c0fea8ca31f53427
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/include/asm/io.h
Auto-merging arch/x86/mm/ioremap.c
Trying simple merge with 042cbaf88ab48e11afb725541e3c2cbf5b483680
Trying simple merge with 5662a2f8e7313f78d6b17ab383f3e4f04971c335
Trying simple merge with 3b4b75700a245d0d48fc52a4d2f67d3155812aba
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/kernel/signal.c
Trying simple merge with bf3647c44bc76c43c4b2ebb4c37a559e899ac70e
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/kernel/cpu/intel.c
Trying simple merge with 4369f1fb7cd4cf777312f43e1cb9aa5504fc4125
/usr/libexec/git-core/git-merge-octopus: line 52: 26758 Segmentation fault      
git read-tree -u -m --aggressive $common $MRT $SHA1
Merge with strategy octopus failed.
earth4:~/tip> 

Ingo Molnar schrieb:

* Ingo Molnar [off-list ref] wrote:

quoted

a simple 'git fsck' still reports a ton of dangling and missing objects. 
(see it below)

amongst them is the object above:

  missing blob af0e01d4c663a101f48614e40d006ed6272d5c36

but this does not seem to cause problems with git version 1.6.0.6 that 
i'm using now.

hm - maybe it's unrelated, but yesterday, a few hours before the incident 
i did a higher-order Octopus merge with 1.6.1, that segfaulted.

I didnt think much of it - git often crashes with our crazy -tip tree when 
i get above the magic 20 branches limit. The crash left a .git/index.lock 
file around which i removed - and then forgot about the incident. I wanted 
to report those crashes before but procrastinated it.

I used git/maint snapshots because new versions of Git are much better at 
doing Octopus merges.

I've written a reproducer for git-1.6.1.1-1.fc11.i386. See the crash log 
below.

To reproduce, pick up the -tip tree as a remote:

   http://people.redhat.com/mingo/tip.git/README

(you can run the README file as a script)

Then do this:

   git checkout -b tmp.crash v2.6.29-rc3

   git merge  tip/x86/asm        tip/x86/cleanups     tip/x86/cpudetect    \
              tip/x86/debug      tip/x86/doc          tip/x86/header-fixes \
              tip/x86/mm         tip/x86/paravirt     tip/x86/pat          \
              tip/x86/setup-v2   tip/x86/subarch      tip/x86/uaccess      \
              tip/x86/urgent     tip/core/percpu

and you should see the segfault.

interestingly i did not reproduce with the sha1's hardcoded:

  git checkout -b tmp.crash 18e352e4a73465349711a9324767e1b2453383e2

 git merge 2d4d57db692ea790e185656516e6ebe8791f1788 a448720ca3248e8a7a426336885549d6e923fd8e b38b0665905538e76e26f2a4c686179abb1f69f6 d5e397cb49b53381e4c99a064ca733c665646de8 e56d0cfe7790fd3218ae4f6aae1335547fea8763 dbca1df48e89d8aa59254fdc10ef16c16e73d94e fb746d0e1365b7472ccc4c3d5b0672b34a092d0b 6522869c34664dd5f05a0a327e93915b1281c90d d639bab8da86d330493487e8c0fea8ca31f53427 042cbaf88ab48e11afb725541e3c2cbf5b483680 5662a2f8e7313f78d6b17ab383f3e4f04971c335 3b4b75700a245d0d48fc52a4d2f67d3155812aba bf3647c44bc76c43c4b2ebb4c37a559e899ac70e 4369f1fb7cd4cf777312f43e1cb9aa5504fc4125

	Ingo

-------------------->
earth4:~/tip> git merge x86/asm x86/cleanups x86/cpudetect x86/debug 
x86/doc x86/header-fixes x86/mm x86/paravirt x86/pat x86/setup-v2 
x86/subarch x86/uaccess x86/urgent core/percpu
Trying simple merge with 2d4d57db692ea790e185656516e6ebe8791f1788
Trying simple merge with a448720ca3248e8a7a426336885549d6e923fd8e
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/include/asm/io.h
Auto-merging arch/x86/include/asm/spinlock.h
Auto-merging arch/x86/kernel/mpparse.c
Auto-merging arch/x86/kernel/setup_percpu.c
Auto-merging arch/x86/mm/init_32.c
Trying simple merge with b38b0665905538e76e26f2a4c686179abb1f69f6
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/kernel/cpu/common.c
Auto-merging arch/x86/kernel/cpu/intel.c
Auto-merging arch/x86/mm/pat.c
Trying simple merge with d5e397cb49b53381e4c99a064ca733c665646de8
Trying simple merge with e56d0cfe7790fd3218ae4f6aae1335547fea8763
Trying simple merge with dbca1df48e89d8aa59254fdc10ef16c16e73d94e
Trying simple merge with fb746d0e1365b7472ccc4c3d5b0672b34a092d0b
Trying simple merge with 6522869c34664dd5f05a0a327e93915b1281c90d
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/include/asm/paravirt.h
Trying simple merge with d639bab8da86d330493487e8c0fea8ca31f53427
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/include/asm/io.h
Auto-merging arch/x86/mm/ioremap.c
Trying simple merge with 042cbaf88ab48e11afb725541e3c2cbf5b483680
Trying simple merge with 5662a2f8e7313f78d6b17ab383f3e4f04971c335
Trying simple merge with 3b4b75700a245d0d48fc52a4d2f67d3155812aba
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/kernel/signal.c
Trying simple merge with bf3647c44bc76c43c4b2ebb4c37a559e899ac70e
Simple merge did not work, trying automatic merge.
Auto-merging arch/x86/kernel/cpu/intel.c
Trying simple merge with 4369f1fb7cd4cf777312f43e1cb9aa5504fc4125
/usr/libexec/git-core/git-merge-octopus: line 52: 26758 Segmentation fault      
git read-tree -u -m --aggressive $common $MRT $SHA1
Merge with strategy octopus failed.
earth4:~/tip> 

Current master (a34a9dbbc) with the following patch should not
segfault anymore (it doesn't here), but I don't know where the
magic number six is coming from. :-/

diff --git a/unpack-trees.c b/unpack-trees.c
index 16bc2ca..4b02fbf 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -8,6 +8,8 @@
 #include "progress.h"
 #include "refs.h"
 
+#define STAGES (5 + 1)
+
 /*
  * Error messages expected by scripts out of plumbing commands such as
  * read-tree.  Non-scripted Porcelain is not required to use these messages

@@ -143,7 +145,7 @@ static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_o
 
 static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_options *o)
 {
-	struct cache_entry *src[5] = { ce, };
+	struct cache_entry *src[STAGES] = { ce, };
 
 	o->pos++;
 	if (ce_stage(ce)) {

@@ -240,7 +242,7 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info, con
 	return ce;
 }
 
-static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmask, struct cache_entry *src[5],
+static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmask, struct cache_entry *src[STAGES],
 	const struct name_entry *names, const struct traverse_info *info)
 {
 	int i;

@@ -291,7 +293,7 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas
 
 static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
 {
-	struct cache_entry *src[5] = { NULL, };
+	struct cache_entry *src[STAGES] = { NULL, };
 	struct unpack_trees_options *o = info->data;
 	const struct name_entry *p = names;
 

@@ -728,7 +730,7 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 {
 	struct cache_entry *index;
 	struct cache_entry *head;
-	struct cache_entry *remote = stages[o->head_idx + 1];
+	struct cache_entry *remote;
 	int count;
 	int head_match = 0;
 	int remote_match = 0;

@@ -755,6 +757,9 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 		head = NULL;
 	}
 
+	if (o->head_idx + 1 >= STAGES)
+		die("Too many stages (internal error)");
+	remote = stages[o->head_idx + 1];
 	if (remote == o->df_conflict_entry) {
 		df_conflict_remote = 1;
 		remote = NULL;

On Thu, 29 Jan 2009, René Scharfe wrote:

Current master (a34a9dbbc) with the following patch should not
segfault anymore (it doesn't here), but I don't know where the
magic number six is coming from. :-/

Looks like octopus is using multiple merge bases directly, rather than 
combining them using merge-recursive. The merge-recursive concept turned 
out to be superior, and we kind of left multiple merge base support 
hanging. We should probably remove the old support and switch octopus to 
the better way.

My guess is that 6 is just that there happen to be 3 merge bases for this 
particular merge (plus the merge results and the two sides).

	-Daniel
*This .sig left intentionally blank*

The following on top of master lets git finish the merge without segfault
(reporting a merge conflict) and valgrind doesn't find an more memory
errors here.  Can you confirm that, Ingo?

-- 8< --
The parameter n of unpack_callback() can have a value of up to
MAX_UNPACK_TREES.  The check at the top of unpack_trees() (its only
(indirect) caller) makes sure it cannot exceed this limit.

unpack_callback() passes it and the array src to unpack_nondirectories(),
which has this loop:

	for (i = 0; i < n; i++) {
		/* ... */
		src[i + o->merge] = o->df_conflict_entry;

o->merge can be 0 or 1, so unpack_nondirectories() potentially accesses
the array src at index MAX_UNPACK_TREES.  This patch makes it big enough.

Reported-by: Ingo Molnar <redacted>
Signed-off-by: Rene Scharfe <redacted>
---
 unpack-trees.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 16bc2ca..e547282 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c

@@ -240,8 +240,11 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info, con
 	return ce;
 }
 
-static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmask, struct cache_entry *src[5],
-	const struct name_entry *names, const struct traverse_info *info)
+static int unpack_nondirectories(int n, unsigned long mask,
+				 unsigned long dirmask,
+				 struct cache_entry **src,
+				 const struct name_entry *names,
+				 const struct traverse_info *info)
 {
 	int i;
 	struct unpack_trees_options *o = info->data;

@@ -291,7 +294,7 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas
 
 static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
 {
-	struct cache_entry *src[5] = { NULL, };
+	struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
 	struct unpack_trees_options *o = info->data;
 	const struct name_entry *p = names;
 

-- 
1.6.1