threads / patch / 29804

patch, 4 partsAnother bundle fix: reading freed memory

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

## tl;dr

11 messages between Mar 1, 2012 and Mar 1, 2012. Diffs are folded; open one to read it.

replies: 10people: 2as markdown or json

Thomas Rast· Mar 1, 2012, 21:40 UTC · lore

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
Thomas Rast· Mar 1, 2012, 21:40 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to t/t5510-fetch.sh +10 −14
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
Thomas Rast· Mar 1, 2012, 21:40 UTC · re: Thomas Rast · lore

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

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 t/t5510-fetch.sh |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)
Show changes to t/t5510-fetch.sh +8 −8
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
Thomas Rast· Mar 1, 2012, 21:40 UTC · re: Thomas Rast · lore

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

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(+)
Show changes to t/t5510-fetch.sh +1 −0
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
Junio C Hamano· Mar 1, 2012, 21:57 UTC · re: Thomas Rast · lore

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

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?
Show 17 quoted lines
> 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 &&
Thomas Rast· Mar 1, 2012, 22:09 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> 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
Junio C Hamano· Mar 1, 2012, 22:32 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@inf.ethz.ch> writes:
Show 22 quoted lines
> 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.
Junio C Hamano· Mar 1, 2012, 22:33 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@inf.ethz.ch> writes:
Show 22 quoted lines
> 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.
Thomas Rast· Mar 1, 2012, 21:40 UTC · re: Thomas Rast · lore

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

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(-)
Show changes to 2 files +17 −1

bundle.c, t/t5510-fetch.sh

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
Junio C Hamano· Mar 1, 2012, 22:05 UTC · re: Thomas Rast · lore

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

Thomas Rast <trast@student.ethz.ch> writes:
Show 8 quoted lines
> 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.
Show 10 quoted lines
> 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.
Thomas Rast· Mar 1, 2012, 22:22 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
> 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

← back to recent threads