[PATCH 0/4] Another bundle fix: reading freed memory

[PATCH 0/4] Another bundle fix: reading freed memory

[Thomas Rast]

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

[PATCH 1/4] t5510: refactor bundle->pack conversion

[Thomas Rast]

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.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t5510-fetch.sh |   24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 79ee913..6df24b2 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -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

[PATCH 2/4] t5510: fix indent with spaces

[Thomas Rast]

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t5510-fetch.sh |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 6df24b2..8827828 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -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

[PATCH 3/4] t5510: ensure we stay in the toplevel test dir

[Thomas Rast]

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

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t5510-fetch.sh |    1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 8827828..dd035bf 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -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

[PATCH 4/4] bundle: keep around names passed to add_pending_object()

[Thomas Rast]

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.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 bundle.c         |    2 +-
 t/t5510-fetch.sh |   16 ++++++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/bundle.c b/bundle.c
index 7a760db..d9cfd90 100644
--- a/bundle.c
+++ b/bundle.c
@@ -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);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index dd035bf..8b914f9 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -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

Re: [PATCH 3/4] t5510: ensure we stay in the toplevel test dir

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

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

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?

> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  t/t5510-fetch.sh |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 8827828..dd035bf 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -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 &&

Re: [PATCH 4/4] bundle: keep around names passed to add_pending_object()

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> 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.

Ouch.

> diff --git a/bundle.c b/bundle.c
> index 7a760db..d9cfd90 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -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));

We'll release &buf after the loop but the elements in the pending object
array will keep their own copies, so now we would be safe.

Thanks for fixing this.

Re: [PATCH 3/4] t5510: ensure we stay in the toplevel test dir

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> The last test descended into a subdir without ever re-emerging, which
>> is not so nice to the next test writer.
>
> 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

Re: [PATCH 4/4] bundle: keep around names passed to add_pending_object()

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
>
>> 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.
>
> 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

Re: [PATCH 3/4] t5510: ensure we stay in the toplevel test dir

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> The last test descended into a subdir without ever re-emerging, which
>>> is not so nice to the next test writer.
>>
>> 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?

No. I was just curious.

Re: [PATCH 3/4] t5510: ensure we stay in the toplevel test dir

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Thomas Rast <trast@student.ethz.ch> writes:
>>
>>> The last test descended into a subdir without ever re-emerging, which
>>> is not so nice to the next test writer.
>>
>> 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?

I was just curious.

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

Thanks.