{"thread":{"id":"29804","subject":"[PATCH 0/4] Another bundle fix: reading freed memory","startedAt":"2012-03-01T21:40:47Z","lastAt":"2012-03-01T22:33:31Z","messageCount":11,"participants":["Thomas Rast","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"185854","messageId":"cover.1330637923.git.trast@student.ethz.ch","threadId":"29804","inReplyTo":null,"subject":"[PATCH 0/4] Another bundle fix: reading freed memory","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-01T21:40:47Z","receivedAt":"2012-03-01T21:40:47Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"This is about the fourth patch, the other three are just cleanups.\nValgrind caught me reading free()d memory after the recent change to a\nstrbuf in create_bundle. Luckily it's the only report I got.\n\nI made a token effort to verify that nobody else is playing mind games\nwith the .name fields in struct object_array, but I'm rather tired so\nit would be nice if someone could double-check. In any case bundle is\nthe only user of object_array_remove_duplicates.\n\nThomas Rast (4):\n t5510: refactor bundle->pack conversion\n t5510: fix indent with spaces\n t5510: ensure we stay in the toplevel test dir\n bundle: keep around names passed to add_pending_object()\n\n bundle.c | 2 +-\n t/t5510-fetch.sh | 57 +++++++++++++++++++++++++++++++++---------------------\n 2 files changed, 36 insertions(+), 23 deletions(-)\n\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185856","messageId":"c8e641fe6f33a4ed795da8d40f5be743a2e5c45d.1330637923.git.trast@student.ethz.ch","threadId":"29804","inReplyTo":"cover.1330637923.git.trast@student.ethz.ch","subject":"[PATCH 1/4] t5510: refactor bundle->pack conversion","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-01T21:40:48Z","receivedAt":"2012-03-01T21:40:48Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"It's not so much a conversion as a \"strip everything up to and\nincluding the first blank line\", but it will come in handy again.\n\nSigned-off-by: Thomas Rast \n---\n t/t5510-fetch.sh | 24 ++++++++++--------------\n 1 file changed, 10 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 79ee913..6df24b2 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -14,6 +14,14 @@ test_bundle_object_count () {\n \ttest \"$2\" = $(grep '^[0-9a-f]\\{40\\} ' verify.out | wc -l)\n }\n \n+convert_bundle_to_pack () {\n+\twhile read x && test -n \"$x\"\n+\tdo\n+\t\t:;\n+\tdone\n+\tcat\n+}\n+\n test_expect_success setup '\n \techo >file original &&\n \tgit add file &&\n@@ -206,13 +214,7 @@ test_expect_success 'unbundle 1' '\n \n test_expect_success 'bundle 1 has only 3 files ' '\n \tcd \"$D\" &&\n-\t(\n-\t\twhile read x && test -n \"$x\"\n-\t\tdo\n-\t\t\t:;\n-\t\tdone\n-\t\tcat\n-\t) bundle.pack &&\n+\tconvert_bundle_to_pack bundle.pack &&\n \tgit index-pack bundle.pack &&\n \ttest_bundle_object_count bundle.pack 3\n '\n@@ -229,13 +231,7 @@ test_expect_success 'bundle does not prerequisite objects' '\n \tgit add file2 &&\n \tgit commit -m add.file2 file2 &&\n \tgit bundle create bundle3 -1 HEAD &&\n-\t(\n-\t\twhile read x && test -n \"$x\"\n-\t\tdo\n-\t\t\t:;\n-\t\tdone\n-\t\tcat\n-\t) bundle.pack &&\n+\tconvert_bundle_to_pack bundle.pack &&\n \tgit index-pack bundle.pack &&\n \ttest_bundle_object_count bundle.pack 3\n '\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185858","messageId":"72a62936d799cb72a39943a60a15541d7aa1db0f.1330637923.git.trast@student.ethz.ch","threadId":"29804","inReplyTo":"cover.1330637923.git.trast@student.ethz.ch","subject":"[PATCH 2/4] t5510: fix indent with spaces","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-01T21:40:49Z","receivedAt":"2012-03-01T21:40:49Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Signed-off-by: Thomas Rast \n---\n t/t5510-fetch.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 6df24b2..8827828 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -429,14 +429,14 @@ test_expect_success 'fetch --dry-run' '\n '\n \n test_expect_success \"should be able to fetch with duplicate refspecs\" '\n- mkdir dups &&\n- cd dups &&\n- git init &&\n- git config branch.master.remote three &&\n- git config remote.three.url ../three/.git &&\n- git config remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&\n- git config --add remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&\n- git fetch three\n+\tmkdir dups &&\n+\tcd dups &&\n+\tgit init &&\n+\tgit config branch.master.remote three &&\n+\tgit config remote.three.url ../three/.git &&\n+\tgit config remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&\n+\tgit config --add remote.three.fetch +refs/heads/*:refs/remotes/origin/* &&\n+\tgit fetch three\n '\n \n test_done\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185857","messageId":"80204781996b200d08b73cf61fa50df468e4b51d.1330637923.git.trast@student.ethz.ch","threadId":"29804","inReplyTo":"cover.1330637923.git.trast@student.ethz.ch","subject":"[PATCH 3/4] t5510: ensure we stay in the toplevel test dir","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-01T21:40:50Z","receivedAt":"2012-03-01T21:40:50Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The last test descended into a subdir without ever re-emerging, which\nis not so nice to the next test writer.\n\nSigned-off-by: Thomas Rast \n---\n t/t5510-fetch.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 8827828..dd035bf 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -431,6 +431,7 @@ test_expect_success 'fetch --dry-run' '\n test_expect_success \"should be able to fetch with duplicate refspecs\" '\n \tmkdir dups &&\n \tcd dups &&\n+\ttest_when_finished \"cd ..\" &&\n \tgit init &&\n \tgit config branch.master.remote three &&\n \tgit config remote.three.url ../three/.git &&\n-- \n1.7.9.2.467.g7fee4\n"},{"id":"185855","messageId":"6fba13f255f76481dc9098b5e8fbb33bd10735cb.1330637923.git.trast@student.ethz.ch","threadId":"29804","inReplyTo":"cover.1330637923.git.trast@student.ethz.ch","subject":"[PATCH 4/4] bundle: keep around names passed to add_pending_object()","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-01T21:40:51Z","receivedAt":"2012-03-01T21:40:51Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"The 'name' field passed to add_pending_object() is used to later\ndeduplicate in object_array_remove_duplicates().\n\ngit-bundle had a bug in this area since 18449ab (git-bundle: avoid\npacking objects which are in the prerequisites, 2007-03-08): it passed\nthe name of each boundary object in a static buffer. In other words,\nall that object_array_remove_duplicates() saw was the name of the\n*last* added boundary object.\n\nThe recent switch to a strbuf in bc2fed4 (bundle: use a strbuf to scan\nthe log for boundary commits, 2012-02-22) made this slightly worse: we\nnow free the buffer at the end, so it is not even guaranteed that it\nstill points into addressable memory by the time object_array_remove_\nduplicates looks at it. On the plus side however, it was now\ndetectable by valgrind.\n\nThe fix is easy: pass a copy of the string to add_pending_object.\n\nSigned-off-by: Thomas Rast \n---\n bundle.c | 2 +-\n t/t5510-fetch.sh | 16 ++++++++++++++++\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 7a760db..d9cfd90 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -273,7 +273,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object(sha1);\n \t\t\t\tobject->flags |= UNINTERESTING;\n-\t\t\t\tadd_pending_object(&revs, object, buf.buf);\n+\t\t\t\tadd_pending_object(&revs, object, xstrdup(buf.buf));\n \t\t\t}\n \t\t} else if (!get_sha1_hex(buf.buf, sha1)) {\n \t\t\tstruct object *object = parse_object(sha1);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex dd035bf..8b914f9 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -440,4 +440,20 @@ test_expect_success \"should be able to fetch with duplicate refspecs\" '\n \tgit fetch three\n '\n \n+\n+test_expect_success 'all boundary commits are excluded' '\n+\ttest_commit base &&\n+\ttest_commit oneside &&\n+\tgit checkout HEAD^ &&\n+\ttest_commit otherside &&\n+\tgit checkout master &&\n+\ttest_tick &&\n+\tgit merge otherside &&\n+\tad=$(git log --no-walk --format=%ad HEAD) &&\n+\tgit bundle create twoside-boundary.bdl master --since=\"$ad\" &&\n+\tconvert_bundle_to_pack twoside-boundary.pack &&\n+\tpack=$(git index-pack --fix-thin --stdin writes:\n\n> The last test descended into a subdir without ever re-emerging, which\n> is not so nice to the next test writer.\n\nTrue. Making the test suite more robust like this patch does is very much\nappreciated.\n\nIs there a reason why we shouldn't be sticking to the more usual\n\n\tmkdir dups &&\n (\n \tcd dups &&\n do whatever in dups\n\t)\n\npattern?\n\n> Signed-off-by: Thomas Rast \n> ---\n> t/t5510-fetch.sh | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 8827828..dd035bf 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -431,6 +431,7 @@ test_expect_success 'fetch --dry-run' '\n> test_expect_success \"should be able to fetch with duplicate refspecs\" '\n> \tmkdir dups &&\n> \tcd dups &&\n> +\ttest_when_finished \"cd ..\" &&\n> \tgit init &&\n> \tgit config branch.master.remote three &&\n> \tgit config remote.three.url ../three/.git &&\n"},{"id":"185862","messageId":"7vehtckmee.fsf@alter.siamese.dyndns.org","threadId":"29804","inReplyTo":"6fba13f255f76481dc9098b5e8fbb33bd10735cb.1330637923.git.trast@student.ethz.ch","subject":"Re: [PATCH 4/4] bundle: keep around names passed to add_pending_object()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-01T22:05:13Z","receivedAt":"2012-03-01T22:05:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> The 'name' field passed to add_pending_object() is used to later\n> deduplicate in object_array_remove_duplicates().\n>\n> git-bundle had a bug in this area since 18449ab (git-bundle: avoid\n> packing objects which are in the prerequisites, 2007-03-08): it passed\n> the name of each boundary object in a static buffer. In other words,\n> all that object_array_remove_duplicates() saw was the name of the\n> *last* added boundary object.\n\nOuch.\n\n> diff --git a/bundle.c b/bundle.c\n> index 7a760db..d9cfd90 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -273,7 +273,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n> \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n> \t\t\t\tstruct object *object = parse_object(sha1);\n> \t\t\t\tobject->flags |= UNINTERESTING;\n> -\t\t\t\tadd_pending_object(&revs, object, buf.buf);\n> +\t\t\t\tadd_pending_object(&revs, object, xstrdup(buf.buf));\n\nWe'll release &buf after the loop but the elements in the pending object\narray will keep their own copies, so now we would be safe.\n\nThanks for fixing this.\n"},{"id":"185863","messageId":"871upcht2h.fsf@thomas.inf.ethz.ch","threadId":"29804","inReplyTo":"7vipiokmrz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] t5510: ensure we stay in the toplevel test dir","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-01T22:09:26Z","receivedAt":"2012-03-01T22:09:26Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> The last test descended into a subdir without ever re-emerging, which\n>> is not so nice to the next test writer.\n>\n> True. Making the test suite more robust like this patch does is very much\n> appreciated.\n>\n> Is there a reason why we shouldn't be sticking to the more usual\n>\n> \tmkdir dups &&\n> (\n> \tcd dups &&\n> do whatever in dups\n> \t)\n>\n> pattern?\n\nNone in particular. It would also perhaps reduce the churn since (as it\ntouches every line anyway) it could be squashed with 2/4. Should I reroll?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185864","messageId":"87obsggdwh.fsf@thomas.inf.ethz.ch","threadId":"29804","inReplyTo":"7vehtckmee.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] bundle: keep around names passed to add_pending_object()","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2012-03-01T22:22:21Z","receivedAt":"2012-03-01T22:22:21Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano writes:\n\n> Thomas Rast writes:\n>\n>> The 'name' field passed to add_pending_object() is used to later\n>> deduplicate in object_array_remove_duplicates().\n>>\n>> git-bundle had a bug in this area since 18449ab (git-bundle: avoid\n>> packing objects which are in the prerequisites, 2007-03-08): it passed\n>> the name of each boundary object in a static buffer. In other words,\n>> all that object_array_remove_duplicates() saw was the name of the\n>> *last* added boundary object.\n>\n> Ouch.\n\nActually, I just noticed that object_array_remove_duplicates() is much\nnewer than 18449ab, from b2a6d1c (bundle: allow the same ref to be given\nmore than once, 2009-01-17). However it did pass in the same buffer all\nthe time starting from 18449ab, which is a bug waiting to happen (and\nperhaps NULL would have been more appropriate).\n\nArchaeology sure is fun.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"185865","messageId":"7vaa40kl4h.fsf@alter.siamese.dyndns.org","threadId":"29804","inReplyTo":"871upcht2h.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 3/4] t5510: ensure we stay in the toplevel test dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-01T22:32:46Z","receivedAt":"2012-03-01T22:32:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Junio C Hamano writes:\n>\n>> Thomas Rast writes:\n>>\n>>> The last test descended into a subdir without ever re-emerging, which\n>>> is not so nice to the next test writer.\n>>\n>> True. Making the test suite more robust like this patch does is very much\n>> appreciated.\n>>\n>> Is there a reason why we shouldn't be sticking to the more usual\n>>\n>> \tmkdir dups &&\n>> (\n>> \tcd dups &&\n>> do whatever in dups\n>> \t)\n>>\n>> pattern?\n>\n> None in particular. It would also perhaps reduce the churn since (as it\n> touches every line anyway) it could be squashed with 2/4. Should I reroll?\n\nNo. I was just curious.\n"},{"id":"185866","messageId":"7v8vjkkl38.fsf@alter.siamese.dyndns.org","threadId":"29804","inReplyTo":"871upcht2h.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 3/4] t5510: ensure we stay in the toplevel test dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-01T22:33:31Z","receivedAt":"2012-03-01T22:33:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast writes:\n\n> Junio C Hamano writes:\n>\n>> Thomas Rast writes:\n>>\n>>> The last test descended into a subdir without ever re-emerging, which\n>>> is not so nice to the next test writer.\n>>\n>> True. Making the test suite more robust like this patch does is very much\n>> appreciated.\n>>\n>> Is there a reason why we shouldn't be sticking to the more usual\n>>\n>> \tmkdir dups &&\n>> (\n>> \tcd dups &&\n>> do whatever in dups\n>> \t)\n>>\n>> pattern?\n>\n> None in particular. It would also perhaps reduce the churn since (as it\n> touches every line anyway) it could be squashed with 2/4. Should I reroll?\n\nI was just curious.\n\nBut it does sound like squashing the two may make sense. I can manage\nlocally.\n\nThanks.\n"}]}