Re: testsuite failures in mainline...

David Miller [off-list ref] writes:

	if (!prefixcmp(key, "branch.")) {
		name = key + 7;
		subkey = strrchr(name, '.');
		branch = make_branch(name, subkey - name);

What if 'subkey' is NULL?  I bet that's what happening here.

Wow, good eyes.

It makes me wonder what my C library has been returning during the
tests...

---

 remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index 3fb0f99..0e00680 100644
--- a/remote.c
+++ b/remote.c

@@ -220,11 +220,11 @@ static int handle_config(const char *key, const char *value)
 	if (!prefixcmp(key, "branch.")) {
 		name = key + 7;
 		subkey = strrchr(name, '.');
-		branch = make_branch(name, subkey - name);
 		if (!subkey)
 			return 0;
 		if (!value)
 			return 0;
+		branch = make_branch(name, subkey - name);
 		if (!strcmp(subkey, ".remote")) {
 			branch->remote_name = xstrdup(value);
 			if (branch == current_branch)

From: Junio C Hamano <redacted>
Date: Fri, 14 Dec 2007 15:18:02 -0800

It makes me wonder what my C library has been returning during the
tests...

If the 'name' string is high enough in the address space, the
'NULL - name' is still small enough to keep malloc() from
failing.

It might be neat to defeat bugs like this by making a
pointer_diff(a,b) macro or similar, that abort()'s when
one of the arguments is NULL.  Otherwise these bugs are
so hard to find.

I tested your patch and that part of the testsuite passes now.

It now fails on t9301-fast-export.sh

+ eval '

	MASTER=$(git rev-parse --verify master) &&
	REIN=$(git rev-parse --verify rein) &&
	WER=$(git rev-parse --verify wer) &&
	MUSS=$(git rev-parse --verify muss) &&
	mkdir new &&
	git --git-dir=new/.git init &&
	git fast-export --all |
	(cd new &&
	 git fast-import &&
	 test $MASTER = $(git rev-parse --verify refs/heads/master) &&
	 test $REIN = $(git rev-parse --verify refs/tags/rein) &&
	 test $WER = $(git rev-parse --verify refs/heads/wer) &&
	 test $MUSS = $(git rev-parse --verify refs/tags/muss))

'

+++ git rev-parse --verify master
++ MASTER=e529bca54909ee82f6ed442ef855ff541aec034c
+++ git rev-parse --verify rein
++ REIN=e529bca54909ee82f6ed442ef855ff541aec034c
+++ git rev-parse --verify wer
++ WER=ce754ded7a378a51278b2ff76d6898ec20093068
+++ git rev-parse --verify muss
++ MUSS=d85ef2305117d94969d4990d3c752752d4719be1
++ mkdir new
++ git --git-dir=new/.git init

Initialized empty Git repository in new/.git/
++ git fast-export --all
++ cd new
++ git fast-import
./test-lib.sh: line 194: 17409 Bus error               (core dumped) git fast-import

This usually indicates an unaligned memory access on sparc,
which is where I'm running this.

The problem is the pool allocator in fast-import.c, it aligned objects
on the size of a pointer.  But this is insufficient, it needs to be at
least "uintmax_t" aligned.

Also, mem_pool->space needs to be suitably aligned for a uintmax_t
as well.

The following patch fixes the bug, and together with your patch all
test cases now pass for me on sparc.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/fast-import.c b/fast-import.c
index 98c2bd5..4646c05 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -196,7 +196,7 @@ struct mem_pool
 	struct mem_pool *next_pool;
 	char *next_free;
 	char *end;
-	char space[FLEX_ARRAY]; /* more */
+	uintmax_t space[FLEX_ARRAY]; /* more */
 };
 
 struct atom_str

@@ -534,15 +534,15 @@ static void *pool_alloc(size_t len)
 		total_allocd += sizeof(struct mem_pool) + mem_pool_alloc;
 		p = xmalloc(sizeof(struct mem_pool) + mem_pool_alloc);
 		p->next_pool = mem_pool;
-		p->next_free = p->space;
+		p->next_free = (char *) p->space;
 		p->end = p->next_free + mem_pool_alloc;
 		mem_pool = p;
 	}
 
 	r = p->next_free;
-	/* round out to a pointer alignment */
-	if (len & (sizeof(void*) - 1))
-		len += sizeof(void*) - (len & (sizeof(void*) - 1));
+	/* round out to a 'uintmax_t' alignment */
+	if (len & (sizeof(uintmax_t) - 1))
+		len += sizeof(uintmax_t) - (len & (sizeof(uintmax_t) - 1));
 	p->next_free += len;
 	return r;
 }

Hi,

On Fri, 14 Dec 2007, David Miller wrote:

-	char space[FLEX_ARRAY]; /* more */
+	uintmax_t space[FLEX_ARRAY]; /* more */

Usually, a much better idea is to use

	union {
		char cp[FLEX_ARRAY];
		uintmax_t up[FLEX_ARRAY];
	}

because that is exactly the reason union was invented for.

Ciao,
Dscho