{"thread":{"id":"14455","subject":"[BUG] commit walk machinery is dangerous !","startedAt":"2008-07-14T20:54:24Z","lastAt":"2008-07-15T05:12:30Z","messageCount":9,"participants":["Nicolas Pitre","Junio C Hamano","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"83301","messageId":"alpine.LFD.1.10.0807141641110.12484@xanadu.home","threadId":"14455","inReplyTo":null,"subject":"[BUG] commit walk machinery is dangerous !","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-14T20:54:24Z","receivedAt":"2008-07-14T20:54:24Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Good!  I have all your attention now.\n\nYes, I'm kinda fscking upset about my hardware at this moment.  I \napparently have git packs corrupting themselves from time to time which \nprompted me to make git more robust against some kind of corruptions \nrecently.\n\nHowever this time a corruption turned up and exposed what I think is a \nmajor flaw in git's error checking.  To demonstrate it, I created the \nfollowing test case.  Turning the error() into a die() on line 772 of \ncommit.c makes this test pass but I don't know if this is the \nappropriate fix (e.g. some attempt to parse non existing commits could \nbe valid usage, etc.).  Note this is critical only for git versions \nlater than commit 8eca0b47ff15.\n\nSo here's the test.  The catastrophic consequences that this can have on \none's repository is left as an exercise to the reader.\n\ndiff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh\nnew file mode 100755\nindex 0000000..a5fe190\n--- /dev/null\n+++ b/t/t6011-rev-list-with-bad-commit.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='git rev-list should notice bad commits'\n+\n+. ./test-lib.sh\n+\n+# Note:\n+# - compression level is set to zero to make \"corruptions\" easier to perform\n+# - reflog is disabled to avoid extra references which would twart the test\n+\n+test_expect_success 'setup' \\\n+   '\n+   git init &&\n+   git config core.compression 0 &&\n+   git config core.logallrefupdates false &&\n+   echo \"foo\" > foo &&\n+   git add foo &&\n+   git commit -m \"first commit\" &&\n+   echo \"bar\" > bar &&\n+   git add bar &&\n+   git commit -m \"second commit\" &&\n+   echo \"baz\" > baz &&\n+   git add baz &&\n+   git commit -m \"third commit\" &&\n+   echo \"foo again\" >> foo &&\n+   git add foo &&\n+   git commit -m \"fourth commit\" &&\n+   git repack -a -f -d\n+   '\n+\n+test_expect_success 'verify number of revisions' \\\n+   '\n+   revs=$(git rev-list --all | wc -l) &&\n+   test $revs -eq 4 &&\n+   first_commit=$(git rev-parse HEAD~3)\n+   '\n+\n+test_expect_success 'corrupt second commit object' \\\n+   '\n+   perl -i.bak -pe \"s/second commit/socond commit/\" .git/objects/pack/*.pack &&\n+   test_must_fail git fsck --all\n+   '\n+\n+test_expect_success 'rev-list should fail' \\\n+   '\n+   test_must_fail git rev-list --all > /dev/null\n+   '\n+\n+test_expect_success 'git repack _MUST_ fail' \\\n+   '\n+   test_must_fail git repack -a -f -d\n+   '\n+\n+test_expect_success 'first commit is still available' \\\n+   '\n+   git log $first_commit\n+   '\n+\n+test_done\n+\n"},{"id":"83309","messageId":"7vbq10f7wr.fsf@gitster.siamese.dyndns.org","threadId":"14455","inReplyTo":"alpine.LFD.1.10.0807141641110.12484@xanadu.home","subject":"Re: [BUG] commit walk machinery is dangerous !","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-14T21:55:00Z","receivedAt":"2008-07-14T21:55:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> However this time a corruption turned up and exposed what I think is a \n> major flaw in git's error checking.  To demonstrate it, I created the \n> following test case.  Turning the error() into a die() on line 772 of \n> commit.c makes this test pass but I don't know if this is the \n> appropriate fix (e.g. some attempt to parse non existing commits could \n> be valid usage, etc.).  Note this is critical only for git versions \n> later than commit 8eca0b47ff15.\n\nWhich probably means we should revert that commit as faulty?  IIRC, before\nthat commit we did check and error out correctly but you loosened the\ncheck to introduce \"a major flaw\" with that commit.\n\n$ for b in maint master next pu\n  do\n      echo -n $b; git cat-file blob $b:commit.c | wc -l\n  done\nmaint 672\nmaster 672\nnext 779\npu 789\n\nHmph...\n"},{"id":"83313","messageId":"alpine.LFD.1.10.0807141904250.12484@xanadu.home","threadId":"14455","inReplyTo":"7vbq10f7wr.fsf@gitster.siamese.dyndns.org","subject":"Re: [BUG] commit walk machinery is dangerous !","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-14T23:08:59Z","receivedAt":"2008-07-14T23:08:59Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 14 Jul 2008, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > However this time a corruption turned up and exposed what I think is a \n> > major flaw in git's error checking.  To demonstrate it, I created the \n> > following test case.  Turning the error() into a die() on line 772 of \n> > commit.c makes this test pass but I don't know if this is the \n> > appropriate fix (e.g. some attempt to parse non existing commits could \n> > be valid usage, etc.).  Note this is critical only for git versions \n> > later than commit 8eca0b47ff15.\n> \n> Which probably means we should revert that commit as faulty?  IIRC, before\n> that commit we did check and error out correctly but you loosened the\n> check to introduce \"a major flaw\" with that commit.\n> \n> $ for b in maint master next pu\n>   do\n>       echo -n $b; git cat-file blob $b:commit.c | wc -l\n>   done\n> maint 672\n> master 672\n> next 779\n> pu 789\n> \n> Hmph...\n\nWell, most of them aren't that critical.  If anything they will only \ncause a segfault if ever the return value is not checked.\n\nIt is those with semantic meaning (e.g. object doesn't exist) which \nshould be audited, especially if used in the context of repository \nmodification, which pretty much limits it to the test case I produced.\n\n\nNicolas\n"},{"id":"83318","messageId":"7vod50dote.fsf@gitster.siamese.dyndns.org","threadId":"14455","inReplyTo":"alpine.LFD.1.10.0807141904250.12484@xanadu.home","subject":"Re: [BUG] commit walk machinery is dangerous !","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-07-14T23:32:45Z","receivedAt":"2008-07-14T23:32:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> It is those with semantic meaning (e.g. object doesn't exist) which \n> should be audited, especially if used in the context of repository \n> modification, which pretty much limits it to the test case I produced.\n\nI've been wondering if we should make the change 8eca0b4 (implement some\nresilience against pack corruptions, 2008-06-23) less aggressive.\n\nIt makes loose objects and data from other packs to be used as fallback\nwhere we used to just punt, which is a genuine improvement for \"salvaging\"\nmode of operation, but at the same time, it now forbids the callers to\nexpect that the objects they learned to exist from has_sha1_file() or\nnth_packed_object_sha1() should never result NULL return value from\nread_sha1_file().\n\nIt may make it safe again to fail if you cannot salvage using fallback\nmethod after all.  Something like the attached.\n\nThis is unrelated to the issue at hand, but I also notice that there are\nfew callsites outside sha1_file.c that bypasses cache_or_unpack_entry()\nand call unpack_entry() directly.  I wonder if they should be using the\ncached version, making unpack_entry() static...\n\n sha1_file.c |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2df78b5..55aa361 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1649,7 +1649,7 @@ static void *unpack_delta_entry(struct packed_git *p,\n \t\tmark_bad_packed_object(p, base_sha1);\n \t\tbase = read_sha1_file(base_sha1, type, &base_size);\n \t\tif (!base)\n-\t\t\treturn NULL;\n+\t\t\texit(129);\n \t}\n \n \tdelta_data = unpack_compressed_entry(p, w_curs, curpos, delta_size);\n@@ -1946,6 +1946,8 @@ static void *read_packed_sha1(const unsigned char *sha1,\n \t\t      sha1_to_hex(sha1), (uintmax_t)e.offset, e.p->pack_name);\n \t\tmark_bad_packed_object(e.p, sha1);\n \t\tdata = read_sha1_file(sha1, type, size);\n+\t\tif (!data)\n+\t\t\texit(129);\n \t}\n \treturn data;\n }\n"},{"id":"83319","messageId":"alpine.LFD.1.10.0807141937180.12484@xanadu.home","threadId":"14455","inReplyTo":"7vod50dote.fsf@gitster.siamese.dyndns.org","subject":"Re: [BUG] commit walk machinery is dangerous !","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-14T23:39:08Z","receivedAt":"2008-07-14T23:39:08Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 14 Jul 2008, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > It is those with semantic meaning (e.g. object doesn't exist) which \n> > should be audited, especially if used in the context of repository \n> > modification, which pretty much limits it to the test case I produced.\n> \n> I've been wondering if we should make the change 8eca0b4 (implement some\n> resilience against pack corruptions, 2008-06-23) less aggressive.\n> \n> It makes loose objects and data from other packs to be used as fallback\n> where we used to just punt, which is a genuine improvement for \"salvaging\"\n> mode of operation, but at the same time, it now forbids the callers to\n> expect that the objects they learned to exist from has_sha1_file() or\n> nth_packed_object_sha1() should never result NULL return value from\n> read_sha1_file().\n> \n> It may make it safe again to fail if you cannot salvage using fallback\n> method after all.  Something like the attached.\n\nWell, I have a different solution which should restore the original \n\"behavior\" in the presence of existing but non-readable objects.  Patch \nwill follow later.\n\n\nNicolas\n"},{"id":"83333","messageId":"alpine.LFD.1.10.0807142134450.12484@xanadu.home","threadId":"14455","inReplyTo":"alpine.LFD.1.10.0807141937180.12484@xanadu.home","subject":"[PATCH 1/2] restore legacy behavior for read_sha1_file()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-15T01:46:48Z","receivedAt":"2008-07-15T01:46:48Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Since commit 8eca0b47ff1598a6d163df9358c0e0c9bd92d4c8, it is possible\nfor read_sha1_file() to return NULL even with existing objects when they\nare corrupted.  Previously a corrupted object would have terminated the\nprogram immediately, effectively making read_sha1_file() return NULL \nonly when specified object is not found.\n\nLet's restore this behavior for all users of read_sha1_file() and \nprovide a separate function with the ability to not terminate when\nbad objects are encountered.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n\nOn Mon, 14 Jul 2008, Nicolas Pitre wrote:\n\n> Well, I have a different solution which should restore the original \n> \"behavior\" in the presence of existing but non-readable objects.  Patch \n> will follow later.\n\nSo here it is.\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 2df78b5..e281c14 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1006,6 +1006,18 @@ static void mark_bad_packed_object(struct packed_git *p,\n \tp->num_bad_objects++;\n }\n \n+static int has_packed_and_bad(const unsigned char *sha1)\n+{\n+\tstruct packed_git *p;\n+\tunsigned i;\n+\n+\tfor (p = packed_git; p; p = p->next)\n+\t\tfor (i = 0; i < p->num_bad_objects; i++)\n+\t\t\tif (!hashcmp(sha1, p->bad_object_sha1 + 20 * i))\n+\t\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long size, const char *type)\n {\n \tunsigned char real_sha1[20];\n@@ -1647,7 +1659,7 @@ static void *unpack_delta_entry(struct packed_git *p,\n \t\t      sha1_to_hex(base_sha1), (uintmax_t)base_offset,\n \t\t      p->pack_name);\n \t\tmark_bad_packed_object(p, base_sha1);\n-\t\tbase = read_sha1_file(base_sha1, type, &base_size);\n+\t\tbase = read_object(base_sha1, type, &base_size);\n \t\tif (!base)\n \t\t\treturn NULL;\n \t}\n@@ -1945,7 +1957,7 @@ static void *read_packed_sha1(const unsigned char *sha1,\n \t\terror(\"failed to read object %s at offset %\"PRIuMAX\" from %s\",\n \t\t      sha1_to_hex(sha1), (uintmax_t)e.offset, e.p->pack_name);\n \t\tmark_bad_packed_object(e.p, sha1);\n-\t\tdata = read_sha1_file(sha1, type, size);\n+\t\tdata = read_object(sha1, type, size);\n \t}\n \treturn data;\n }\n@@ -2010,8 +2022,8 @@ int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,\n \treturn 0;\n }\n \n-void *read_sha1_file(const unsigned char *sha1, enum object_type *type,\n-\t\t     unsigned long *size)\n+void *read_object(const unsigned char *sha1, enum object_type *type,\n+\t\t  unsigned long *size)\n {\n \tunsigned long mapsize;\n \tvoid *map, *buf;\n@@ -2037,6 +2049,16 @@ void *read_sha1_file(const unsigned char *sha1, enum object_type *type,\n \treturn read_packed_sha1(sha1, type, size);\n }\n \n+void *read_sha1_file(const unsigned char *sha1, enum object_type *type,\n+\t\t     unsigned long *size)\n+{\n+\tvoid *data = read_object(sha1, type, size);\n+\t/* legacy behavior is to die on corrupted objects */\n+\tif (!data && (has_loose_object(sha1) || has_packed_and_bad(sha1)))\n+\t\tdie(\"object %s is corrupted\", sha1_to_hex(sha1));\n+\treturn data;\n+}\n+\n void *read_object_with_reference(const unsigned char *sha1,\n \t\t\t\t const char *required_type_name,\n \t\t\t\t unsigned long *size,\n"},{"id":"83335","messageId":"alpine.LFD.1.10.0807142147270.12484@xanadu.home","threadId":"14455","inReplyTo":"alpine.LFD.1.10.0807142134450.12484@xanadu.home","subject":"[PATCH 2/2] test case for previous commit","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-15T01:50:46Z","receivedAt":"2008-07-15T01:50:46Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Here's a test demonstrating a case of what was broken before\n(and fixed by) the previous patch.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\ndiff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh\nnew file mode 100755\nindex 0000000..e51eb41\n--- /dev/null\n+++ b/t/t6011-rev-list-with-bad-commit.sh\n@@ -0,0 +1,60 @@\n+#!/bin/sh\n+\n+test_description='git rev-list should notice bad commits'\n+\n+. ./test-lib.sh\n+\n+# Note:\n+# - compression level is set to zero to make \"corruptions\" easier to perform\n+# - reflog is disabled to avoid extra references which would twart the test\n+\n+test_expect_success 'setup' \\\n+   '\n+   git init &&\n+   git config core.compression 0 &&\n+   git config core.logallrefupdates false &&\n+   echo \"foo\" > foo &&\n+   git add foo &&\n+   git commit -m \"first commit\" &&\n+   echo \"bar\" > bar &&\n+   git add bar &&\n+   git commit -m \"second commit\" &&\n+   echo \"baz\" > baz &&\n+   git add baz &&\n+   git commit -m \"third commit\" &&\n+   echo \"foo again\" >> foo &&\n+   git add foo &&\n+   git commit -m \"fourth commit\" &&\n+   git repack -a -f -d\n+   '\n+\n+test_expect_success 'verify number of revisions' \\\n+   '\n+   revs=$(git rev-list --all | wc -l) &&\n+   test $revs -eq 4 &&\n+   first_commit=$(git rev-parse HEAD~3)\n+   '\n+\n+test_expect_success 'corrupt second commit object' \\\n+   '\n+   perl -i.bak -pe \"s/second commit/socond commit/\" .git/objects/pack/*.pack &&\n+   test_must_fail git fsck --full\n+   '\n+\n+test_expect_success 'rev-list should fail' \\\n+   '\n+   test_must_fail git rev-list --all > /dev/null\n+   '\n+\n+test_expect_success 'git repack _MUST_ fail' \\\n+   '\n+   test_must_fail git repack -a -f -d\n+   '\n+\n+test_expect_success 'first commit is still available' \\\n+   '\n+   git log $first_commit\n+   '\n+\n+test_done\n+\n"},{"id":"83361","messageId":"20080715051056.GI2432@spearce.org","threadId":"14455","inReplyTo":"7vod50dote.fsf@gitster.siamese.dyndns.org","subject":"unpack_entry (was: [BUG] commit walk machinery is dangerous !)","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-07-15T05:10:56Z","receivedAt":"2008-07-15T05:10:56Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> This is unrelated to the issue at hand, but I also notice that there are\n> few callsites outside sha1_file.c that bypasses cache_or_unpack_entry()\n> and call unpack_entry() directly.  I wonder if they should be using the\n> cached version, making unpack_entry() static...\n\nThere are two such callsites that I found:\n\n$ git grep -n --cached unpack_entry\nfast-import.c:1201:     return unpack_entry(p, oe->offset, &type, sizep);\npack-check.c:114:               data = unpack_entry(p, entries[i].offset, &type,\n\nNow the one in fast-import.c could be using the cached version.\nThis is simply fast-import trying to reuse sha1_file.c for reading\na previously written object to the pack.\n\nStuffing the item into the cache is perhaps pointless here as gfi\ndoes its own sort of caching and only goes through this code path\nwhen that caching has tossed the least-recently-used data and its\nsuddenly needed again.\n\nSo yea, it could be using the caching form, but it would maybe be\ndoing more work (and using more memory) than it really needs.\n\n\nIn pack-check.c we are looping through the objects in the order they\nappear in the index so we can unpack them and verify each object's\nSHA-1 signature.  Please note this is perhaps the slowest way to\nenumerate through the pack and is why you can clone a repository\nover git:// faster than you can run `fsck --full`.\n\nIf you really want to verify every single object's SHA-1, run the\ndamn pack through index-pack and compare the new index to the old\nindex (hint they should be identical, bit-for-bit).\n\nSo again, in this case the caching may only cause us to do more\nwork (and use more memory) than we need as we are slamming the\ndelta base cache with all sorts of unrelated base objects.  It\nprobably has a low hit ratio anyway during this loop.  :-|\n\n-- \nShawn.\n"},{"id":"83362","messageId":"alpine.LFD.1.10.0807150111110.12484@xanadu.home","threadId":"14455","inReplyTo":"alpine.LFD.1.10.0807142134450.12484@xanadu.home","subject":"Re: [PATCH 1/2] restore legacy behavior for read_sha1_file()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-07-15T05:12:30Z","receivedAt":"2008-07-15T05:12:30Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Mon, 14 Jul 2008, Nicolas Pitre wrote:\n\n> Since commit 8eca0b47ff1598a6d163df9358c0e0c9bd92d4c8, it is possible\n> for read_sha1_file() to return NULL even with existing objects when they\n> are corrupted.  Previously a corrupted object would have terminated the\n> program immediately, effectively making read_sha1_file() return NULL \n> only when specified object is not found.\n> \n> Let's restore this behavior for all users of read_sha1_file() and \n> provide a separate function with the ability to not terminate when\n> bad objects are encountered.\n> \n> Signed-off-by: Nicolas Pitre <nico@cam.org>\n\nGrrrr.  Forgot to 'git add' one file.  So this goes with the same \ncommit:\n\ndiff --git a/cache.h b/cache.h\nindex 4a8b125..bc52af6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -540,6 +540,9 @@ extern int write_sha1_file(void *buf, unsigned long len, const char *type, unsig\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n \n+/* just like read_sha1_file(), but non fatal in presence of bad objects */\n+extern void *read_object(const unsigned char *sha1, enum object_type *type, unsigned long *size);\n+\n extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);\n \n extern int move_temp_to_file(const char *tmpfile, const char *filename);\n"}]}