Another bundle fix: reading freed memory

This is about the fourth patch, the other three are just cleanups. Valgrind caught me reading free()d memory after the recent change to a strbuf in create_bundle. Luckily it's the only report I got.

I made a token effort to verify that nobody else is playing mind games with the .name fields in struct object_array, but I'm rather tired so it would be nice if someone could double-check. In any case bundle is the only user of object_array_remove_duplicates.

Thomas Rast (4):
t5510: refactor bundle->pack conversion
t5510: fix indent with spaces
t5510: ensure we stay in the toplevel test dir
bundle: keep around names passed to add_pending_object()

 bundle.c         |    2 +-
 t/t5510-fetch.sh |   57 +++++++++++++++++++++++++++++++++---------------------
 2 files changed, 36 insertions(+), 23 deletions(-)

1.7.9.2.467.g7fee4

It's not so much a conversion as a "strip everything up to and including the first blank line", but it will come in handy again.

---
 t/t5510-fetch.sh |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

index 79ee913..6df24b2 100755
@@ -14,6 +14,14 @@ test_bundle_object_count () {
 	test "$2" = $(grep '^[0-9a-f]\{40\} ' verify.out | wc -l)
 }
 
+convert_bundle_to_pack () {
+	while read x && test -n "$x"
+	do
+		:;
+	done
+	cat
+}
+
 test_expect_success setup '
 	echo >file original &&
 	git add file &&

@@ -206,13 +214,7 @@ test_expect_success 'unbundle 1' '
 
 test_expect_success 'bundle 1 has only 3 files ' '
 	cd "$D" &&
-	(
-		while read x && test -n "$x"
-		do
-			:;
-		done
-		cat
-	) <bundle1 >bundle.pack &&
+	convert_bundle_to_pack <bundle1 >bundle.pack &&
 	git index-pack bundle.pack &&
 	test_bundle_object_count bundle.pack 3
 '

@@ -229,13 +231,7 @@ test_expect_success 'bundle does not prerequisite objects' '
 	git add file2 &&
 	git commit -m add.file2 file2 &&
 	git bundle create bundle3 -1 HEAD &&
-	(
-		while read x && test -n "$x"
-		do
-			:;
-		done
-		cat
-	) <bundle3 >bundle.pack &&
+	convert_bundle_to_pack <bundle3 >bundle.pack &&
 	git index-pack bundle.pack &&
 	test_bundle_object_count bundle.pack 3
 '

1.7.9.2.467.g7fee4

---
 t/t5510-fetch.sh |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index 6df24b2..8827828 100755
@@ -429,14 +429,14 @@ test_expect_success 'fetch --dry-run' '
 '
 
 test_expect_success "should be able to fetch with duplicate refspecs" '
-        mkdir dups &&
-        cd dups &&
-        git init &&
-        git config branch.master.remote three &&
-        git config remote.three.url ../three/.git &&
-        git config remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&
-        git config --add remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&
-        git fetch three
+	mkdir dups &&
+	cd dups &&
+	git init &&
+	git config branch.master.remote three &&
+	git config remote.three.url ../three/.git &&
+	git config remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&
+	git config --add remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&
+	git fetch three
 '
 
 test_done

1.7.9.2.467.g7fee4

The last test descended into a subdir without ever re-emerging, which is not so nice to the next test writer.

---
 t/t5510-fetch.sh |    1 +
 1 file changed, 1 insertion(+)

index 8827828..dd035bf 100755
@@ -431,6 +431,7 @@ test_expect_success 'fetch --dry-run' '
 test_expect_success "should be able to fetch with duplicate refspecs" '
 	mkdir dups &&
 	cd dups &&
+	test_when_finished "cd .." &&
 	git init &&
 	git config branch.master.remote three &&
 	git config remote.three.url ../three/.git &&

1.7.9.2.467.g7fee4

True. Making the test suite more robust like this patch does is very much appreciated.

Is there a reason why we shouldn't be sticking to the more usual

	mkdir dups &&
        (
        	cd dups &&
                do whatever in dups
	)

pattern?

None in particular. It would also perhaps reduce the churn since (as it touches every line anyway) it could be squashed with 2/4. Should I reroll?

Thomas Rast
trast@{inf,student}.ethz.ch

I was just curious.

But it does sound like squashing the two may make sense. I can manage locally.

Thanks.

No. I was just curious.

The 'name' field passed to add_pending_object() is used to later deduplicate in object_array_remove_duplicates().

git-bundle had a bug in this area since 18449ab (git-bundle: avoid packing objects which are in the prerequisites, 2007-03-08): it passed the name of each boundary object in a static buffer. In other words, all that object_array_remove_duplicates() saw was the name of the *last* added boundary object.

The recent switch to a strbuf in bc2fed4 (bundle: use a strbuf to scan the log for boundary commits, 2012-02-22) made this slightly worse: we now free the buffer at the end, so it is not even guaranteed that it still points into addressable memory by the time object_array_remove_ duplicates looks at it. On the plus side however, it was now detectable by valgrind.

The fix is easy: pass a copy of the string to add_pending_object.

---
 bundle.c         |    2 +-
 t/t5510-fetch.sh |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

index 7a760db..d9cfd90 100644
@@ -273,7 +273,7 @@ int create_bundle(struct bundle_header *header, const char *path,
 			if (!get_sha1_hex(buf.buf + 1, sha1)) {
 				struct object *object = parse_object(sha1);
 				object->flags |= UNINTERESTING;
-				add_pending_object(&revs, object, buf.buf);
+				add_pending_object(&revs, object, xstrdup(buf.buf));
 			}
 		} else if (!get_sha1_hex(buf.buf, sha1)) {
 			struct object *object = parse_object(sha1);

index dd035bf..8b914f9 100755
@@ -440,4 +440,20 @@ test_expect_success "should be able to fetch with duplicate refspecs" '
 	git fetch three
 '
 
+
+test_expect_success 'all boundary commits are excluded' '
+	test_commit base &&
+	test_commit oneside &&
+	git checkout HEAD^ &&
+	test_commit otherside &&
+	git checkout master &&
+	test_tick &&
+	git merge otherside &&
+	ad=$(git log --no-walk --format=%ad HEAD) &&
+	git bundle create twoside-boundary.bdl master --since="$ad" &&
+	convert_bundle_to_pack <twoside-boundary.bdl >twoside-boundary.pack &&
+	pack=$(git index-pack --fix-thin --stdin <twoside-boundary.pack) &&
+	test_bundle_object_count .git/objects/pack/pack-${pack##pack	}.pack 3
+'
+
 test_done

1.7.9.2.467.g7fee4

Ouch.

Actually, I just noticed that object_array_remove_duplicates() is much newer than 18449ab, from b2a6d1c (bundle: allow the same ref to be given more than once, 2009-01-17). However it did pass in the same buffer all the time starting from 18449ab, which is a bug waiting to happen (and perhaps NULL would have been more appropriate).

Archaeology sure is fun.

Thomas Rast
trast@{inf,student}.ethz.ch