{"thread":{"id":"15270","subject":"[PATCH 2/3] pack-objects: use fixup_pack_header_footer()'s validation mode","startedAt":"2008-08-29T02:05:56Z","lastAt":"2008-08-31T07:10:37Z","messageCount":16,"participants":["Nicolas Pitre","Shawn O. Pearce","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"89074","messageId":"alpine.LFD.1.10.0808282142490.1624@xanadu.home","threadId":"15270","inReplyTo":"alpine.LFD.1.10.0808271627540.1624@xanadu.home","subject":"[PATCH 0/3] don't let disk corruptions escape pack SHA1 checksum","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T02:05:56Z","receivedAt":"2008-08-29T02:05:56Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 27 Aug 2008, Nicolas Pitre wrote:\n\n> On Wed, 27 Aug 2008, Linus Torvalds wrote:\n> \n> > However, we do have this odd case of fixing up the pack after-the-fact \n> > when we receive it from somebody else (because we get a thin pack and \n> > don't know how many objects the final result will have). And that case \n> > seems to be not as safe, because it\n> > \n> >  - re-reads the file to recompute the SHA1\n> > \n> >    This is understandable, and it's fairly ok, but it does mean that there \n> >    is a bigger chance of the SHA1 matching if something has corrupted the \n> >    file in the meantime!\n> \n> I think that can be fixed.  When reading the file back, it is possible \n> to compute 2 sha1s: one to compare with the recieved one using original \n> pack header, and the second which would be the final one.  FRom a \n> certain offset, new objects were added, so that first sha1 is validated \n> against the received one and reset, and at the end, it should correspond \n> to the sha1 of added objects that we should compute when writing them.\n\nSo here is 3 patches for this in C git (Shawn already did it for jgit).\n\nThere are 3 spots where this needs to be plugged: in pack-objects, \nindex-pack and fast-import.  I did the first two.\n\nI don't think this is that pertinent in fast-import because:\n\n - we want fast-import to be fast\n\n - after fast-import is done, a full repack is typically done which \n   would revalidate everything (maybe fast-import should use index v2 \n   with its per object CRC32 though)\n\nBut maybe I'm wrong about that. In any case, I am leaving the \nfast-import to someone more knowledgeable about its code than I do.\n\nNicolas Pitre (3):\n      improve reliability of fixup_pack_header_footer()\n      pack-objects: use fixup_pack_header_footer()'s validation mode\n      index-pack: use fixup_pack_header_footer()'s validation mode\n\n\nNicolas\n"},{"id":"89075","messageId":"1219975624-7653-1-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"alpine.LFD.1.10.0808282142490.1624@xanadu.home","subject":"[PATCH 1/3] improve reliability of fixup_pack_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T02:07:02Z","receivedAt":"2008-08-29T02:07:02Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Currently, this function has the potential to read corrupted pack data\nfrom disk and give it a valid SHA1 checksum.  Let's add the ability to\nvalidate SHA1 checksum of existing data along the way, including before\nand after any arbitrary point in the pack.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |    3 +-\n fast-import.c          |    3 +-\n index-pack.c           |    3 +-\n pack-write.c           |   78 ++++++++++++++++++++++++++++++++++++++++-------\n pack.h                 |    2 +-\n 5 files changed, 73 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex ef3befe..ec80f14 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -498,7 +498,8 @@ static void write_pack_file(void)\n \t\t\tsha1close(f, sha1, CSUM_FSYNC);\n \t\t} else {\n \t\t\tint fd = sha1close(f, NULL, 0);\n-\t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name, nr_written);\n+\t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name,\n+\t\t\t\t\t\t nr_written, NULL, 0);\n \t\t\tclose(fd);\n \t\t}\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 7089e6f..d85b3a5 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -951,7 +951,8 @@ static void end_packfile(void)\n \n \t\tclose_pack_windows(pack_data);\n \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n-\t\t\t\t    pack_data->pack_name, object_count);\n+\t\t\t\t    pack_data->pack_name, object_count,\n+\t\t\t\t    NULL, 0);\n \t\tclose(pack_data->pack_fd);\n \t\tidx_name = keep_pack(create_index());\n \ndiff --git a/index-pack.c b/index-pack.c\nindex 728af7d..411b80d 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -982,7 +982,8 @@ int main(int argc, char **argv)\n \t\t\t\tnr_objects - nr_objects_initial);\n \t\t\tstop_progress_msg(&progress, msg);\n \t\t\tfixup_pack_header_footer(output_fd, sha1,\n-\t\t\t\t\t\t curr_pack, nr_objects);\n+\t\t\t\t\t\t curr_pack, nr_objects,\n+\t\t\t\t\t\t NULL, 0);\n \t\t}\n \t\tif (nr_deltas != nr_resolved_deltas)\n \t\t\tdie(\"pack has %d unresolved deltas\",\ndiff --git a/pack-write.c b/pack-write.c\nindex ddcfd37..0c0abce 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -144,41 +144,95 @@ char *write_idx_file(char *index_name, struct pack_idx_entry **objects,\n \treturn index_name;\n }\n \n+/*\n+ * Update pack header with object_count and compute new SHA1 for pack data\n+ * associated to pack_fd, and write that SHA1 at the end.  That new SHA1\n+ * is also returned in new_pack_sha1.\n+ *\n+ * If partial_pack_sha1 is non null, then the SHA1 of the existing pack\n+ * (without the header update) is computed and validated against the one\n+ * provided in partial_pack_sha1.  The validation is performed at\n+ * partial_pack_offset bytes in the pack file, or at the end of the pack\n+ * file if partial_pack_offset is zero.  Also, when partial_pack_offset is\n+ * non zero, the SHA1 of the remaining data (i.e. from partial_pack_offset\n+ * to the end) is returned in partial_pack_sha1.\n+ *\n+ * Note that new_pack_sha1 is updated last, so both new_pack_sha1 and\n+ * partial_pack_sha1 can refer to the same buffer if the caller is not\n+ * interested in the resulting SHA1 of pack data above partial_pack_offset.\n+ */\n void fixup_pack_header_footer(int pack_fd,\n-\t\t\t unsigned char *pack_file_sha1,\n+\t\t\t unsigned char *new_pack_sha1,\n \t\t\t const char *pack_name,\n-\t\t\t uint32_t object_count)\n+\t\t\t uint32_t object_count,\n+\t\t\t unsigned char *partial_pack_sha1,\n+\t\t\t off_t partial_pack_offset)\n {\n \tstatic const int buf_sz = 128 * 1024;\n-\tSHA_CTX c;\n+\tSHA_CTX old_sha1_ctx, new_sha1_ctx;\n \tstruct pack_header hdr;\n \tchar *buf;\n \n+\tSHA1_Init(&old_sha1_ctx);\n+\tSHA1_Init(&new_sha1_ctx);\n+\n+\tif (partial_pack_sha1 && !partial_pack_offset) {\n+\t\tpartial_pack_offset = lseek(pack_fd, 0, SEEK_CUR);\n+\t\tif (partial_pack_offset == (off_t)-1)\n+\t\t\tdie(\"Can't get size of %s: %s\", pack_name, strerror(errno));\n+\t}\n+\n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n-\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\t\tdie(\"Failed seeking to start of %s: %s\", pack_name, strerror(errno));\n \tif (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n \t\tdie(\"Unable to reread header of %s: %s\", pack_name, strerror(errno));\n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n-\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\t\tdie(\"Failed seeking to start of %s: %s\", pack_name, strerror(errno));\n+\tSHA1_Update(&old_sha1_ctx, &hdr, sizeof(hdr));\n \thdr.hdr_entries = htonl(object_count);\n+\tSHA1_Update(&new_sha1_ctx, &hdr, sizeof(hdr));\n \twrite_or_die(pack_fd, &hdr, sizeof(hdr));\n-\n-\tSHA1_Init(&c);\n-\tSHA1_Update(&c, &hdr, sizeof(hdr));\n+\tpartial_pack_offset -= sizeof(hdr);\n \n \tbuf = xmalloc(buf_sz);\n \tfor (;;) {\n-\t\tssize_t n = xread(pack_fd, buf, buf_sz);\n+\t\tssize_t m, n;\n+\t\tm = (partial_pack_sha1 && partial_pack_offset < buf_sz) ?\n+\t\t\tpartial_pack_offset : buf_sz;\n+\t\tn = xread(pack_fd, buf, m);\n \t\tif (!n)\n \t\t\tbreak;\n \t\tif (n < 0)\n \t\t\tdie(\"Failed to checksum %s: %s\", pack_name, strerror(errno));\n-\t\tSHA1_Update(&c, buf, n);\n+\t\tSHA1_Update(&new_sha1_ctx, buf, n);\n+\n+\t\tif (!partial_pack_sha1)\n+\t\t\tcontinue;\n+\n+\t\tSHA1_Update(&old_sha1_ctx, buf, n);\n+\t\tpartial_pack_offset -= n;\n+\t\tif (partial_pack_offset == 0) {\n+\t\t\tunsigned char sha1[20];\n+\t\t\tSHA1_Final(sha1, &old_sha1_ctx);\n+\t\t\tif (hashcmp(sha1, partial_pack_sha1) != 0)\n+\t\t\t\tdie(\"Unexpected checksum for %s \"\n+\t\t\t\t    \"(disk corruption?)\", pack_name);\n+\t\t\t/*\n+\t\t\t * Now let's compute the SHA1 of the remainder of the\n+\t\t\t * pack, which also means making partial_pack_offset\n+\t\t\t * big enough not to matter anymore.\n+\t\t\t */\n+\t\t\tSHA1_Init(&old_sha1_ctx);\n+\t\t\tpartial_pack_offset = ~partial_pack_offset;\n+\t\t\tpartial_pack_offset -= MSB(partial_pack_offset, 1);\n+\t\t}\n \t}\n \tfree(buf);\n \n-\tSHA1_Final(pack_file_sha1, &c);\n-\twrite_or_die(pack_fd, pack_file_sha1, 20);\n+\tif (partial_pack_sha1)\n+\t\tSHA1_Final(partial_pack_sha1, &old_sha1_ctx);\n+\tSHA1_Final(new_pack_sha1, &new_sha1_ctx);\n+\twrite_or_die(pack_fd, new_pack_sha1, 20);\n \tfsync_or_die(pack_fd, pack_name);\n }\n \ndiff --git a/pack.h b/pack.h\nindex 76e6aa2..a883334 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -58,7 +58,7 @@ struct pack_idx_entry {\n extern char *write_idx_file(char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n extern int verify_pack(struct packed_git *);\n-extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t);\n+extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\n \n #define PH_ERROR_EOF\t\t(-1)\n-- \n1.6.0.1.174.g97d7e.dirty\n"},{"id":"89073","messageId":"1219975624-7653-2-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"1219975624-7653-1-git-send-email-nico@cam.org","subject":"[PATCH 2/3] pack-objects: use fixup_pack_header_footer()'s validation mode","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T02:07:03Z","receivedAt":"2008-08-29T02:07:03Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"When limiting the pack size, a new header has to be written to the\npack and a new SHA1 computed.  Make sure that the SHA1 of what is being\nread back matches the SHA1 of what was written.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |    4 ++--\n csum-file.c            |    6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex ec80f14..487943e 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -497,9 +497,9 @@ static void write_pack_file(void)\n \t\t} else if (nr_written == nr_remaining) {\n \t\t\tsha1close(f, sha1, CSUM_FSYNC);\n \t\t} else {\n-\t\t\tint fd = sha1close(f, NULL, 0);\n+\t\t\tint fd = sha1close(f, sha1, 0);\n \t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name,\n-\t\t\t\t\t\t nr_written, NULL, 0);\n+\t\t\t\t\t\t nr_written, sha1, 0);\n \t\t\tclose(fd);\n \t\t}\n \ndiff --git a/csum-file.c b/csum-file.c\nindex ace64f1..2838954 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -42,11 +42,11 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n \t\tsha1flush(f, offset);\n \t\tf->offset = 0;\n \t}\n+\tSHA1_Final(f->buffer, &f->ctx);\n+\tif (result)\n+\t\thashcpy(result, f->buffer);\n \tif (flags & (CSUM_CLOSE | CSUM_FSYNC)) {\n \t\t/* write checksum and close fd */\n-\t\tSHA1_Final(f->buffer, &f->ctx);\n-\t\tif (result)\n-\t\t\thashcpy(result, f->buffer);\n \t\tsha1flush(f, 20);\n \t\tif (flags & CSUM_FSYNC)\n \t\t\tfsync_or_die(f->fd, f->name);\n-- \n1.6.0.1.174.g97d7e.dirty\n"},{"id":"89076","messageId":"1219975624-7653-3-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"1219975624-7653-2-git-send-email-nico@cam.org","subject":"[PATCH 3/3] index-pack: use fixup_pack_header_footer()'s validation mode","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T02:07:04Z","receivedAt":"2008-08-29T02:07:04Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"When completing a thin pack, a new header has to be written to\nthe pack and a new SHA1 computed.  Make sure that the SHA1 of what\nis being read back matches the SHA1 of what was written for both:\nthe original pack and the appended objects.\n\nTo do so, a couple write_or_die() calls were converted to sha1write()\nwhich has the advantage of doing some buffering as well as handling\nSHA1 and CRC32 checksum already.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n index-pack.c |   43 +++++++++++++++++++++++++------------------\n 1 files changed, 25 insertions(+), 18 deletions(-)\n\ndiff --git a/index-pack.c b/index-pack.c\nindex 411b80d..a6e91fe 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -654,7 +654,7 @@ static void parse_pack_objects(unsigned char *sha1)\n \t}\n }\n \n-static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_crc)\n+static int write_compressed(struct sha1file *f, void *in, unsigned int size)\n {\n \tz_stream stream;\n \tunsigned long maxsize;\n@@ -674,13 +674,12 @@ static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_c\n \tdeflateEnd(&stream);\n \n \tsize = stream.total_out;\n-\twrite_or_die(fd, out, size);\n-\t*obj_crc = crc32(*obj_crc, out, size);\n+\tsha1write(f, out, size);\n \tfree(out);\n \treturn size;\n }\n \n-static struct object_entry *append_obj_to_pack(\n+static struct object_entry *append_obj_to_pack(struct sha1file *f,\n \t\t\t       const unsigned char *sha1, void *buf,\n \t\t\t       unsigned long size, enum object_type type)\n {\n@@ -696,15 +695,15 @@ static struct object_entry *append_obj_to_pack(\n \t\ts >>= 7;\n \t}\n \theader[n++] = c;\n-\twrite_or_die(output_fd, header, n);\n-\tobj[0].idx.crc32 = crc32(0, Z_NULL, 0);\n-\tobj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);\n+\tcrc32_begin(f);\n+\tsha1write(f, header, n);\n \tobj[0].size = size;\n \tobj[0].hdr_size = n;\n \tobj[0].type = type;\n \tobj[0].real_type = type;\n \tobj[1].idx.offset = obj[0].idx.offset + n;\n-\tobj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);\n+\tobj[1].idx.offset += write_compressed(f, buf, size);\n+\tobj[0].idx.crc32 = crc32_end(f);\n \thashcpy(obj->idx.sha1, sha1);\n \treturn obj;\n }\n@@ -716,7 +715,7 @@ static int delta_pos_compare(const void *_a, const void *_b)\n \treturn a->obj_no - b->obj_no;\n }\n \n-static void fix_unresolved_deltas(int nr_unresolved)\n+static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)\n {\n \tstruct delta_entry **sorted_by_pos;\n \tint i, n = 0;\n@@ -754,8 +753,8 @@ static void fix_unresolved_deltas(int nr_unresolved)\n \t\tif (check_sha1_signature(d->base.sha1, base_obj.data,\n \t\t\t\tbase_obj.size, typename(type)))\n \t\t\tdie(\"local object %s is corrupt\", sha1_to_hex(d->base.sha1));\n-\t\tbase_obj.obj = append_obj_to_pack(d->base.sha1, base_obj.data,\n-\t\t\tbase_obj.size, type);\n+\t\tbase_obj.obj = append_obj_to_pack(f, d->base.sha1,\n+\t\t\t\t\tbase_obj.data, base_obj.size, type);\n \t\tlink_base_data(NULL, &base_obj);\n \n \t\tfind_delta_children(&d->base, &first, &last);\n@@ -875,7 +874,7 @@ int main(int argc, char **argv)\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n \tstruct pack_idx_entry **idx_objects;\n-\tunsigned char sha1[20];\n+\tunsigned char pack_sha1[20];\n \tint nongit = 0;\n \n \tsetup_git_directory_gently(&nongit);\n@@ -962,13 +961,15 @@ int main(int argc, char **argv)\n \tparse_pack_header();\n \tobjects = xmalloc((nr_objects + 1) * sizeof(struct object_entry));\n \tdeltas = xmalloc(nr_objects * sizeof(struct delta_entry));\n-\tparse_pack_objects(sha1);\n+\tparse_pack_objects(pack_sha1);\n \tif (nr_deltas == nr_resolved_deltas) {\n \t\tstop_progress(&progress);\n \t\t/* Flush remaining pack final 20-byte SHA1. */\n \t\tflush();\n \t} else {\n \t\tif (fix_thin_pack) {\n+\t\t\tstruct sha1file *f;\n+\t\t\tunsigned char read_sha1[20], tail_sha1[20];\n \t\t\tchar msg[48];\n \t\t\tint nr_unresolved = nr_deltas - nr_resolved_deltas;\n \t\t\tint nr_objects_initial = nr_objects;\n@@ -977,13 +978,19 @@ int main(int argc, char **argv)\n \t\t\tobjects = xrealloc(objects,\n \t\t\t\t\t   (nr_objects + nr_unresolved + 1)\n \t\t\t\t\t   * sizeof(*objects));\n-\t\t\tfix_unresolved_deltas(nr_unresolved);\n+\t\t\tf = sha1fd(output_fd, curr_pack);\n+\t\t\tfix_unresolved_deltas(f, nr_unresolved);\n \t\t\tsprintf(msg, \"completed with %d local objects\",\n \t\t\t\tnr_objects - nr_objects_initial);\n \t\t\tstop_progress_msg(&progress, msg);\n-\t\t\tfixup_pack_header_footer(output_fd, sha1,\n+\t\t\tsha1close(f, tail_sha1, 0);\n+\t\t\thashcpy(read_sha1, pack_sha1);\n+\t\t\tfixup_pack_header_footer(output_fd, pack_sha1,\n \t\t\t\t\t\t curr_pack, nr_objects,\n-\t\t\t\t\t\t NULL, 0);\n+\t\t\t\t\t\t read_sha1, consumed_bytes-20);\n+\t\t\tif (hashcmp(read_sha1, tail_sha1) != 0)\n+\t\t\t\tdie(\"Unexpected tail checksum for %s \"\n+\t\t\t\t    \"(disk corruption?)\", curr_pack);\n \t\t}\n \t\tif (nr_deltas != nr_resolved_deltas)\n \t\t\tdie(\"pack has %d unresolved deltas\",\n@@ -996,13 +1003,13 @@ int main(int argc, char **argv)\n \tidx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));\n \tfor (i = 0; i < nr_objects; i++)\n \t\tidx_objects[i] = &objects[i].idx;\n-\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, sha1);\n+\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, pack_sha1);\n \tfree(idx_objects);\n \n \tfinal(pack_name, curr_pack,\n \t\tindex_name, curr_index,\n \t\tkeep_name, keep_msg,\n-\t\tsha1);\n+\t\tpack_sha1);\n \tfree(objects);\n \tfree(index_name_buf);\n \tfree(keep_name_buf);\n-- \n1.6.0.1.174.g97d7e.dirty\n"},{"id":"89081","messageId":"20080829044459.GA28492@spearce.org","threadId":"15270","inReplyTo":"1219975624-7653-1-git-send-email-nico@cam.org","subject":"Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-29T04:44:59Z","receivedAt":"2008-08-29T04:44:59Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> Currently, this function has the potential to read corrupted pack data\n> from disk and give it a valid SHA1 checksum.  Let's add the ability to\n> validate SHA1 checksum of existing data along the way, including before\n> and after any arbitrary point in the pack.\n\nI found your implementation in fixup_pack_header to be very\ncontorted.  Did you read the JGit patch I posted?  I think its\nimplementation is more elegant and easier to follow.  Oh, and its\nBSD licensed... so easy for a GPLv2 project to borrow.  ;-)\n \n>  void fixup_pack_header_footer(int pack_fd,\n...\n> +\tif (partial_pack_sha1 && !partial_pack_offset) {\n> +\t\tpartial_pack_offset = lseek(pack_fd, 0, SEEK_CUR);\n> +\t\tif (partial_pack_offset == (off_t)-1)\n\nEh?\n\nI find this to be nonsense.  If the caller gave us a SHA-1 but\nwants us to do fixup then they have increased the size of the pack.\nWhich means they must pass us the original length.  Computing it\nhere is relying on the caller leaving the file pointer positioned\nat the old end.  Who the heck does that after fixing a thin pack?\n\n>  \tbuf = xmalloc(buf_sz);\n>  \tfor (;;) {\n> -\t\tssize_t n = xread(pack_fd, buf, buf_sz);\n> +\t\tssize_t m, n;\n> +\t\tm = (partial_pack_sha1 && partial_pack_offset < buf_sz) ?\n> +\t\t\tpartial_pack_offset : buf_sz;\n> +\t\tn = xread(pack_fd, buf, m);\n\nWhy not read the full buf_sz per round and then use only part of\nthe buffer in SHA1_Update(&old_sha1_ctx)?  It took me a while to\nfigure out what the heck you were trying to measure here.  It also\nwould be a few less CPU instructions if you always just read buf_sz\nand leave the min test to after the \"if (!partial_pack_sha1)\" below.\n\n> +\t\tif (!partial_pack_sha1)\n> +\t\t\tcontinue;\n> +\n> +\t\tSHA1_Update(&old_sha1_ctx, buf, n);\n> +\t\tpartial_pack_offset -= n;\n> +\t\tif (partial_pack_offset == 0) {\n> +\t\t\tunsigned char sha1[20];\n> +\t\t\tSHA1_Final(sha1, &old_sha1_ctx);\n> +\t\t\tif (hashcmp(sha1, partial_pack_sha1) != 0)\n> +\t\t\t\tdie(\"Unexpected checksum for %s \"\n> +\t\t\t\t    \"(disk corruption?)\", pack_name);\n> +\t\t\t/*\n> +\t\t\t * Now let's compute the SHA1 of the remainder of the\n> +\t\t\t * pack, which also means making partial_pack_offset\n> +\t\t\t * big enough not to matter anymore.\n> +\t\t\t */\n> +\t\t\tSHA1_Init(&old_sha1_ctx);\n> +\t\t\tpartial_pack_offset = ~partial_pack_offset;\n> +\t\t\tpartial_pack_offset -= MSB(partial_pack_offset, 1);\n\nThat's freaking brilliant.  And something I missed in JGit.\n\nThe way its implemented is downright difficult to follow though.\nI'll admit it took me a good 10 minutes to understand what you were\ndoing here, and why.\n\n-- \nShawn.\n"},{"id":"89082","messageId":"20080829045535.GB28492@spearce.org","threadId":"15270","inReplyTo":"alpine.LFD.1.10.0808282142490.1624@xanadu.home","subject":"Re: [PATCH 0/3] don't let disk corruptions escape pack SHA1 checksum","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-29T04:55:35Z","receivedAt":"2008-08-29T04:55:35Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> \n> So here is 3 patches for this in C git (Shawn already did it for jgit).\n> \n> There are 3 spots where this needs to be plugged: in pack-objects, \n> index-pack and fast-import.  I did the first two.\n> \n> I don't think this is that pertinent in fast-import because:\n> \n>  - we want fast-import to be fast\n> \n>  - after fast-import is done, a full repack is typically done which \n>    would revalidate everything (maybe fast-import should use index v2 \n>    with its per object CRC32 though)\n> \n> But maybe I'm wrong about that. In any case, I am leaving the \n> fast-import to someone more knowledgeable about its code than I do.\n\nfast-import is the backend to a number of incremental import tools.\nThey stream data in from foreign VCS systems (notably Perforce)\nand write into a live repository.  That repository may not get\nrepacked for days/weeks.  We should treat it just like index-pack.\n\nSo I think we should plug it.  The cost to keep a running SHA-1\nfor what we have written is pretty low.  I'd rather trade off a\nminute or so on an hour long import for data safety than have a\nchance for data corruption going unnoticed by the end user.\n\nAnd now that index v2 is available, yea, fast-import should write\nits index in that format so that a later repack can safely delta\nreuse if it (by rare chance) decides the current packed form is the\nbest representation.  This is especially true with \"git gc --auto\"\ntriggering a repack every so often due to the incremental import\npacks I just metioned above.\n\nI've just been too busy with other things to add index v2 support.\n\n-- \nShawn.\n"},{"id":"89130","messageId":"alpine.LFD.1.10.0808290844200.1624@xanadu.home","threadId":"15270","inReplyTo":"20080829044459.GA28492@spearce.org","subject":"Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T13:08:48Z","receivedAt":"2008-08-29T13:08:48Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 28 Aug 2008, Shawn O. Pearce wrote:\n\n> Nicolas Pitre <nico@cam.org> wrote:\n> > Currently, this function has the potential to read corrupted pack data\n> > from disk and give it a valid SHA1 checksum.  Let's add the ability to\n> > validate SHA1 checksum of existing data along the way, including before\n> > and after any arbitrary point in the pack.\n> \n> I found your implementation in fixup_pack_header to be very\n> contorted.  Did you read the JGit patch I posted?  I think its\n> implementation is more elegant and easier to follow.  Oh, and its\n> BSD licensed... so easy for a GPLv2 project to borrow.  ;-)\n\nWell, actually, I don't find the JGit implementation easier to follow at \nall.  Of course I wrote the C version while you wrote the Java version.  \nMaybe without our respective bias then things are just fine in both \ncases.\n\n> >  void fixup_pack_header_footer(int pack_fd,\n> ...\n> > +\tif (partial_pack_sha1 && !partial_pack_offset) {\n> > +\t\tpartial_pack_offset = lseek(pack_fd, 0, SEEK_CUR);\n> > +\t\tif (partial_pack_offset == (off_t)-1)\n> \n> Eh?\n> \n> I find this to be nonsense.  If the caller gave us a SHA-1 but\n> wants us to do fixup then they have increased the size of the pack.\n> Which means they must pass us the original length.  Computing it\n> here is relying on the caller leaving the file pointer positioned\n> at the old end.  Who the heck does that after fixing a thin pack?\n\nThis is not for thin packs but for split packs in repack-objects. And \nyeah, the caller has the offset information already in that case too, so \nthis could be removed.  It just felt more generic that way originally.\n\n> >  \tbuf = xmalloc(buf_sz);\n> >  \tfor (;;) {\n> > -\t\tssize_t n = xread(pack_fd, buf, buf_sz);\n> > +\t\tssize_t m, n;\n> > +\t\tm = (partial_pack_sha1 && partial_pack_offset < buf_sz) ?\n> > +\t\t\tpartial_pack_offset : buf_sz;\n> > +\t\tn = xread(pack_fd, buf, m);\n> \n> Why not read the full buf_sz per round and then use only part of\n> the buffer in SHA1_Update(&old_sha1_ctx)?  It took me a while to\n> figure out what the heck you were trying to measure here.  It also\n> would be a few less CPU instructions if you always just read buf_sz\n> and leave the min test to after the \"if (!partial_pack_sha1)\" below.\n\nI doubt it.  That would only move the offset test from before the read \nto after it, and actually complicate things for restarting the \nchecksumming after the special offset was crossed.  And we're talking \nabout a comparison with a conditional move instructions only.\n\nAnd another thing I had in store (but for which you _again_ beat me to :-) )\nis to realign data reads onto filesystem blocks.\n\n> > +\t\tif (!partial_pack_sha1)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tSHA1_Update(&old_sha1_ctx, buf, n);\n> > +\t\tpartial_pack_offset -= n;\n> > +\t\tif (partial_pack_offset == 0) {\n> > +\t\t\tunsigned char sha1[20];\n> > +\t\t\tSHA1_Final(sha1, &old_sha1_ctx);\n> > +\t\t\tif (hashcmp(sha1, partial_pack_sha1) != 0)\n> > +\t\t\t\tdie(\"Unexpected checksum for %s \"\n> > +\t\t\t\t    \"(disk corruption?)\", pack_name);\n> > +\t\t\t/*\n> > +\t\t\t * Now let's compute the SHA1 of the remainder of the\n> > +\t\t\t * pack, which also means making partial_pack_offset\n> > +\t\t\t * big enough not to matter anymore.\n> > +\t\t\t */\n> > +\t\t\tSHA1_Init(&old_sha1_ctx);\n> > +\t\t\tpartial_pack_offset = ~partial_pack_offset;\n> > +\t\t\tpartial_pack_offset -= MSB(partial_pack_offset, 1);\n> \n> That's freaking brilliant.  And something I missed in JGit.\n> \n> The way its implemented is downright difficult to follow though.\n> I'll admit it took me a good 10 minutes to understand what you were\n> doing here, and why.\n\nAgain maybe that's just because you're just too biased by your own \nimplementation.  I don't consider this code particularly obscur.\n\n\nNicolas\n"},{"id":"89120","messageId":"20080829143023.GA7403@spearce.org","threadId":"15270","inReplyTo":"alpine.LFD.1.10.0808290844200.1624@xanadu.home","subject":"Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-29T14:30:23Z","receivedAt":"2008-08-29T14:30:23Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> On Thu, 28 Aug 2008, Shawn O. Pearce wrote:\n> > \n> > I found your implementation in fixup_pack_header to be very\n> > contorted.  Did you read the JGit patch I posted?\n> \n> Well, actually, I don't find the JGit implementation easier to follow at \n> all.  Of course I wrote the C version while you wrote the Java version.  \n> Maybe without our respective bias then things are just fine in both \n> cases.\n\nI probably should have gone and edited my eariler comments after I\nreached the end of the patch and the light dawned about why you are\nsumming the tail.  Most of what I found to be complex in your code\nwas just to handle that boundary condition at partial_pack_offset\nand restart the checksum for the tail.  But I honestly cannot see\nan easier way to write that, so what you have is just fine.\n \n> > >  void fixup_pack_header_footer(int pack_fd,\n> > ...\n> > > +\tif (partial_pack_sha1 && !partial_pack_offset) {\n> > > +\t\tpartial_pack_offset = lseek(pack_fd, 0, SEEK_CUR);\n> > > +\t\tif (partial_pack_offset == (off_t)-1)\n> > \n> > Eh?\n> > \n> > I find this to be nonsense.\n> \n> This is not for thin packs but for split packs in repack-objects. And \n> yeah, the caller has the offset information already in that case too, so \n> this could be removed.  It just felt more generic that way originally.\n\nOh, yea, that makes sense.  It still seems like playing with fire.\n\nI'd rather the caller pass in the proper offset than rely on it\nbeing the current position of the fd.  Especially if the caller\ndoes actually have it available.\n\nIf you change anything, I'd like to see this lseek(SEEK_CUR) go away.\n \n> And another thing I had in store (but for which you _again_ beat me to :-) )\n> is to realign data reads onto filesystem blocks.\n\nThat _really_ made the JGit code ugly.  But I think its worth it.\n\nI also want to try and buffer the whole object appending we do\nduring fixThinPack(), as right now we write the object header in\none write and then compressed data bursts in the others.  Moving it\nto at least write a full 4k at a time should remove about 2 write\ncalls per object.\n \n> > That's freaking brilliant.  And something I missed in JGit.\n> > The way its implemented is downright difficult to follow though.\n> > I'll admit it took me a good 10 minutes to understand what you were\n> > doing here, and why.\n> \n> Again maybe that's just because you're just too biased by your own \n> implementation.  I don't consider this code particularly obscur.\n\nMy own code in JGit got \"obscure\" when I added this check too.\nI take back the remark above.\n\n-- \nShawn.\n"},{"id":"89153","messageId":"1220040482-10108-1-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"20080829143023.GA7403@spearce.org","subject":"[PATCH 0/5] pack header rewriting improvements","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T20:07:57Z","receivedAt":"2008-08-29T20:07:57Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"This replaces my earlier 3-patch series called \"don't let disk\ncorruptions escape pack SHA1 checksum\" with this set.  Same purpose\nbut better!\n\nOnce again I'm leaving fast-import out.  I'm sure Shawn will figure\nit out much faster than I could.\n\n\nNicolas\n"},{"id":"89154","messageId":"1220040482-10108-2-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"1220040482-10108-1-git-send-email-nico@cam.org","subject":"[PATCH 1/5] pack-objects: improve returned information from write_one()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T20:07:58Z","receivedAt":"2008-08-29T20:07:58Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"This function returns 0 when the current object couldn't be written\ndue to the pack size limit, otherwise the current offset in the pack.\nThere is a problem with this approach however, since current object\ncould be a delta and its delta base might just have been written in\nthe same write_one() call, but those successfully written objects are\nnot accounted in the offset variable tracked by the caller. Currently\nthis is not an issue but a subsequent patch will need this.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |   29 ++++++++++++-----------------\n 1 files changed, 12 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex ef3befe..a615fcc 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -410,25 +410,22 @@ static unsigned long write_object(struct sha1file *f,\n \treturn hdrlen + datalen;\n }\n \n-static off_t write_one(struct sha1file *f,\n+static int write_one(struct sha1file *f,\n \t\t\t       struct object_entry *e,\n-\t\t\t       off_t offset)\n+\t\t\t       off_t *offset)\n {\n \tunsigned long size;\n \n \t/* offset is non zero if object is written already. */\n \tif (e->idx.offset || e->preferred_base)\n-\t\treturn offset;\n+\t\treturn 1;\n \n \t/* if we are deltified, write out base object first. */\n-\tif (e->delta) {\n-\t\toffset = write_one(f, e->delta, offset);\n-\t\tif (!offset)\n-\t\t\treturn 0;\n-\t}\n+\tif (e->delta && !write_one(f, e->delta, offset))\n+\t\treturn 0;\n \n-\te->idx.offset = offset;\n-\tsize = write_object(f, e, offset);\n+\te->idx.offset = *offset;\n+\tsize = write_object(f, e, *offset);\n \tif (!size) {\n \t\te->idx.offset = 0;\n \t\treturn 0;\n@@ -436,9 +433,10 @@ static off_t write_one(struct sha1file *f,\n \twritten_list[nr_written++] = &e->idx;\n \n \t/* make sure off_t is sufficiently large not to wrap */\n-\tif (offset > offset + size)\n+\tif (*offset > *offset + size)\n \t\tdie(\"pack too large for current definition of off_t\");\n-\treturn offset + size;\n+\t*offset += size;\n+\treturn 1;\n }\n \n /* forward declaration for write_pack_file */\n@@ -448,7 +446,7 @@ static void write_pack_file(void)\n {\n \tuint32_t i = 0, j;\n \tstruct sha1file *f;\n-\toff_t offset, offset_one, last_obj_offset = 0;\n+\toff_t offset;\n \tstruct pack_header hdr;\n \tuint32_t nr_remaining = nr_result;\n \ttime_t last_mtime = 0;\n@@ -480,11 +478,8 @@ static void write_pack_file(void)\n \t\toffset = sizeof(hdr);\n \t\tnr_written = 0;\n \t\tfor (; i < nr_objects; i++) {\n-\t\t\tlast_obj_offset = offset;\n-\t\t\toffset_one = write_one(f, objects + i, offset);\n-\t\t\tif (!offset_one)\n+\t\t\tif (!write_one(f, objects + i, &offset))\n \t\t\t\tbreak;\n-\t\t\toffset = offset_one;\n \t\t\tdisplay_progress(progress_state, written);\n \t\t}\n \n-- \n1.6.0.1.212.g35f9f\n"},{"id":"89155","messageId":"1220040482-10108-3-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"1220040482-10108-2-git-send-email-nico@cam.org","subject":"[PATCH 2/5] improve reliability of fixup_pack_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T20:07:59Z","receivedAt":"2008-08-29T20:07:59Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Currently, this function has the potential to read corrupted pack data\nfrom disk and give it a valid SHA1 checksum.  Let's add the ability to\nvalidate SHA1 checksum of existing data along the way, including before\nand after any arbitrary point in the pack.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |    3 +-\n fast-import.c          |    3 +-\n index-pack.c           |    3 +-\n pack-write.c           |   71 +++++++++++++++++++++++++++++++++++++++--------\n pack.h                 |    2 +-\n 5 files changed, 66 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex a615fcc..79417cb 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -493,7 +493,8 @@ static void write_pack_file(void)\n \t\t\tsha1close(f, sha1, CSUM_FSYNC);\n \t\t} else {\n \t\t\tint fd = sha1close(f, NULL, 0);\n-\t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name, nr_written);\n+\t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name,\n+\t\t\t\t\t\t nr_written, NULL, 0);\n \t\t\tclose(fd);\n \t\t}\n \ndiff --git a/fast-import.c b/fast-import.c\nindex 7089e6f..d85b3a5 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -951,7 +951,8 @@ static void end_packfile(void)\n \n \t\tclose_pack_windows(pack_data);\n \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n-\t\t\t\t    pack_data->pack_name, object_count);\n+\t\t\t\t    pack_data->pack_name, object_count,\n+\t\t\t\t    NULL, 0);\n \t\tclose(pack_data->pack_fd);\n \t\tidx_name = keep_pack(create_index());\n \ndiff --git a/index-pack.c b/index-pack.c\nindex 728af7d..411b80d 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -982,7 +982,8 @@ int main(int argc, char **argv)\n \t\t\t\tnr_objects - nr_objects_initial);\n \t\t\tstop_progress_msg(&progress, msg);\n \t\t\tfixup_pack_header_footer(output_fd, sha1,\n-\t\t\t\t\t\t curr_pack, nr_objects);\n+\t\t\t\t\t\t curr_pack, nr_objects,\n+\t\t\t\t\t\t NULL, 0);\n \t\t}\n \t\tif (nr_deltas != nr_resolved_deltas)\n \t\t\tdie(\"pack has %d unresolved deltas\",\ndiff --git a/pack-write.c b/pack-write.c\nindex ddcfd37..f9776c5 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -144,41 +144,88 @@ char *write_idx_file(char *index_name, struct pack_idx_entry **objects,\n \treturn index_name;\n }\n \n+/*\n+ * Update pack header with object_count and compute new SHA1 for pack data\n+ * associated to pack_fd, and write that SHA1 at the end.  That new SHA1\n+ * is also returned in new_pack_sha1.\n+ *\n+ * If partial_pack_sha1 is non null, then the SHA1 of the existing pack\n+ * (without the header update) is computed and validated against the\n+ * one provided in partial_pack_sha1.  The validation is performed at\n+ * partial_pack_offset bytes in the pack file.  The SHA1 of the remaining\n+ * data (i.e. from partial_pack_offset to the end) is then computed and\n+ * returned in partial_pack_sha1.\n+ *\n+ * Note that new_pack_sha1 is updated last, so both new_pack_sha1 and\n+ * partial_pack_sha1 can refer to the same buffer if the caller is not\n+ * interested in the resulting SHA1 of pack data above partial_pack_offset.\n+ */\n void fixup_pack_header_footer(int pack_fd,\n-\t\t\t unsigned char *pack_file_sha1,\n+\t\t\t unsigned char *new_pack_sha1,\n \t\t\t const char *pack_name,\n-\t\t\t uint32_t object_count)\n+\t\t\t uint32_t object_count,\n+\t\t\t unsigned char *partial_pack_sha1,\n+\t\t\t off_t partial_pack_offset)\n {\n \tstatic const int buf_sz = 128 * 1024;\n-\tSHA_CTX c;\n+\tSHA_CTX old_sha1_ctx, new_sha1_ctx;\n \tstruct pack_header hdr;\n \tchar *buf;\n \n+\tSHA1_Init(&old_sha1_ctx);\n+\tSHA1_Init(&new_sha1_ctx);\n+\n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n-\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\t\tdie(\"Failed seeking to start of %s: %s\", pack_name, strerror(errno));\n \tif (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n \t\tdie(\"Unable to reread header of %s: %s\", pack_name, strerror(errno));\n \tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n-\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\t\tdie(\"Failed seeking to start of %s: %s\", pack_name, strerror(errno));\n+\tSHA1_Update(&old_sha1_ctx, &hdr, sizeof(hdr));\n \thdr.hdr_entries = htonl(object_count);\n+\tSHA1_Update(&new_sha1_ctx, &hdr, sizeof(hdr));\n \twrite_or_die(pack_fd, &hdr, sizeof(hdr));\n-\n-\tSHA1_Init(&c);\n-\tSHA1_Update(&c, &hdr, sizeof(hdr));\n+\tpartial_pack_offset -= sizeof(hdr);\n \n \tbuf = xmalloc(buf_sz);\n \tfor (;;) {\n-\t\tssize_t n = xread(pack_fd, buf, buf_sz);\n+\t\tssize_t m, n;\n+\t\tm = (partial_pack_sha1 && partial_pack_offset < buf_sz) ?\n+\t\t\tpartial_pack_offset : buf_sz;\n+\t\tn = xread(pack_fd, buf, m);\n \t\tif (!n)\n \t\t\tbreak;\n \t\tif (n < 0)\n \t\t\tdie(\"Failed to checksum %s: %s\", pack_name, strerror(errno));\n-\t\tSHA1_Update(&c, buf, n);\n+\t\tSHA1_Update(&new_sha1_ctx, buf, n);\n+\n+\t\tif (!partial_pack_sha1)\n+\t\t\tcontinue;\n+\n+\t\tSHA1_Update(&old_sha1_ctx, buf, n);\n+\t\tpartial_pack_offset -= n;\n+\t\tif (partial_pack_offset == 0) {\n+\t\t\tunsigned char sha1[20];\n+\t\t\tSHA1_Final(sha1, &old_sha1_ctx);\n+\t\t\tif (hashcmp(sha1, partial_pack_sha1) != 0)\n+\t\t\t\tdie(\"Unexpected checksum for %s \"\n+\t\t\t\t    \"(disk corruption?)\", pack_name);\n+\t\t\t/*\n+\t\t\t * Now let's compute the SHA1 of the remainder of the\n+\t\t\t * pack, which also means making partial_pack_offset\n+\t\t\t * big enough not to matter anymore.\n+\t\t\t */\n+\t\t\tSHA1_Init(&old_sha1_ctx);\n+\t\t\tpartial_pack_offset = ~partial_pack_offset;\n+\t\t\tpartial_pack_offset -= MSB(partial_pack_offset, 1);\n+\t\t}\n \t}\n \tfree(buf);\n \n-\tSHA1_Final(pack_file_sha1, &c);\n-\twrite_or_die(pack_fd, pack_file_sha1, 20);\n+\tif (partial_pack_sha1)\n+\t\tSHA1_Final(partial_pack_sha1, &old_sha1_ctx);\n+\tSHA1_Final(new_pack_sha1, &new_sha1_ctx);\n+\twrite_or_die(pack_fd, new_pack_sha1, 20);\n \tfsync_or_die(pack_fd, pack_name);\n }\n \ndiff --git a/pack.h b/pack.h\nindex 76e6aa2..a883334 100644\n--- a/pack.h\n+++ b/pack.h\n@@ -58,7 +58,7 @@ struct pack_idx_entry {\n extern char *write_idx_file(char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1);\n extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr);\n extern int verify_pack(struct packed_git *);\n-extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t);\n+extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t);\n extern char *index_pack_lockfile(int fd);\n \n #define PH_ERROR_EOF\t\t(-1)\n-- \n1.6.0.1.212.g35f9f\n"},{"id":"89156","messageId":"1220040482-10108-4-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"1220040482-10108-3-git-send-email-nico@cam.org","subject":"[PATCH 3/5] pack-objects: use fixup_pack_header_footer()'s validation mode","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T20:08:00Z","receivedAt":"2008-08-29T20:08:00Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"When limiting the pack size, a new header has to be written to the\npack and a new SHA1 computed.  Make sure that the SHA1 of what is being\nread back matches the SHA1 of what was written.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n builtin-pack-objects.c |    4 ++--\n csum-file.c            |    6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 79417cb..ba2cf00 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -492,9 +492,9 @@ static void write_pack_file(void)\n \t\t} else if (nr_written == nr_remaining) {\n \t\t\tsha1close(f, sha1, CSUM_FSYNC);\n \t\t} else {\n-\t\t\tint fd = sha1close(f, NULL, 0);\n+\t\t\tint fd = sha1close(f, sha1, 0);\n \t\t\tfixup_pack_header_footer(fd, sha1, pack_tmp_name,\n-\t\t\t\t\t\t nr_written, NULL, 0);\n+\t\t\t\t\t\t nr_written, sha1, offset);\n \t\t\tclose(fd);\n \t\t}\n \ndiff --git a/csum-file.c b/csum-file.c\nindex ace64f1..2838954 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -42,11 +42,11 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags)\n \t\tsha1flush(f, offset);\n \t\tf->offset = 0;\n \t}\n+\tSHA1_Final(f->buffer, &f->ctx);\n+\tif (result)\n+\t\thashcpy(result, f->buffer);\n \tif (flags & (CSUM_CLOSE | CSUM_FSYNC)) {\n \t\t/* write checksum and close fd */\n-\t\tSHA1_Final(f->buffer, &f->ctx);\n-\t\tif (result)\n-\t\t\thashcpy(result, f->buffer);\n \t\tsha1flush(f, 20);\n \t\tif (flags & CSUM_FSYNC)\n \t\t\tfsync_or_die(f->fd, f->name);\n-- \n1.6.0.1.212.g35f9f\n"},{"id":"89157","messageId":"1220040482-10108-5-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"1220040482-10108-4-git-send-email-nico@cam.org","subject":"[PATCH 4/5] index-pack: use fixup_pack_header_footer()'s validation mode","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T20:08:01Z","receivedAt":"2008-08-29T20:08:01Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"When completing a thin pack, a new header has to be written to\nthe pack and a new SHA1 computed.  Make sure that the SHA1 of what\nis being read back matches the SHA1 of what was written for both:\nthe original pack and the appended objects.\n\nTo do so, a couple write_or_die() calls were converted to sha1write()\nwhich has the advantage of doing some buffering as well as handling\nSHA1 and CRC32 checksum already.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n index-pack.c |   43 +++++++++++++++++++++++++------------------\n 1 files changed, 25 insertions(+), 18 deletions(-)\n\ndiff --git a/index-pack.c b/index-pack.c\nindex 411b80d..a6e91fe 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -654,7 +654,7 @@ static void parse_pack_objects(unsigned char *sha1)\n \t}\n }\n \n-static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_crc)\n+static int write_compressed(struct sha1file *f, void *in, unsigned int size)\n {\n \tz_stream stream;\n \tunsigned long maxsize;\n@@ -674,13 +674,12 @@ static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_c\n \tdeflateEnd(&stream);\n \n \tsize = stream.total_out;\n-\twrite_or_die(fd, out, size);\n-\t*obj_crc = crc32(*obj_crc, out, size);\n+\tsha1write(f, out, size);\n \tfree(out);\n \treturn size;\n }\n \n-static struct object_entry *append_obj_to_pack(\n+static struct object_entry *append_obj_to_pack(struct sha1file *f,\n \t\t\t       const unsigned char *sha1, void *buf,\n \t\t\t       unsigned long size, enum object_type type)\n {\n@@ -696,15 +695,15 @@ static struct object_entry *append_obj_to_pack(\n \t\ts >>= 7;\n \t}\n \theader[n++] = c;\n-\twrite_or_die(output_fd, header, n);\n-\tobj[0].idx.crc32 = crc32(0, Z_NULL, 0);\n-\tobj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n);\n+\tcrc32_begin(f);\n+\tsha1write(f, header, n);\n \tobj[0].size = size;\n \tobj[0].hdr_size = n;\n \tobj[0].type = type;\n \tobj[0].real_type = type;\n \tobj[1].idx.offset = obj[0].idx.offset + n;\n-\tobj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32);\n+\tobj[1].idx.offset += write_compressed(f, buf, size);\n+\tobj[0].idx.crc32 = crc32_end(f);\n \thashcpy(obj->idx.sha1, sha1);\n \treturn obj;\n }\n@@ -716,7 +715,7 @@ static int delta_pos_compare(const void *_a, const void *_b)\n \treturn a->obj_no - b->obj_no;\n }\n \n-static void fix_unresolved_deltas(int nr_unresolved)\n+static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved)\n {\n \tstruct delta_entry **sorted_by_pos;\n \tint i, n = 0;\n@@ -754,8 +753,8 @@ static void fix_unresolved_deltas(int nr_unresolved)\n \t\tif (check_sha1_signature(d->base.sha1, base_obj.data,\n \t\t\t\tbase_obj.size, typename(type)))\n \t\t\tdie(\"local object %s is corrupt\", sha1_to_hex(d->base.sha1));\n-\t\tbase_obj.obj = append_obj_to_pack(d->base.sha1, base_obj.data,\n-\t\t\tbase_obj.size, type);\n+\t\tbase_obj.obj = append_obj_to_pack(f, d->base.sha1,\n+\t\t\t\t\tbase_obj.data, base_obj.size, type);\n \t\tlink_base_data(NULL, &base_obj);\n \n \t\tfind_delta_children(&d->base, &first, &last);\n@@ -875,7 +874,7 @@ int main(int argc, char **argv)\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n \tstruct pack_idx_entry **idx_objects;\n-\tunsigned char sha1[20];\n+\tunsigned char pack_sha1[20];\n \tint nongit = 0;\n \n \tsetup_git_directory_gently(&nongit);\n@@ -962,13 +961,15 @@ int main(int argc, char **argv)\n \tparse_pack_header();\n \tobjects = xmalloc((nr_objects + 1) * sizeof(struct object_entry));\n \tdeltas = xmalloc(nr_objects * sizeof(struct delta_entry));\n-\tparse_pack_objects(sha1);\n+\tparse_pack_objects(pack_sha1);\n \tif (nr_deltas == nr_resolved_deltas) {\n \t\tstop_progress(&progress);\n \t\t/* Flush remaining pack final 20-byte SHA1. */\n \t\tflush();\n \t} else {\n \t\tif (fix_thin_pack) {\n+\t\t\tstruct sha1file *f;\n+\t\t\tunsigned char read_sha1[20], tail_sha1[20];\n \t\t\tchar msg[48];\n \t\t\tint nr_unresolved = nr_deltas - nr_resolved_deltas;\n \t\t\tint nr_objects_initial = nr_objects;\n@@ -977,13 +978,19 @@ int main(int argc, char **argv)\n \t\t\tobjects = xrealloc(objects,\n \t\t\t\t\t   (nr_objects + nr_unresolved + 1)\n \t\t\t\t\t   * sizeof(*objects));\n-\t\t\tfix_unresolved_deltas(nr_unresolved);\n+\t\t\tf = sha1fd(output_fd, curr_pack);\n+\t\t\tfix_unresolved_deltas(f, nr_unresolved);\n \t\t\tsprintf(msg, \"completed with %d local objects\",\n \t\t\t\tnr_objects - nr_objects_initial);\n \t\t\tstop_progress_msg(&progress, msg);\n-\t\t\tfixup_pack_header_footer(output_fd, sha1,\n+\t\t\tsha1close(f, tail_sha1, 0);\n+\t\t\thashcpy(read_sha1, pack_sha1);\n+\t\t\tfixup_pack_header_footer(output_fd, pack_sha1,\n \t\t\t\t\t\t curr_pack, nr_objects,\n-\t\t\t\t\t\t NULL, 0);\n+\t\t\t\t\t\t read_sha1, consumed_bytes-20);\n+\t\t\tif (hashcmp(read_sha1, tail_sha1) != 0)\n+\t\t\t\tdie(\"Unexpected tail checksum for %s \"\n+\t\t\t\t    \"(disk corruption?)\", curr_pack);\n \t\t}\n \t\tif (nr_deltas != nr_resolved_deltas)\n \t\t\tdie(\"pack has %d unresolved deltas\",\n@@ -996,13 +1003,13 @@ int main(int argc, char **argv)\n \tidx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));\n \tfor (i = 0; i < nr_objects; i++)\n \t\tidx_objects[i] = &objects[i].idx;\n-\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, sha1);\n+\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, pack_sha1);\n \tfree(idx_objects);\n \n \tfinal(pack_name, curr_pack,\n \t\tindex_name, curr_index,\n \t\tkeep_name, keep_msg,\n-\t\tsha1);\n+\t\tpack_sha1);\n \tfree(objects);\n \tfree(index_name_buf);\n \tfree(keep_name_buf);\n-- \n1.6.0.1.212.g35f9f\n"},{"id":"89158","messageId":"1220040482-10108-6-git-send-email-nico@cam.org","threadId":"15270","inReplyTo":"1220040482-10108-5-git-send-email-nico@cam.org","subject":"[PATCH 5/5] fixup_pack_header_footer(): use nicely aligned buffer sizes","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T20:08:02Z","receivedAt":"2008-08-29T20:08:02Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"It should be more efficient to use nicely aligned buffer sizes, either\nfor filesystem operations or SHA1 checksums.  Also, using a relatively\nsmall nominal size might allow for the data to remain in L1 cache\nbetween both SHA1_Update() calls.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n pack-write.c |   11 ++++++++---\n 1 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex f9776c5..939ed56 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -167,7 +167,7 @@ void fixup_pack_header_footer(int pack_fd,\n \t\t\t unsigned char *partial_pack_sha1,\n \t\t\t off_t partial_pack_offset)\n {\n-\tstatic const int buf_sz = 128 * 1024;\n+\tint aligned_sz, buf_sz = 8 * 1024;\n \tSHA_CTX old_sha1_ctx, new_sha1_ctx;\n \tstruct pack_header hdr;\n \tchar *buf;\n@@ -188,10 +188,11 @@ void fixup_pack_header_footer(int pack_fd,\n \tpartial_pack_offset -= sizeof(hdr);\n \n \tbuf = xmalloc(buf_sz);\n+\taligned_sz = buf_sz - sizeof(hdr);\n \tfor (;;) {\n \t\tssize_t m, n;\n-\t\tm = (partial_pack_sha1 && partial_pack_offset < buf_sz) ?\n-\t\t\tpartial_pack_offset : buf_sz;\n+\t\tm = (partial_pack_sha1 && partial_pack_offset < aligned_sz) ?\n+\t\t\tpartial_pack_offset : aligned_sz;\n \t\tn = xread(pack_fd, buf, m);\n \t\tif (!n)\n \t\t\tbreak;\n@@ -199,6 +200,10 @@ void fixup_pack_header_footer(int pack_fd,\n \t\t\tdie(\"Failed to checksum %s: %s\", pack_name, strerror(errno));\n \t\tSHA1_Update(&new_sha1_ctx, buf, n);\n \n+\t\taligned_sz -= n;\n+\t\tif (!aligned_sz)\n+\t\t\taligned_sz = buf_sz;\n+\n \t\tif (!partial_pack_sha1)\n \t\t\tcontinue;\n \n-- \n1.6.0.1.212.g35f9f\n"},{"id":"89159","messageId":"alpine.LFD.1.10.0808291609030.1624@xanadu.home","threadId":"15270","inReplyTo":"20080829143023.GA7403@spearce.org","subject":"Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer()","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-08-29T20:14:14Z","receivedAt":"2008-08-29T20:14:14Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 29 Aug 2008, Shawn O. Pearce wrote:\n\n> Oh, yea, that makes sense.  It still seems like playing with fire.\n> \n> I'd rather the caller pass in the proper offset than rely on it\n> being the current position of the fd.  Especially if the caller\n> does actually have it available.\n> \n> If you change anything, I'd like to see this lseek(SEEK_CUR) go away.\n\nDone.  I struggled a bit since it simply didn't work initially -- see \nthe first patch in the updated series.\n\n> > And another thing I had in store (but for which you _again_ beat me to :-) )\n> > is to realign data reads onto filesystem blocks.\n> \n> That _really_ made the JGit code ugly.  But I think its worth it.\n\nSee my version.  I think it is reasonably clear.\n\n> I also want to try and buffer the whole object appending we do\n> during fixThinPack(), as right now we write the object header in\n> one write and then compressed data bursts in the others.  Moving it\n> to at least write a full 4k at a time should remove about 2 write\n> calls per object.\n\nYep.  In the C git case, moving to sha1write() added that buffering \nfor free.\n\n\nNicolas\n"},{"id":"89290","messageId":"7v7i9xd5hu.fsf@gitster.siamese.dyndns.org","threadId":"15270","inReplyTo":"1220040482-10108-6-git-send-email-nico@cam.org","subject":"Re: [PATCH 5/5] fixup_pack_header_footer(): use nicely aligned buffer sizes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-31T07:10:37Z","receivedAt":"2008-08-31T07:10:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks -- it was a very good read.  I wish every series I receive is\nclearly done like this one.\n\nQueued for 'next'.\n"}]}