{"thread":{"id":"13727","subject":"[PATCH] make verify-pack a bit more useful with bad packs","startedAt":"2008-05-29T21:34:50Z","lastAt":"2008-06-02T06:27:40Z","messageCount":2,"participants":["Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"78087","messageId":"alpine.LFD.1.10.0805291716470.23581@xanadu.home","threadId":"13727","inReplyTo":null,"subject":"[PATCH] make verify-pack a bit more useful with bad packs","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-05-29T21:34:50Z","receivedAt":"2008-05-29T21:34:50Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"When a pack gets corrupted, its SHA1 checksum will fail.  However, this\nis more useful to let the test go on in order to find the actual \nproblem location than only complain about the SHA1 mismatch and\nbail out.\n\nAlso, it is more useful to compare the stored pack SHA1 with the one in \nthe index file instead of the computed SHA1 since the computed SHA1\nfrom a corrupted pack won't match the one stored in the index either.\n\nFinally a few code and message cleanups were thrown in as a bonus.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n\nYes, I just had my first real usage Git repository corruption...\nStill investigating.\n\ndiff --git a/pack-check.c b/pack-check.c\nindex 0f8ad2c..c03525e 100644\n--- a/pack-check.c\n+++ b/pack-check.c\n@@ -25,10 +25,10 @@ static int verify_packfile(struct packed_git *p,\n \toff_t index_size = p->index_size;\n \tconst unsigned char *index_base = p->index_data;\n \tSHA_CTX ctx;\n-\tunsigned char sha1[20];\n-\toff_t offset = 0, pack_sig = p->pack_size - 20;\n+\tunsigned char sha1[20], *pack_sig;\n+\toff_t offset = 0, pack_sig_ofs = p->pack_size - 20;\n \tuint32_t nr_objects, i;\n-\tint err;\n+\tint err = 0;\n \tstruct idx_entry *entries;\n \n \t/* Note that the pack header checks are actually performed by\n@@ -38,21 +38,22 @@ static int verify_packfile(struct packed_git *p,\n \t */\n \n \tSHA1_Init(&ctx);\n-\twhile (offset < pack_sig) {\n+\twhile (offset < pack_sig_ofs) {\n \t\tunsigned int remaining;\n \t\tunsigned char *in = use_pack(p, w_curs, offset, &remaining);\n \t\toffset += remaining;\n-\t\tif (offset > pack_sig)\n-\t\t\tremaining -= (unsigned int)(offset - pack_sig);\n+\t\tif (offset > pack_sig_ofs)\n+\t\t\tremaining -= (unsigned int)(offset - pack_sig_ofs);\n \t\tSHA1_Update(&ctx, in, remaining);\n \t}\n \tSHA1_Final(sha1, &ctx);\n-\tif (hashcmp(sha1, use_pack(p, w_curs, pack_sig, NULL)))\n-\t\treturn error(\"Packfile %s SHA1 mismatch with itself\",\n-\t\t\t     p->pack_name);\n-\tif (hashcmp(sha1, index_base + index_size - 40))\n-\t\treturn error(\"Packfile %s SHA1 mismatch with idx\",\n-\t\t\t     p->pack_name);\n+\tpack_sig = use_pack(p, w_curs, pack_sig_ofs, NULL);\n+\tif (hashcmp(sha1, pack_sig))\n+\t\terr = error(\"%s SHA1 checksum mismatch\",\n+\t\t\t    p->pack_name);\n+\tif (hashcmp(index_base + index_size - 40, pack_sig))\n+\t\terr = error(\"%s SHA1 does not match its inddex\",\n+\t\t\t    p->pack_name);\n \tunuse_pack(w_curs);\n \n \t/* Make sure everything reachable from idx is valid.  Since we\n@@ -72,22 +73,23 @@ static int verify_packfile(struct packed_git *p,\n \t}\n \tqsort(entries, nr_objects, sizeof(*entries), compare_entries);\n \n-\tfor (i = 0, err = 0; i < nr_objects; i++) {\n+\tfor (i = 0; i < nr_objects; i++) {\n \t\tvoid *data;\n \t\tenum object_type type;\n \t\tunsigned long size;\n \n \t\tdata = unpack_entry(p, entries[i].offset, &type, &size);\n \t\tif (!data) {\n-\t\t\terr = error(\"cannot unpack %s from %s\",\n-\t\t\t\t    sha1_to_hex(entries[i].sha1), p->pack_name);\n-\t\t\tcontinue;\n+\t\t\terr = error(\"cannot unpack %s from %s at offset %\"PRIuMAX\"\",\n+\t\t\t\t    sha1_to_hex(entries[i].sha1), p->pack_name,\n+\t\t\t\t    (uintmax_t)entries[i].offset);\n+\t\t\tbreak;\n \t\t}\n \t\tif (check_sha1_signature(entries[i].sha1, data, size, typename(type))) {\n \t\t\terr = error(\"packed %s from %s is corrupt\",\n \t\t\t\t    sha1_to_hex(entries[i].sha1), p->pack_name);\n \t\t\tfree(data);\n-\t\t\tcontinue;\n+\t\t\tbreak;\n \t\t}\n \t\tfree(data);\n \t}\n@@ -158,31 +160,28 @@ int verify_pack(struct packed_git *p, int verbose)\n \tconst unsigned char *index_base;\n \tSHA_CTX ctx;\n \tunsigned char sha1[20];\n-\tint ret;\n+\tint err = 0;\n+\tstruct pack_window *w_curs = NULL;\n \n \tif (open_pack_index(p))\n \t\treturn error(\"packfile %s index not opened\", p->pack_name);\n \tindex_size = p->index_size;\n \tindex_base = p->index_data;\n \n-\tret = 0;\n \t/* Verify SHA1 sum of the index file */\n \tSHA1_Init(&ctx);\n \tSHA1_Update(&ctx, index_base, (unsigned int)(index_size - 20));\n \tSHA1_Final(sha1, &ctx);\n \tif (hashcmp(sha1, index_base + index_size - 20))\n-\t\tret = error(\"Packfile index for %s SHA1 mismatch\",\n+\t\terr = error(\"Packfile index for %s SHA1 mismatch\",\n \t\t\t    p->pack_name);\n \n-\tif (!ret) {\n-\t\t/* Verify pack file */\n-\t\tstruct pack_window *w_curs = NULL;\n-\t\tret = verify_packfile(p, &w_curs);\n-\t\tunuse_pack(&w_curs);\n-\t}\n+\t/* Verify pack file */\n+\terr = verify_packfile(p, &w_curs);\n+\tunuse_pack(&w_curs);\n \n \tif (verbose) {\n-\t\tif (ret)\n+\t\tif (err)\n \t\t\tprintf(\"%s: bad\\n\", p->pack_name);\n \t\telse {\n \t\t\tshow_pack_info(p);\n@@ -190,5 +189,5 @@ int verify_pack(struct packed_git *p, int verbose)\n \t\t}\n \t}\n \n-\treturn ret;\n+\treturn err;\n }\n"},{"id":"78337","messageId":"7vskvw8i77.fsf@gitster.siamese.dyndns.org","threadId":"13727","inReplyTo":"alpine.LFD.1.10.0805291716470.23581@xanadu.home","subject":"Re: [PATCH] make verify-pack a bit more useful with bad packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-02T06:27:40Z","receivedAt":"2008-06-02T06:27:40Z","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> When a pack gets corrupted, its SHA1 checksum will fail.  However, this\n> is more useful to let the test go on in order to find the actual \n> problem location than only complain about the SHA1 mismatch and\n> bail out.\n> ...\n>  \t/* Verify SHA1 sum of the index file */\n>  \tSHA1_Init(&ctx);\n>  \tSHA1_Update(&ctx, index_base, (unsigned int)(index_size - 20));\n>  \tSHA1_Final(sha1, &ctx);\n>  \tif (hashcmp(sha1, index_base + index_size - 20))\n> -\t\tret = error(\"Packfile index for %s SHA1 mismatch\",\n> +\t\terr = error(\"Packfile index for %s SHA1 mismatch\",\n>  \t\t\t    p->pack_name);\n>  \n> -\tif (!ret) {\n> -\t\t/* Verify pack file */\n> -\t\tstruct pack_window *w_curs = NULL;\n> -\t\tret = verify_packfile(p, &w_curs);\n> -\t\tunuse_pack(&w_curs);\n> -\t}\n> +\t/* Verify pack file */\n> +\terr = verify_packfile(p, &w_curs);\n\nThis overwrites the index SHA-1 mismatch you detected earlier.  An obvious\nfix-up would be to:\n\n\terr |= verify_packfile(p, &w_curs);\n\nWill be in 1.5.6-rc1.\n"}]}