{"thread":{"id":"16088","subject":"[PATCH 0/9] more robustness against pack corruptions","startedAt":"2008-10-29T23:02:43Z","lastAt":"2008-10-31T15:31:08Z","messageCount":12,"participants":["Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"94237","messageId":"1225321372-6570-1-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":null,"subject":"[PATCH 0/9] more robustness against pack corruptions","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:43Z","receivedAt":"2008-10-29T23:02:43Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"A few months ago I produced a set of patches to allow git to work even\nin the presence of pack corruption given that the corrupted objects have\na good duplicate in the object store.  Turns out that this work was\nrather incomplete and covered only a limited set of cases.\n\nThis series extend coverage to all cases I could think about, and make\nrepack-objects able to create a good pack in such conditions to \"fix\"\nthe corruption without having to perform a full repack.\n\nYes, this is all about the small and trivial patch I posted a while ago\nthat I intended to repost with a test case.  Well, the test failed\nmiserably, resulting in this series before it finally all passed.  ;-)\n\n builtin-pack-objects.c                |   79 +++++++++++++++++-----\n builtin-unpack-objects.c              |    2 +\n cache.h                               |    2 +-\n index-pack.c                          |    2 +-\n pack-revindex.c                       |    3 +-\n sha1_file.c                           |   85 ++++++++++++++++++-----\n t/t5302-pack-index.sh                 |    3 +-\n t/t5303-pack-corruption-resilience.sh |   96 +++++++++++++++++++++++++--\n 8 files changed, 223 insertions(+), 49 deletions(-)\n\n\nNicolas\n"},{"id":"94238","messageId":"1225321372-6570-2-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-1-git-send-email-nico@cam.org","subject":"[PATCH 1/9] close another possibility for propagating pack corruption","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:44Z","receivedAt":"2008-10-29T23:02:44Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Abstract\n--------\n\nWith index v2 we have a per object CRC to allow quick and safe reuse of\npack data when repacking.  this, however, doesn't currently prevent a\nstealth corruption from being propagated into a new pack when _not_\nreusing pack data as demonstrated by the modification to t5302 included\nhere.\n\nThe Context\n-----------\n\nThe Git database is all checksumed with SHA1 hashes.  Any kind of\ncorruption can be confirmed by verifying this per object hash against\ncorresponding data.  However this can be costly to perform systematically\nand therefore this check is often not performed at run time when\naccessing the object database.\n\nFirst, the loose object format is entirely compressed with zlib which\nalready provide a CRC verification of its own when inflating data.  Any\ndisk corruption would be caught already in this case.\n\nThen, packed objects are also compressed with zlib but only for their\nactual payload.  The object headers and delta base references are not\ndeflated for obvious performance reasons, however this leave them\nvulnerable to potentially undetected disk corruptions.  Object types\nare often validated against the expected type when they're requested,\nand deflated size must always match the size recorded in the object header,\nso those cases are pretty much covered as well.\n\nWhere corruptions could go unnoticed is in the delta base reference.\nOf course, in the OBJ_REF_DELTA case,  the odds for a SHA1 reference to\nget corrupted so it actually matches the SHA1 of another object with the\nsame size (the delta header stores the expected size of the base object\nto apply against) are virtually zero.  In the OBJ_OFS_DELTA case, the\nreference is a pack offset which would have to match the start boundary\nof a different base object but still with the same size, and although this\nis relatively much more \"probable\" than in the OBJ_REF_DELTA case, the\nprobability is also about zero in absolute terms.  Still, the possibility\nexists as demonstrated in t5302 and is certainly greater than a SHA1\ncollision, especially in the OBJ_OFS_DELTA case which is now the default\nwhen repacking.\n\nAgain, repacking by reusing existing pack data is OK since the per object\nCRC provided by index v2 guards against any such corruptions. What t5302\nfailed to test is a full repack in such case.\n\nThe Solution\n------------\n\nAs unlikely as this kind of stealth corruption can be in practice, it\ncertainly isn't acceptable to propagate it into a freshly created pack.\nBut, because this is so unlikely, we don't want to pay the run time cost\nassociated with extra validation checks all the time either.  Furthermore,\nconsequences of such corruption in anything but repacking should be rather\nvisible, and even if it could be quite unpleasant, it still has far less\nsevere consequences than actively creating bad packs.\n\nSo the best compromize is to check packed object CRC when unpacking\nobjects, and only during the compression/writing phase of a repack, and\nonly when not streaming the result.  The cost of this is minimal (less\nthan 1% CPU time), and visible only with a full repack.\n\nSomeone with a stats background could provide an objective evaluation of\nthis, but I suspect that it's bad RAM that has more potential for data\ncorruptions at this point, even in those cases where this extra check\nis not performed.  Still, it is best to prevent a known hole for\ncorruption when recreating object data into a new pack.\n\nWhat about the streamed pack case?  Well, any client receiving a pack\nmust always consider that pack as untrusty and perform full validation\nanyway, hence no such stealth corruption could be propagated to remote\nrepositoryes already.  It is therefore worthless doing local validation\nin that case.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |   12 ++++++++++++\n sha1_file.c            |   15 +++++++++++++++\n t/t5302-pack-index.sh  |    3 ++-\n 3 files changed, 29 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 59c30d1..0366277 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1689,6 +1689,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo\n \treturn 0;\n }\n \n+extern int do_check_packed_object_crc;\n+\n static void prepare_pack(int window, int depth)\n {\n \tstruct object_entry **delta_list;\n@@ -1697,6 +1699,16 @@ static void prepare_pack(int window, int depth)\n \n \tget_object_details();\n \n+\t/*\n+\t * If we're locally repacking then we need to be doubly careful\n+\t * from now on in order to make sure no stealth corruption gets\n+\t * propagated to the new pack.  Clients receiving streamed packs\n+\t * should validate everything they get anyway so no need to incure\n+\t * the additional cost here in that case.\n+\t */\n+\tif (!pack_to_stdout)\n+\t\tdo_check_packed_object_crc = 1;\n+\n \tif (!nr_objects || !window || !depth)\n \t\treturn;\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex ab2b520..88d9cf3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1694,6 +1694,8 @@ static void *unpack_delta_entry(struct packed_git *p,\n \treturn result;\n }\n \n+int do_check_packed_object_crc;\n+\n void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \t\t   enum object_type *type, unsigned long *sizep)\n {\n@@ -1701,6 +1703,19 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \toff_t curpos = obj_offset;\n \tvoid *data;\n \n+\tif (do_check_packed_object_crc && p->index_version > 1) {\n+\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n+\t\tunsigned long len = revidx[1].offset - obj_offset;\n+\t\tif (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {\n+\t\t\tconst unsigned char *sha1 =\n+\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n+\t\t\terror(\"bad packed object CRC for %s\",\n+\t\t\t      sha1_to_hex(sha1));\n+\t\t\tmark_bad_packed_object(p, sha1);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\n \t*type = unpack_object_header(p, &w_curs, &curpos, sizep);\n \tswitch (*type) {\n \tcase OBJ_OFS_DELTA:\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex b0b0fda..884e242 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -196,7 +196,8 @@ test_expect_success \\\n \n test_expect_success \\\n     '[index v2] 5) pack-objects refuses to reuse corrupted data' \\\n-    'test_must_fail git pack-objects test-5 <obj-list'\n+    'test_must_fail git pack-objects test-5 <obj-list &&\n+     test_must_fail git pack-objects --no-reuse-object test-6 <obj-list'\n \n test_expect_success \\\n     '[index v2] 6) verify-pack detects CRC mismatch' \\\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94240","messageId":"1225321372-6570-3-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-2-git-send-email-nico@cam.org","subject":"[PATCH 2/9] better validation on delta base object offsets","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:45Z","receivedAt":"2008-10-29T23:02:45Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"In one case, it was possible to have a bad offset equal to 0 effectively\npointing a delta onto itself and crashing git after too many recursions.\nIn the other cases, a negative offset could result due to off_t being\nsigned.  Catch those.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c   |    4 ++--\n builtin-unpack-objects.c |    2 ++\n index-pack.c             |    2 +-\n sha1_file.c              |    2 +-\n 4 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 0366277..d4c721b 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1038,10 +1038,10 @@ static void check_object(struct object_entry *entry)\n \t\t\t\tc = buf[used_0++];\n \t\t\t\tofs = (ofs << 7) + (c & 127);\n \t\t\t}\n-\t\t\tif (ofs >= entry->in_pack_offset)\n+\t\t\tofs = entry->in_pack_offset - ofs;\n+\t\t\tif (ofs <= 0 || ofs >= entry->in_pack_offset)\n \t\t\t\tdie(\"delta base offset out of bound for %s\",\n \t\t\t\t    sha1_to_hex(entry->idx.sha1));\n-\t\t\tofs = entry->in_pack_offset - ofs;\n \t\t\tif (reuse_delta && !entry->preferred_base) {\n \t\t\t\tstruct revindex_entry *revidx;\n \t\t\t\trevidx = find_pack_revindex(p, ofs);\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 9f4bdd3..47ed610 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -370,6 +370,8 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n \t\t\tbase_offset = (base_offset << 7) + (c & 127);\n \t\t}\n \t\tbase_offset = obj_list[nr].offset - base_offset;\n+\t\tif (base_offset <= 0 || base_offset >= obj_list[nr].offset)\n+\t\t\tdie(\"offset value out of bound for delta base object\");\n \n \t\tdelta_data = get_data(delta_size);\n \t\tif (dry_run || !delta_data) {\ndiff --git a/index-pack.c b/index-pack.c\nindex fe75332..60ed41a 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -338,7 +338,7 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_\n \t\t\tbase_offset = (base_offset << 7) + (c & 127);\n \t\t}\n \t\tdelta_base->offset = obj->idx.offset - base_offset;\n-\t\tif (delta_base->offset >= obj->idx.offset)\n+\t\tif (delta_base->offset <= 0 || delta_base->offset >= obj->idx.offset)\n \t\t\tbad_object(obj->idx.offset, \"delta base offset is out of bound\");\n \t\tbreak;\n \tcase OBJ_COMMIT:\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 88d9cf3..e57949b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1355,7 +1355,7 @@ static off_t get_delta_base(struct packed_git *p,\n \t\t\tbase_offset = (base_offset << 7) + (c & 127);\n \t\t}\n \t\tbase_offset = delta_obj_offset - base_offset;\n-\t\tif (base_offset >= delta_obj_offset)\n+\t\tif (base_offset <= 0 || base_offset >= delta_obj_offset)\n \t\t\treturn 0;  /* out of bound */\n \t\t*curpos += used;\n \t} else if (type == OBJ_REF_DELTA) {\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94239","messageId":"1225321372-6570-4-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-3-git-send-email-nico@cam.org","subject":"[PATCH 3/9] make unpack_object_header() non fatal","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:46Z","receivedAt":"2008-10-29T23:02:46Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"It is possible to have pack corruption in the object header.  Currently\nunpack_object_header() simply die() on them instead of letting the caller\ndeal with that gracefully.\n\nSo let's have unpack_object_header() return an error instead, and find\na better name for unpack_object_header_gently() in that context.  All\ncallers of unpack_object_header() are ready for it.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |    2 +-\n cache.h                |    2 +-\n sha1_file.c            |   20 +++++++++++---------\n 3 files changed, 13 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex d4c721b..9e249c9 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1002,7 +1002,7 @@ static void check_object(struct object_entry *entry)\n \t\t * We want in_pack_type even if we do not reuse delta\n \t\t * since non-delta representations could still be reused.\n \t\t */\n-\t\tused = unpack_object_header_gently(buf, avail,\n+\t\tused = unpack_object_header_buffer(buf, avail,\n \t\t\t\t\t\t   &entry->in_pack_type,\n \t\t\t\t\t\t   &entry->size);\n \ndiff --git a/cache.h b/cache.h\nindex a3c77f0..1a9edf3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -751,7 +751,7 @@ extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t\n extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);\n extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);\n extern void *unpack_entry(struct packed_git *, off_t, enum object_type *, unsigned long *);\n-extern unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n+extern unsigned long unpack_object_header_buffer(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep);\n extern unsigned long get_size_from_delta(struct packed_git *, struct pack_window **, off_t);\n extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigned long *, unsigned long *, unsigned int *, unsigned char *);\n extern int matches_pack_name(struct packed_git *p, const char *name);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e57949b..7698177 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1110,7 +1110,8 @@ static int legacy_loose_object(unsigned char *map)\n \t\treturn 0;\n }\n \n-unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned long len, enum object_type *type, unsigned long *sizep)\n+unsigned long unpack_object_header_buffer(const unsigned char *buf,\n+\t\tunsigned long len, enum object_type *type, unsigned long *sizep)\n {\n \tunsigned shift;\n \tunsigned char c;\n@@ -1122,10 +1123,10 @@ unsigned long unpack_object_header_gently(const unsigned char *buf, unsigned lon\n \tsize = c & 15;\n \tshift = 4;\n \twhile (c & 0x80) {\n-\t\tif (len <= used)\n-\t\t\treturn 0;\n-\t\tif (sizeof(long) * 8 <= shift)\n+\t\tif (len <= used || sizeof(long) * 8 <= shift) {\n+\t\t\terror(\"bad object header\");\n \t\t\treturn 0;\n+\t\t}\n \t\tc = buf[used++];\n \t\tsize += (c & 0x7f) << shift;\n \t\tshift += 7;\n@@ -1164,7 +1165,7 @@ static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned lon\n \t * really worth it and we don't write it any longer.  But we\n \t * can still read it.\n \t */\n-\tused = unpack_object_header_gently(map, mapsize, &type, &size);\n+\tused = unpack_object_header_buffer(map, mapsize, &type, &size);\n \tif (!used || !valid_loose_object_type[type])\n \t\treturn -1;\n \tmap += used;\n@@ -1411,10 +1412,11 @@ static int unpack_object_header(struct packed_git *p,\n \t * insane, so we know won't exceed what we have been given.\n \t */\n \tbase = use_pack(p, w_curs, *curpos, &left);\n-\tused = unpack_object_header_gently(base, left, &type, sizep);\n-\tif (!used)\n-\t\tdie(\"object offset outside of pack file\");\n-\t*curpos += used;\n+\tused = unpack_object_header_buffer(base, left, &type, sizep);\n+\tif (!used) {\n+\t\ttype = OBJ_BAD;\n+\t} else\n+\t\t*curpos += used;\n \n \treturn type;\n }\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94242","messageId":"1225321372-6570-5-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-4-git-send-email-nico@cam.org","subject":"[PATCH 4/9] make packed_object_info() resilient to pack corruptions","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:47Z","receivedAt":"2008-10-29T23:02:47Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"In the same spirit as commit 8eca0b47ff, let's try to survive a pack\ncorruption by making packed_object_info() able to fall back to alternate\npacks or loose objects.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n sha1_file.c |   36 ++++++++++++++++++++++++++++++------\n 1 files changed, 30 insertions(+), 6 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 7698177..384a430 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1314,8 +1314,10 @@ unsigned long get_size_from_delta(struct packed_git *p,\n \t} while ((st == Z_OK || st == Z_BUF_ERROR) &&\n \t\t stream.total_out < sizeof(delta_head));\n \tinflateEnd(&stream);\n-\tif ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head))\n-\t\tdie(\"delta data unpack-initial failed\");\n+\tif ((st != Z_STREAM_END) && stream.total_out != sizeof(delta_head)) {\n+\t\terror(\"delta data unpack-initial failed\");\n+\t\treturn 0;\n+\t}\n \n \t/* Examine the initial part of the delta to figure out\n \t * the result size.\n@@ -1382,15 +1384,29 @@ static int packed_delta_info(struct packed_git *p,\n \toff_t base_offset;\n \n \tbase_offset = get_delta_base(p, w_curs, &curpos, type, obj_offset);\n+\tif (!base_offset)\n+\t\treturn OBJ_BAD;\n \ttype = packed_object_info(p, base_offset, NULL);\n+\tif (type <= OBJ_NONE) {\n+\t\tstruct revindex_entry *revidx = find_pack_revindex(p, base_offset);\n+\t\tconst unsigned char *base_sha1 =\n+\t\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n+\t\tmark_bad_packed_object(p, base_sha1);\n+\t\ttype = sha1_object_info(base_sha1, NULL);\n+\t\tif (type <= OBJ_NONE)\n+\t\t\treturn OBJ_BAD;\n+\t}\n \n \t/* We choose to only get the type of the base object and\n \t * ignore potentially corrupt pack file that expects the delta\n \t * based on a base with a wrong size.  This saves tons of\n \t * inflate() calls.\n \t */\n-\tif (sizep)\n+\tif (sizep) {\n \t\t*sizep = get_size_from_delta(p, w_curs, curpos);\n+\t\tif (*sizep == 0)\n+\t\t\ttype = OBJ_BAD;\n+\t}\n \n \treturn type;\n }\n@@ -1500,8 +1516,9 @@ static int packed_object_info(struct packed_git *p, off_t obj_offset,\n \t\t\t*sizep = size;\n \t\tbreak;\n \tdefault:\n-\t\tdie(\"pack %s contains unknown object type %d\",\n-\t\t    p->pack_name, type);\n+\t\terror(\"unknown object type %i at offset %\"PRIuMAX\" in %s\",\n+\t\t      type, (uintmax_t)obj_offset, p->pack_name);\n+\t\ttype = OBJ_BAD;\n \t}\n \tunuse_pack(&w_curs);\n \treturn type;\n@@ -1971,7 +1988,14 @@ int sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n \t\tif (!find_pack_entry(sha1, &e, NULL))\n \t\t\treturn status;\n \t}\n-\treturn packed_object_info(e.p, e.offset, sizep);\n+\n+\tstatus = packed_object_info(e.p, e.offset, sizep);\n+\tif (status < 0) {\n+\t\tmark_bad_packed_object(e.p, sha1);\n+\t\tstatus = sha1_object_info(sha1, sizep);\n+\t}\n+\n+\treturn status;\n }\n \n static void *read_packed_sha1(const unsigned char *sha1,\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94243","messageId":"1225321372-6570-6-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-5-git-send-email-nico@cam.org","subject":"[PATCH 5/9] make check_object() resilient to pack corruptions","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:48Z","receivedAt":"2008-10-29T23:02:48Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"The check_object() function tries to get away with the least amount of\npack access possible when it already has partial information on given\nobject rather than calling the more costly packed_object_info().\n\nWhen things don't look right, it should just give up and fall back to\npacked_object_info() directly instead of die()'ing.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |   23 +++++++++++++++++------\n 1 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 9e249c9..b595d04 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1005,6 +1005,8 @@ static void check_object(struct object_entry *entry)\n \t\tused = unpack_object_header_buffer(buf, avail,\n \t\t\t\t\t\t   &entry->in_pack_type,\n \t\t\t\t\t\t   &entry->size);\n+\t\tif (used == 0)\n+\t\t\tgoto give_up;\n \n \t\t/*\n \t\t * Determine if this is a delta and if so whether we can\n@@ -1016,6 +1018,8 @@ static void check_object(struct object_entry *entry)\n \t\t\t/* Not a delta hence we've already got all we need. */\n \t\t\tentry->type = entry->in_pack_type;\n \t\t\tentry->in_pack_header_size = used;\n+\t\t\tif (entry->type < OBJ_COMMIT || entry->type > OBJ_BLOB)\n+\t\t\t\tgoto give_up;\n \t\t\tunuse_pack(&w_curs);\n \t\t\treturn;\n \t\tcase OBJ_REF_DELTA:\n@@ -1032,16 +1036,20 @@ static void check_object(struct object_entry *entry)\n \t\t\tofs = c & 127;\n \t\t\twhile (c & 128) {\n \t\t\t\tofs += 1;\n-\t\t\t\tif (!ofs || MSB(ofs, 7))\n-\t\t\t\t\tdie(\"delta base offset overflow in pack for %s\",\n-\t\t\t\t\t    sha1_to_hex(entry->idx.sha1));\n+\t\t\t\tif (!ofs || MSB(ofs, 7)) {\n+\t\t\t\t\terror(\"delta base offset overflow in pack for %s\",\n+\t\t\t\t\t      sha1_to_hex(entry->idx.sha1));\n+\t\t\t\t\tgoto give_up;\n+\t\t\t\t}\n \t\t\t\tc = buf[used_0++];\n \t\t\t\tofs = (ofs << 7) + (c & 127);\n \t\t\t}\n \t\t\tofs = entry->in_pack_offset - ofs;\n-\t\t\tif (ofs <= 0 || ofs >= entry->in_pack_offset)\n-\t\t\t\tdie(\"delta base offset out of bound for %s\",\n-\t\t\t\t    sha1_to_hex(entry->idx.sha1));\n+\t\t\tif (ofs <= 0 || ofs >= entry->in_pack_offset) {\n+\t\t\t\terror(\"delta base offset out of bound for %s\",\n+\t\t\t\t      sha1_to_hex(entry->idx.sha1));\n+\t\t\t\tgoto give_up;\n+\t\t\t}\n \t\t\tif (reuse_delta && !entry->preferred_base) {\n \t\t\t\tstruct revindex_entry *revidx;\n \t\t\t\trevidx = find_pack_revindex(p, ofs);\n@@ -1078,6 +1086,8 @@ static void check_object(struct object_entry *entry)\n \t\t\t */\n \t\t\tentry->size = get_size_from_delta(p, &w_curs,\n \t\t\t\t\tentry->in_pack_offset + entry->in_pack_header_size);\n+\t\t\tif (entry->size == 0)\n+\t\t\t\tgoto give_up;\n \t\t\tunuse_pack(&w_curs);\n \t\t\treturn;\n \t\t}\n@@ -1087,6 +1097,7 @@ static void check_object(struct object_entry *entry)\n \t\t * with sha1_object_info() to find about the object type\n \t\t * at this point...\n \t\t */\n+\t\tgive_up:\n \t\tunuse_pack(&w_curs);\n \t}\n \n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94244","messageId":"1225321372-6570-7-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-6-git-send-email-nico@cam.org","subject":"[PATCH 6/9] make find_pack_revindex() aware of the nasty world","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:49Z","receivedAt":"2008-10-29T23:02:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"It currently calls die() whenever given offset is not found thinking\nthat such thing should never happen.  But this offset may come from a\ncorrupted pack whych _could_ happen and not be found.  Callers should\ndeal with this possibility gracefully instead.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |    2 ++\n pack-revindex.c        |    3 ++-\n sha1_file.c            |   18 ++++++++++++------\n 3 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex b595d04..963b432 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1053,6 +1053,8 @@ static void check_object(struct object_entry *entry)\n \t\t\tif (reuse_delta && !entry->preferred_base) {\n \t\t\t\tstruct revindex_entry *revidx;\n \t\t\t\trevidx = find_pack_revindex(p, ofs);\n+\t\t\t\tif (!revidx)\n+\t\t\t\t\tgoto give_up;\n \t\t\t\tbase_ref = nth_packed_object_sha1(p, revidx->nr);\n \t\t\t}\n \t\t\tentry->in_pack_header_size = used + used_0;\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 6096b62..1de53c8 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -140,7 +140,8 @@ struct revindex_entry *find_pack_revindex(struct packed_git *p, off_t ofs)\n \t\telse\n \t\t\tlo = mi + 1;\n \t} while (lo < hi);\n-\tdie(\"internal error: pack revindex corrupt\");\n+\terror(\"bad offset for revindex\");\n+\treturn NULL;\n }\n \n void discard_revindex(void)\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 384a430..9ce1df0 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1388,9 +1388,12 @@ static int packed_delta_info(struct packed_git *p,\n \t\treturn OBJ_BAD;\n \ttype = packed_object_info(p, base_offset, NULL);\n \tif (type <= OBJ_NONE) {\n-\t\tstruct revindex_entry *revidx = find_pack_revindex(p, base_offset);\n-\t\tconst unsigned char *base_sha1 =\n-\t\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n+\t\tstruct revindex_entry *revidx;\n+\t\tconst unsigned char *base_sha1;\n+\t\trevidx = find_pack_revindex(p, base_offset);\n+\t\tif (!revidx)\n+\t\t\treturn OBJ_BAD;\n+\t\tbase_sha1 = nth_packed_object_sha1(p, revidx->nr);\n \t\tmark_bad_packed_object(p, base_sha1);\n \t\ttype = sha1_object_info(base_sha1, NULL);\n \t\tif (type <= OBJ_NONE)\n@@ -1682,9 +1685,12 @@ static void *unpack_delta_entry(struct packed_git *p,\n \t\t * This is costly but should happen only in the presence\n \t\t * of a corrupted pack, and is better than failing outright.\n \t\t */\n-\t\tstruct revindex_entry *revidx = find_pack_revindex(p, base_offset);\n-\t\tconst unsigned char *base_sha1 =\n-\t\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n+\t\tstruct revindex_entry *revidx;\n+\t\tconst unsigned char *base_sha1;\n+\t\trevidx = find_pack_revindex(p, base_offset);\n+\t\tif (!revidx)\n+\t\t\treturn NULL;\n+\t\tbase_sha1 = nth_packed_object_sha1(p, revidx->nr);\n \t\terror(\"failed to read delta base object %s\"\n \t\t      \" at offset %\"PRIuMAX\" from %s\",\n \t\t      sha1_to_hex(base_sha1), (uintmax_t)base_offset,\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94245","messageId":"1225321372-6570-8-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-7-git-send-email-nico@cam.org","subject":"[PATCH 7/9] pack-objects: allow \"fixing\" a corrupted pack without a full repack","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:50Z","receivedAt":"2008-10-29T23:02:50Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"When the pack data to be reused is found to be bad, let's fall back to\nfull object access through the generic path which has its own strategies\nto find alternate object sources in that case.  This allows for \"fixing\"\na corrupted pack simply by copying either another pack containing the\nobject(s) found to be bad, or the loose object itself, into the object\nstore and launch a repack without the need for -f.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |   28 +++++++++++++++++++---------\n 1 files changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 963b432..826c762 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -277,6 +277,7 @@ static unsigned long write_object(struct sha1file *f,\n \t\t\t\t */\n \n \tif (!to_reuse) {\n+\t\tno_reuse:\n \t\tif (!usable_delta) {\n \t\t\tbuf = read_sha1_file(entry->idx.sha1, &type, &size);\n \t\t\tif (!buf)\n@@ -358,20 +359,30 @@ static unsigned long write_object(struct sha1file *f,\n \t\tstruct revindex_entry *revidx;\n \t\toff_t offset;\n \n-\t\tif (entry->delta) {\n+\t\tif (entry->delta)\n \t\t\ttype = (allow_ofs_delta && entry->delta->idx.offset) ?\n \t\t\t\tOBJ_OFS_DELTA : OBJ_REF_DELTA;\n-\t\t\treused_delta++;\n-\t\t}\n \t\thdrlen = encode_header(type, entry->size, header);\n+\n \t\toffset = entry->in_pack_offset;\n \t\trevidx = find_pack_revindex(p, offset);\n \t\tdatalen = revidx[1].offset - offset;\n \t\tif (!pack_to_stdout && p->index_version > 1 &&\n-\t\t    check_pack_crc(p, &w_curs, offset, datalen, revidx->nr))\n-\t\t\tdie(\"bad packed object CRC for %s\", sha1_to_hex(entry->idx.sha1));\n+\t\t    check_pack_crc(p, &w_curs, offset, datalen, revidx->nr)) {\n+\t\t\terror(\"bad packed object CRC for %s\", sha1_to_hex(entry->idx.sha1));\n+\t\t\tunuse_pack(&w_curs);\n+\t\t\tgoto no_reuse;\n+\t\t}\n+\n \t\toffset += entry->in_pack_header_size;\n \t\tdatalen -= entry->in_pack_header_size;\n+\t\tif (!pack_to_stdout && p->index_version == 1 &&\n+\t\t    check_pack_inflate(p, &w_curs, offset, datalen, entry->size)) {\n+\t\t\terror(\"corrupt packed object for %s\", sha1_to_hex(entry->idx.sha1));\n+\t\t\tunuse_pack(&w_curs);\n+\t\t\tgoto no_reuse;\n+\t\t}\n+\n \t\tif (type == OBJ_OFS_DELTA) {\n \t\t\toff_t ofs = entry->idx.offset - entry->delta->idx.offset;\n \t\t\tunsigned pos = sizeof(dheader) - 1;\n@@ -383,21 +394,19 @@ static unsigned long write_object(struct sha1file *f,\n \t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, dheader + pos, sizeof(dheader) - pos);\n \t\t\thdrlen += sizeof(dheader) - pos;\n+\t\t\treused_delta++;\n \t\t} else if (type == OBJ_REF_DELTA) {\n \t\t\tif (limit && hdrlen + 20 + datalen + 20 >= limit)\n \t\t\t\treturn 0;\n \t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, entry->delta->idx.sha1, 20);\n \t\t\thdrlen += 20;\n+\t\t\treused_delta++;\n \t\t} else {\n \t\t\tif (limit && hdrlen + datalen + 20 >= limit)\n \t\t\t\treturn 0;\n \t\t\tsha1write(f, header, hdrlen);\n \t\t}\n-\n-\t\tif (!pack_to_stdout && p->index_version == 1 &&\n-\t\t    check_pack_inflate(p, &w_curs, offset, datalen, entry->size))\n-\t\t\tdie(\"corrupt packed object for %s\", sha1_to_hex(entry->idx.sha1));\n \t\tcopy_pack_data(f, p, &w_curs, offset, datalen);\n \t\tunuse_pack(&w_curs);\n \t\treused++;\n@@ -1074,6 +1083,7 @@ static void check_object(struct object_entry *entry)\n \t\t\t */\n \t\t\tentry->type = entry->in_pack_type;\n \t\t\tentry->delta = base_entry;\n+\t\t\tentry->delta_size = entry->size;\n \t\t\tentry->delta_sibling = base_entry->delta_child;\n \t\t\tbase_entry->delta_child = entry;\n \t\t\tunuse_pack(&w_curs);\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94246","messageId":"1225321372-6570-9-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-8-git-send-email-nico@cam.org","subject":"[PATCH 8/9] extend test coverage for latest pack corruption resilience improvements","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:51Z","receivedAt":"2008-10-29T23:02:51Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Signed-off-by: Nicolas Pitre <nico@cam.org>\n---\n t/t5303-pack-corruption-resilience.sh |   96 ++++++++++++++++++++++++++++++---\n 1 files changed, 89 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5303-pack-corruption-resilience.sh b/t/t5303-pack-corruption-resilience.sh\nindex 31b20b2..ac181ea 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -41,11 +41,17 @@ create_new_pack() {\n     git verify-pack -v ${pack}.pack\n }\n \n+do_repack() {\n+    pack=`printf \"$blob_1\\n$blob_2\\n$blob_3\\n\" |\n+          git pack-objects $@ .git/objects/pack/pack` &&\n+    pack=\".git/objects/pack/pack-${pack}\"\n+}\n+\n do_corrupt_object() {\n     ofs=`git show-index < ${pack}.idx | grep $1 | cut -f1 -d\" \"` &&\n     ofs=$(($ofs + $2)) &&\n     chmod +w ${pack}.pack &&\n-    dd if=/dev/zero of=${pack}.pack count=1 bs=1 conv=notrunc seek=$ofs &&\n+    dd of=${pack}.pack count=1 bs=1 conv=notrunc seek=$ofs &&\n     test_must_fail git verify-pack ${pack}.pack\n }\n \n@@ -60,7 +66,7 @@ test_expect_success \\\n \n test_expect_success \\\n     'create corruption in header of first object' \\\n-    'do_corrupt_object $blob_1 0 &&\n+    'do_corrupt_object $blob_1 0 < /dev/zero &&\n      test_must_fail git cat-file blob $blob_1 > /dev/null &&\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n@@ -119,7 +125,7 @@ test_expect_success \\\n     'create corruption in header of first delta' \\\n     'create_new_pack &&\n      git prune-packed &&\n-     do_corrupt_object $blob_2 0 &&\n+     do_corrupt_object $blob_2 0 < /dev/zero &&\n      git cat-file blob $blob_1 > /dev/null &&\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n@@ -134,6 +140,15 @@ test_expect_success \\\n      git cat-file blob $blob_3 > /dev/null'\n \n test_expect_success \\\n+    '... and then a repack \"clears\" the corruption' \\\n+    'do_repack &&\n+     git prune-packed &&\n+     git verify-pack ${pack}.pack &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     git cat-file blob $blob_2 > /dev/null &&\n+     git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n     'create corruption in data of first delta' \\\n     'create_new_pack &&\n      git prune-packed &&\n@@ -153,10 +168,19 @@ test_expect_success \\\n      git cat-file blob $blob_3 > /dev/null'\n \n test_expect_success \\\n+    '... and then a repack \"clears\" the corruption' \\\n+    'do_repack &&\n+     git prune-packed &&\n+     git verify-pack ${pack}.pack &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     git cat-file blob $blob_2 > /dev/null &&\n+     git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n     'corruption in delta base reference of first delta (OBJ_REF_DELTA)' \\\n     'create_new_pack &&\n      git prune-packed &&\n-     do_corrupt_object $blob_2 2 &&\n+     do_corrupt_object $blob_2 2 < /dev/zero &&\n      git cat-file blob $blob_1 > /dev/null &&\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n@@ -171,17 +195,75 @@ test_expect_success \\\n      git cat-file blob $blob_3 > /dev/null'\n \n test_expect_success \\\n-    'corruption in delta base reference of first delta (OBJ_OFS_DELTA)' \\\n+    '... and then a repack \"clears\" the corruption' \\\n+    'do_repack &&\n+     git prune-packed &&\n+     git verify-pack ${pack}.pack &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     git cat-file blob $blob_2 > /dev/null &&\n+     git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n+    'corruption #0 in delta base reference of first delta (OBJ_OFS_DELTA)' \\\n     'create_new_pack --delta-base-offset &&\n      git prune-packed &&\n-     do_corrupt_object $blob_2 2 &&\n+     do_corrupt_object $blob_2 2 < /dev/zero &&\n      git cat-file blob $blob_1 > /dev/null &&\n      test_must_fail git cat-file blob $blob_2 > /dev/null &&\n      test_must_fail git cat-file blob $blob_3 > /dev/null'\n \n test_expect_success \\\n-    '... and a redundant pack allows for full recovery too' \\\n+    '... but having a loose copy allows for full recovery' \\\n     'mv ${pack}.idx tmp &&\n+     git hash-object -t blob -w file_2 &&\n+     mv tmp ${pack}.idx &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     git cat-file blob $blob_2 > /dev/null &&\n+     git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n+    '... and then a repack \"clears\" the corruption' \\\n+    'do_repack --delta-base-offset &&\n+     git prune-packed &&\n+     git verify-pack ${pack}.pack &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     git cat-file blob $blob_2 > /dev/null &&\n+     git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n+    'corruption #1 in delta base reference of first delta (OBJ_OFS_DELTA)' \\\n+    'create_new_pack --delta-base-offset &&\n+     git prune-packed &&\n+     printf \"\\x01\" | do_corrupt_object $blob_2 2 &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     test_must_fail git cat-file blob $blob_2 > /dev/null &&\n+     test_must_fail git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n+    '... but having a loose copy allows for full recovery' \\\n+    'mv ${pack}.idx tmp &&\n+     git hash-object -t blob -w file_2 &&\n+     mv tmp ${pack}.idx &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     git cat-file blob $blob_2 > /dev/null &&\n+     git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n+    '... and then a repack \"clears\" the corruption' \\\n+    'do_repack --delta-base-offset &&\n+     git prune-packed &&\n+     git verify-pack ${pack}.pack &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     git cat-file blob $blob_2 > /dev/null &&\n+     git cat-file blob $blob_3 > /dev/null'\n+\n+test_expect_success \\\n+    '... and a redundant pack allows for full recovery too' \\\n+    'do_corrupt_object $blob_2 2 < /dev/zero &&\n+     git cat-file blob $blob_1 > /dev/null &&\n+     test_must_fail git cat-file blob $blob_2 > /dev/null &&\n+     test_must_fail git cat-file blob $blob_3 > /dev/null &&\n+     mv ${pack}.idx tmp &&\n      git hash-object -t blob -w file_1 &&\n      git hash-object -t blob -w file_2 &&\n      printf \"$blob_1\\n$blob_2\\n\" | git pack-objects .git/objects/pack/pack &&\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94247","messageId":"1225321372-6570-10-git-send-email-nico@cam.org","threadId":"16088","inReplyTo":"1225321372-6570-9-git-send-email-nico@cam.org","subject":"[PATCH 9/9] pack-objects: don't leak pack window reference when splitting packs","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-29T23:02:52Z","receivedAt":"2008-10-29T23:02:52Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Signed-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |   12 +++++++++---\n 1 files changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 826c762..c93d69a 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -389,22 +389,28 @@ static unsigned long write_object(struct sha1file *f,\n \t\t\tdheader[pos] = ofs & 127;\n \t\t\twhile (ofs >>= 7)\n \t\t\t\tdheader[--pos] = 128 | (--ofs & 127);\n-\t\t\tif (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit)\n+\t\t\tif (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit) {\n+\t\t\t\tunuse_pack(&w_curs);\n \t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, dheader + pos, sizeof(dheader) - pos);\n \t\t\thdrlen += sizeof(dheader) - pos;\n \t\t\treused_delta++;\n \t\t} else if (type == OBJ_REF_DELTA) {\n-\t\t\tif (limit && hdrlen + 20 + datalen + 20 >= limit)\n+\t\t\tif (limit && hdrlen + 20 + datalen + 20 >= limit) {\n+\t\t\t\tunuse_pack(&w_curs);\n \t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, entry->delta->idx.sha1, 20);\n \t\t\thdrlen += 20;\n \t\t\treused_delta++;\n \t\t} else {\n-\t\t\tif (limit && hdrlen + datalen + 20 >= limit)\n+\t\t\tif (limit && hdrlen + datalen + 20 >= limit) {\n+\t\t\t\tunuse_pack(&w_curs);\n \t\t\t\treturn 0;\n+\t\t\t}\n \t\t\tsha1write(f, header, hdrlen);\n \t\t}\n \t\tcopy_pack_data(f, p, &w_curs, offset, datalen);\n-- \n1.6.0.3.757.g01be.dirty\n"},{"id":"94383","messageId":"7vod11884o.fsf@gitster.siamese.dyndns.org","threadId":"16088","inReplyTo":"1225321372-6570-2-git-send-email-nico@cam.org","subject":"Re: [PATCH 1/9] close another possibility for propagating pack corruption","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-10-31T08:46:15Z","receivedAt":"2008-10-31T08:46:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> Abstract\n> --------\n>\n> With index v2 we have a per object CRC to allow quick and safe reuse of\n> pack data when repacking.  this, however, doesn't currently prevent a\n\ns/this/This/;\n\n> stealth corruption from being propagated into a new pack when _not_\n> reusing pack data as demonstrated by the modification to t5302 included\n> here.\n>\n> The Context\n> -----------\n>\n> The Git database is all checksumed with SHA1 hashes.  Any kind of\n\ns/checksumed/checksummed/;\n\n> ...\n> What about the streamed pack case?  Well, any client receiving a pack\n> must always consider that pack as untrusty and perform full validation\n> anyway, hence no such stealth corruption could be propagated to remote\n> repositoryes already.  It is therefore worthless doing local validation\n> in that case.\n\nVery nicely done.  I've never seen a commit message that needs its own\nAbstract ;-)\n\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index 59c30d1..0366277 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> @@ -1689,6 +1689,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo\n>  \treturn 0;\n>  }\n>  \n> +extern int do_check_packed_object_crc;\n> +\n\nThis ought to belong to cache.h or some other header file.  Perhaps you\ndid this to avoid unnecessary recompilation (we've discussed this at\nGitTogether)?\n\n> @@ -1697,6 +1699,16 @@ static void prepare_pack(int window, int depth)\n>  \n>  \tget_object_details();\n>  \n> +\t/*\n> +\t * If we're locally repacking then we need to be doubly careful\n> +\t * from now on in order to make sure no stealth corruption gets\n> +\t * propagated to the new pack.  Clients receiving streamed packs\n> +\t * should validate everything they get anyway so no need to incure\n\ns/incure/incur/;\n"},{"id":"94409","messageId":"alpine.LFD.2.00.0810311115570.13034@xanadu.home","threadId":"16088","inReplyTo":"7vod11884o.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/9] close another possibility for propagating pack corruption","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-10-31T15:31:08Z","receivedAt":"2008-10-31T15:31:08Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 31 Oct 2008, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > Abstract\n> > --------\n> >\n> > With index v2 we have a per object CRC to allow quick and safe reuse of\n> > pack data when repacking.  this, however, doesn't currently prevent a\n> \n> s/this/This/;\n\nThanks for proofreading.  ;-)\n\n> Very nicely done.  I've never seen a commit message that needs its own\n> Abstract ;-)\n\nWell, maybe I could run for the best commit message award!  :-)\n\n> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> > index 59c30d1..0366277 100644\n> > --- a/builtin-pack-objects.c\n> > +++ b/builtin-pack-objects.c\n> > @@ -1689,6 +1689,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo\n> >  \treturn 0;\n> >  }\n> >  \n> > +extern int do_check_packed_object_crc;\n> > +\n> \n> This ought to belong to cache.h or some other header file.  Perhaps you\n> did this to avoid unnecessary recompilation (we've discussed this at\n> GitTogether)?\n\nNo, I just felt this wasn't a really public thing to expose at large.\nBut I don't feel really strongly about that.\n\nRevised patch follows.\n\n--------\nFrom: Nicolas Pitre <nico@cam.org>\nDate: Tue, 28 Oct 2008 20:58:42 -0400\nSubject: [PATCH] close another possibility for propagating pack corruption\n\nAbstract\n--------\n\nWith index v2 we have a per object CRC to allow quick and safe reuse of\npack data when repacking.  This, however, doesn't currently prevent a\nstealth corruption from being propagated into a new pack when _not_\nreusing pack data as demonstrated by the modification to t5302 included\nhere.\n\nThe Context\n-----------\n\nThe Git database is all checksummed with SHA1 hashes.  Any kind of\ncorruption can be confirmed by verifying this per object hash against\ncorresponding data.  However this can be costly to perform systematically\nand therefore this check is often not performed at run time when\naccessing the object database.\n\nFirst, the loose object format is entirely compressed with zlib which\nalready provide a CRC verification of its own when inflating data.  Any\ndisk corruption would be caught already in this case.\n\nThen, packed objects are also compressed with zlib but only for their\nactual payload.  The object headers and delta base references are not\ndeflated for obvious performance reasons, however this leave them\nvulnerable to potentially undetected disk corruptions.  Object types\nare often validated against the expected type when they're requested,\nand deflated size must always match the size recorded in the object header,\nso those cases are pretty much covered as well.\n\nWhere corruptions could go unnoticed is in the delta base reference.\nOf course, in the OBJ_REF_DELTA case,  the odds for a SHA1 reference to\nget corrupted so it actually matches the SHA1 of another object with the\nsame size (the delta header stores the expected size of the base object\nto apply against) are virtually zero.  In the OBJ_OFS_DELTA case, the\nreference is a pack offset which would have to match the start boundary\nof a different base object but still with the same size, and although this\nis relatively much more \"probable\" than in the OBJ_REF_DELTA case, the\nprobability is also about zero in absolute terms.  Still, the possibility\nexists as demonstrated in t5302 and is certainly greater than a SHA1\ncollision, especially in the OBJ_OFS_DELTA case which is now the default\nwhen repacking.\n\nAgain, repacking by reusing existing pack data is OK since the per object\nCRC provided by index v2 guards against any such corruptions. What t5302\nfailed to test is a full repack in such case.\n\nThe Solution\n------------\n\nAs unlikely as this kind of stealth corruption can be in practice, it\ncertainly isn't acceptable to propagate it into a freshly created pack.\nBut, because this is so unlikely, we don't want to pay the run time cost\nassociated with extra validation checks all the time either.  Furthermore,\nconsequences of such corruption in anything but repacking should be rather\nvisible, and even if it could be quite unpleasant, it still has far less\nsevere consequences than actively creating bad packs.\n\nSo the best compromize is to check packed object CRC when unpacking\nobjects, and only during the compression/writing phase of a repack, and\nonly when not streaming the result.  The cost of this is minimal (less\nthan 1% CPU time), and visible only with a full repack.\n\nSomeone with a stats background could provide an objective evaluation of\nthis, but I suspect that it's bad RAM that has more potential for data\ncorruptions at this point, even in those cases where this extra check\nis not performed.  Still, it is best to prevent a known hole for\ncorruption when recreating object data into a new pack.\n\nWhat about the streamed pack case?  Well, any client receiving a pack\nmust always consider that pack as untrusty and perform full validation\nanyway, hence no such stealth corruption could be propagated to remote\nrepositoryes already.  It is therefore worthless doing local validation\nin that case.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 59c30d1..026b7ea 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -1697,6 +1697,16 @@ static void prepare_pack(int window, int depth)\n \n \tget_object_details();\n \n+\t/*\n+\t * If we're locally repacking then we need to be doubly careful\n+\t * from now on in order to make sure no stealth corruption gets\n+\t * propagated to the new pack.  Clients receiving streamed packs\n+\t * should validate everything they get anyway so no need to incur\n+\t * the additional cost here in that case.\n+\t */\n+\tif (!pack_to_stdout)\n+\t\tdo_check_packed_object_crc = 1;\n+\n \tif (!nr_objects || !window || !depth)\n \t\treturn;\n \ndiff --git a/cache.h b/cache.h\nindex a3c77f0..0dc13cc 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -565,6 +565,9 @@ extern int force_object_loose(const unsigned char *sha1, time_t mtime);\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+/* global flag to enable extra checks when accessing packed objects */\n+extern int do_check_packed_object_crc;\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);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ab2b520..88d9cf3 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1694,6 +1694,8 @@ static void *unpack_delta_entry(struct packed_git *p,\n \treturn result;\n }\n \n+int do_check_packed_object_crc;\n+\n void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \t\t   enum object_type *type, unsigned long *sizep)\n {\n@@ -1701,6 +1703,19 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,\n \toff_t curpos = obj_offset;\n \tvoid *data;\n \n+\tif (do_check_packed_object_crc && p->index_version > 1) {\n+\t\tstruct revindex_entry *revidx = find_pack_revindex(p, obj_offset);\n+\t\tunsigned long len = revidx[1].offset - obj_offset;\n+\t\tif (check_pack_crc(p, &w_curs, obj_offset, len, revidx->nr)) {\n+\t\t\tconst unsigned char *sha1 =\n+\t\t\t\tnth_packed_object_sha1(p, revidx->nr);\n+\t\t\terror(\"bad packed object CRC for %s\",\n+\t\t\t      sha1_to_hex(sha1));\n+\t\t\tmark_bad_packed_object(p, sha1);\n+\t\t\treturn NULL;\n+\t\t}\n+\t}\n+\n \t*type = unpack_object_header(p, &w_curs, &curpos, sizep);\n \tswitch (*type) {\n \tcase OBJ_OFS_DELTA:\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex b0b0fda..884e242 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -196,7 +196,8 @@ test_expect_success \\\n \n test_expect_success \\\n     '[index v2] 5) pack-objects refuses to reuse corrupted data' \\\n-    'test_must_fail git pack-objects test-5 <obj-list'\n+    'test_must_fail git pack-objects test-5 <obj-list &&\n+     test_must_fail git pack-objects --no-reuse-object test-6 <obj-list'\n \n test_expect_success \\\n     '[index v2] 6) verify-pack detects CRC mismatch' \\\n"}]}