{"thread":{"id":"12514","subject":"[PATCH 1/3] unpack-objects: prevent writing of inconsistent objects","startedAt":"2008-03-05T10:09:06Z","lastAt":"2008-03-05T10:09:08Z","messageCount":3,"participants":["Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"71062","messageId":"1204711748-10044-1-git-send-email-gitster@pobox.com","threadId":"12514","inReplyTo":null,"subject":"[PATCH 1/3] unpack-objects: prevent writing of inconsistent objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-05T10:09:06Z","receivedAt":"2008-03-05T10:09:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n\nThis patch introduces a strict mode, which ensures that:\n- no malformed object will be written\n- no object with broken links will be written\n\nThe patch ensures this by delaying the write of all non blob object.\nThese object are written, after all objects they link to are written.\n\nAn error can only result in unreferenced objects.\n\nSigned-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is resurrection of the reverted patch, as-is.\n\n Documentation/git-unpack-objects.txt |    3 +\n builtin-unpack-objects.c             |  110 +++++++++++++++++++++++++++++++--\n 2 files changed, 106 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\nindex b79be3f..3697896 100644\n--- a/Documentation/git-unpack-objects.txt\n+++ b/Documentation/git-unpack-objects.txt\n@@ -40,6 +40,9 @@ OPTIONS\n \tand make the best effort to recover as many objects as\n \tpossible.\n \n+--strict::\n+\tDon't write objects with broken content or links.\n+\n \n Author\n ------\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 50e07fa..9d2a854 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -7,11 +7,13 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"tree.h\"\n+#include \"tree-walk.h\"\n #include \"progress.h\"\n #include \"decorate.h\"\n+#include \"fsck.h\"\n \n-static int dry_run, quiet, recover, has_errors;\n-static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] < pack-file\";\n+static int dry_run, quiet, recover, has_errors, strict;\n+static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] [--strict] < pack-file\";\n \n /* We always read in 4kB chunks. */\n static unsigned char buffer[4096];\n@@ -31,6 +33,16 @@ static struct obj_buffer *lookup_object_buffer(struct object *base)\n \treturn lookup_decoration(&obj_decorate, base);\n }\n \n+static void add_object_buffer(struct object *object, char *buffer, unsigned long size)\n+{\n+\tstruct obj_buffer *obj;\n+\tobj = xcalloc(1, sizeof(struct obj_buffer));\n+\tobj->buffer = buffer;\n+\tobj->size = size;\n+\tif (add_decoration(&obj_decorate, object, obj))\n+\t\tdie(\"object %s tried to add buffer twice!\", sha1_to_hex(object->sha1));\n+}\n+\n /*\n  * Make sure at least \"min\" bytes are available in the buffer, and\n  * return the pointer to the buffer.\n@@ -134,9 +146,58 @@ static void add_delta_to_list(unsigned nr, unsigned const char *base_sha1,\n struct obj_info {\n \toff_t offset;\n \tunsigned char sha1[20];\n+\tstruct object *obj;\n };\n \n+#define FLAG_OPEN (1u<<20)\n+#define FLAG_WRITTEN (1u<<21)\n+\n static struct obj_info *obj_list;\n+unsigned nr_objects;\n+\n+static void write_cached_object(struct object *obj)\n+{\n+\tunsigned char sha1[20];\n+\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n+\tif (write_sha1_file(obj_buf->buffer, obj_buf->size, typename(obj->type), sha1) < 0)\n+\t\tdie(\"failed to write object %s\", sha1_to_hex(obj->sha1));\n+\tobj->flags |= FLAG_WRITTEN;\n+}\n+\n+static int check_object(struct object *obj, int type, void *data)\n+{\n+\tif (!obj)\n+\t\treturn 0;\n+\n+\tif (obj->flags & FLAG_WRITTEN)\n+\t\treturn 1;\n+\n+\tif (type != OBJ_ANY && obj->type != type)\n+\t\tdie(\"object type mismatch\");\n+\n+\tif (!(obj->flags & FLAG_OPEN)) {\n+\t\tunsigned long size;\n+\t\tint type = sha1_object_info(obj->sha1, &size);\n+\t\tif (type != obj->type || type <= 0)\n+\t\t\tdie(\"object of unexpected type\");\n+\t\tobj->flags |= FLAG_WRITTEN;\n+\t\treturn 1;\n+\t}\n+\n+\tif (fsck_object(obj, 1, fsck_error_function))\n+\t\tdie(\"Error in object\");\n+\tif (!fsck_walk(obj, check_object, 0))\n+\t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n+\twrite_cached_object(obj);\n+\treturn 1;\n+}\n+\n+static void write_rest(void)\n+{\n+\tunsigned i;\n+\tfor (i = 0; i < nr_objects; i++)\n+\t\tcheck_object(obj_list[i].obj, OBJ_ANY, 0);\n+}\n \n static void added_object(unsigned nr, enum object_type type,\n \t\t\t void *data, unsigned long size);\n@@ -144,9 +205,36 @@ static void added_object(unsigned nr, enum object_type type,\n static void write_object(unsigned nr, enum object_type type,\n \t\t\t void *buf, unsigned long size)\n {\n-\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n-\t\tdie(\"failed to write object\");\n \tadded_object(nr, type, buf, size);\n+\tif (!strict) {\n+\t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n+\t\t\tdie(\"failed to write object\");\n+\t\tfree(buf);\n+\t\tobj_list[nr].obj = 0;\n+\t} else if (type == OBJ_BLOB) {\n+\t\tstruct blob *blob;\n+\t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n+\t\t\tdie(\"failed to write object\");\n+\t\tfree(buf);\n+\n+\t\tblob = lookup_blob(obj_list[nr].sha1);\n+\t\tif (blob)\n+\t\t\tblob->object.flags |= FLAG_WRITTEN;\n+\t\telse\n+\t\t\tdie(\"invalid blob object\");\n+\t\tobj_list[nr].obj = 0;\n+\t} else {\n+\t\tstruct object *obj;\n+\t\tint eaten;\n+\t\thash_sha1_file(buf, size, typename(type), obj_list[nr].sha1);\n+\t\tobj = parse_object_buffer(obj_list[nr].sha1, type, size, buf, &eaten);\n+\t\tif (!obj)\n+\t\t\tdie(\"invalid %s\", typename(type));\n+\t\t/* buf is stored via add_object_buffer and in obj, if its a tree or commit */\n+\t\tadd_object_buffer(obj, buf, size);\n+\t\tobj->flags |= FLAG_OPEN;\n+\t\tobj_list[nr].obj = obj;\n+\t}\n }\n \n static void resolve_delta(unsigned nr, enum object_type type,\n@@ -163,7 +251,6 @@ static void resolve_delta(unsigned nr, enum object_type type,\n \t\tdie(\"failed to apply delta\");\n \tfree(delta);\n \twrite_object(nr, type, result, result_size);\n-\tfree(result);\n }\n \n static void added_object(unsigned nr, enum object_type type,\n@@ -193,7 +280,8 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,\n \n \tif (!dry_run && buf)\n \t\twrite_object(nr, type, buf, size);\n-\tfree(buf);\n+\telse\n+\t\tfree(buf);\n }\n \n static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n@@ -336,7 +424,8 @@ static void unpack_all(void)\n \tint i;\n \tstruct progress *progress = NULL;\n \tstruct pack_header *hdr = fill(sizeof(struct pack_header));\n-\tunsigned nr_objects = ntohl(hdr->hdr_entries);\n+\n+\tnr_objects = ntohl(hdr->hdr_entries);\n \n \tif (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)\n \t\tdie(\"bad pack file\");\n@@ -347,6 +436,7 @@ static void unpack_all(void)\n \tif (!quiet)\n \t\tprogress = start_progress(\"Unpacking objects\", nr_objects);\n \tobj_list = xmalloc(nr_objects * sizeof(*obj_list));\n+\tmemset(obj_list, 0, nr_objects * sizeof(*obj_list));\n \tfor (i = 0; i < nr_objects; i++) {\n \t\tunpack_one(i);\n \t\tdisplay_progress(progress, i + 1);\n@@ -382,6 +472,10 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\trecover = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--strict\")) {\n+\t\t\t\tstrict = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!prefixcmp(arg, \"--pack_header=\")) {\n \t\t\t\tstruct pack_header *hdr;\n \t\t\t\tchar *c;\n@@ -407,6 +501,8 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \tunpack_all();\n \tSHA1_Update(&ctx, buffer, offset);\n \tSHA1_Final(sha1, &ctx);\n+\tif (strict)\n+\t\twrite_rest();\n \tif (hashcmp(fill(20), sha1))\n \t\tdie(\"final sha1 did not match\");\n \tuse(20);\n-- \n1.5.4.3.529.gb25fb\n\n"},{"id":"71064","messageId":"1204711748-10044-2-git-send-email-gitster@pobox.com","threadId":"12514","inReplyTo":"1204711748-10044-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] t5300: add test for \"unpack-objects --strict\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-05T10:09:07Z","receivedAt":"2008-03-05T10:09:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This adds test for unpacking deltified objects with --strict option.\n\n - unpacking full trees with --strict should pass;\n\n - unpacking only trees with --strict should be rejected due to\n   missing blobs;\n\n - unpacking only trees with --strict into an existing\n   repository with necessary blobs should succeed.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * Third time lucky.\n\n t/t5300-pack-object.sh |   49 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 49 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex cd3c149..b297393 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -274,4 +274,53 @@ test_expect_success \\\n      packname_4=$(git pack-objects test-4 <obj-list) &&\n      test 3 = $(ls test-4-*.pack | wc -l)'\n \n+test_expect_failure 'unpacking with --strict' '\n+\n+\tgit config --unset pack.packsizelimit &&\n+\tCOPYING=$(git hash-object -w ../../COPYING) &&\n+\tfor j in a b c d e f g\n+\tdo\n+\t\tfor i in 0 1 2 3 4 5 6 7 8 9\n+\t\tdo\n+\t\t\to=$(echo $j$i | git hash-object -w --stdin) &&\n+\t\t\techo \"100644 $o\t0 $j$i\"\n+\t\tdone\n+\tdone >LIST &&\n+\trm -f .git/index &&\n+\tgit update-index --index-info <LIST &&\n+\tLIST=$(git write-tree) &&\n+\trm -f .git/index &&\n+\thead -n 10 LIST | git update-index --index-info &&\n+\tLI=$(git write-tree) &&\n+\trm -f .git/index &&\n+\ttail -n 10 LIST | git update-index --index-info &&\n+\tST=$(git write-tree) &&\n+\tPACK5=$( git rev-list --objects \"$LIST\" \"$LI\" \"$ST\" | \\\n+\t\tgit pack-objects test-5 ) &&\n+\tPACK6=$( (\n+\t\t\techo \"$LIST\"\n+\t\t\techo \"$LI\"\n+\t\t\techo \"$ST\"\n+\t\t ) | git pack-objects test-6 ) &&\n+\ttest_create_repo test-5 &&\n+\t(\n+\t\tcd test-5 &&\n+\t\tgit unpack-objects --strict <../test-5-$PACK5.pack &&\n+\t\tgit ls-tree -r $LIST &&\n+\t\tgit ls-tree -r $LI &&\n+\t\tgit ls-tree -r $ST\n+\t) &&\n+\ttest_create_repo test-6 &&\n+\t(\n+\t\t# tree-only into empty repo -- many unreachables\n+\t\tcd test-6 &&\n+\t\ttest_must_fail git unpack-objects --strict <../test-6-$PACK6.pack\n+\t) &&\n+\t(\n+\t\t# already populated -- no unreachables\n+\t\tcd test-5 &&\n+\t\tgit unpack-objects --strict <../test-6-$PACK6.pack\n+\t)\n+'\n+\n test_done\n-- \n1.5.4.3.529.gb25fb\n\n"},{"id":"71063","messageId":"1204711748-10044-3-git-send-email-gitster@pobox.com","threadId":"12514","inReplyTo":"1204711748-10044-2-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] unpack-objects: fix --strict handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-05T10:09:08Z","receivedAt":"2008-03-05T10:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier attempt (which was reverted) called added_object() (by the way,\nthe function should be renamed to resolve_dependents() --- it is called\nwhen we have a complete object data, and is responsible to resolve pending\ndeltified objects that use this object as their delta base object) without\nupdating obj_list[nr].sha1 with the correct value.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * I still doubt that FLAG_OPEN is necessary, but at least this\n   seems to fix the issue for me.  I am sick and tired of having\n   spent two nights looking at this issue, and I ran out of time\n   to deal with other topics I looked at on the list.\n\n   The moral of the story is _NOT_ \"do not to trust Martin's\n   patches\", but \"I should ignore topics unless I have enough\n   time to read them line-by-line.  Queuing them, hoping that\n   problems are caught by somebody while they are in 'next',\n   would NOT work\".\n\n builtin-unpack-objects.c |   73 ++++++++++++++++++++++++++++++++++++----------\n t/t5300-pack-object.sh   |    2 +-\n 2 files changed, 58 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 9d2a854..fecf0be 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -21,6 +21,11 @@ static unsigned int offset, len;\n static off_t consumed_bytes;\n static SHA_CTX ctx;\n \n+/*\n+ * When running under --strict mode, objects whose reachability are\n+ * suspect are kept in core without getting written in the object\n+ * store.\n+ */\n struct obj_buffer {\n \tchar *buffer;\n \tunsigned long size;\n@@ -155,6 +160,10 @@ struct obj_info {\n static struct obj_info *obj_list;\n unsigned nr_objects;\n \n+/*\n+ * Called only from check_object() after it verified this object\n+ * is Ok.\n+ */\n static void write_cached_object(struct object *obj)\n {\n \tunsigned char sha1[20];\n@@ -164,6 +173,11 @@ static void write_cached_object(struct object *obj)\n \tobj->flags |= FLAG_WRITTEN;\n }\n \n+/*\n+ * At the very end of the processing, write_rest() scans the objects\n+ * that have reachability requirements and calls this function.\n+ * Verify its reachability and validity recursively and write it out.\n+ */\n static int check_object(struct object *obj, int type, void *data)\n {\n \tif (!obj)\n@@ -202,19 +216,25 @@ static void write_rest(void)\n static void added_object(unsigned nr, enum object_type type,\n \t\t\t void *data, unsigned long size);\n \n+/*\n+ * Write out nr-th object from the list, now we know the contents\n+ * of it.  Under --strict, this buffers structured objects in-core,\n+ * to be checked at the end.\n+ */\n static void write_object(unsigned nr, enum object_type type,\n \t\t\t void *buf, unsigned long size)\n {\n-\tadded_object(nr, type, buf, size);\n \tif (!strict) {\n \t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n \t\t\tdie(\"failed to write object\");\n+\t\tadded_object(nr, type, buf, size);\n \t\tfree(buf);\n-\t\tobj_list[nr].obj = 0;\n+\t\tobj_list[nr].obj = NULL;\n \t} else if (type == OBJ_BLOB) {\n \t\tstruct blob *blob;\n \t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n \t\t\tdie(\"failed to write object\");\n+\t\tadded_object(nr, type, buf, size);\n \t\tfree(buf);\n \n \t\tblob = lookup_blob(obj_list[nr].sha1);\n@@ -222,15 +242,15 @@ static void write_object(unsigned nr, enum object_type type,\n \t\t\tblob->object.flags |= FLAG_WRITTEN;\n \t\telse\n \t\t\tdie(\"invalid blob object\");\n-\t\tobj_list[nr].obj = 0;\n+\t\tobj_list[nr].obj = NULL;\n \t} else {\n \t\tstruct object *obj;\n \t\tint eaten;\n \t\thash_sha1_file(buf, size, typename(type), obj_list[nr].sha1);\n+\t\tadded_object(nr, type, buf, size);\n \t\tobj = parse_object_buffer(obj_list[nr].sha1, type, size, buf, &eaten);\n \t\tif (!obj)\n \t\t\tdie(\"invalid %s\", typename(type));\n-\t\t/* buf is stored via add_object_buffer and in obj, if its a tree or commit */\n \t\tadd_object_buffer(obj, buf, size);\n \t\tobj->flags |= FLAG_OPEN;\n \t\tobj_list[nr].obj = obj;\n@@ -253,6 +273,10 @@ static void resolve_delta(unsigned nr, enum object_type type,\n \twrite_object(nr, type, result, result_size);\n }\n \n+/*\n+ * We now know the contents of an object (which is nr-th in the pack);\n+ * resolve all the deltified objects that are based on it.\n+ */\n static void added_object(unsigned nr, enum object_type type,\n \t\t\t void *data, unsigned long size)\n {\n@@ -284,13 +308,28 @@ static void unpack_non_delta_entry(enum object_type type, unsigned long size,\n \t\tfree(buf);\n }\n \n+static int resolve_against_held(unsigned nr, const unsigned char *base,\n+\t\t\t\tvoid *delta_data, unsigned long delta_size)\n+{\n+\tstruct object *obj;\n+\tstruct obj_buffer *obj_buffer;\n+\tobj = lookup_object(base);\n+\tif (!obj)\n+\t\treturn 0;\n+\tobj_buffer = lookup_object_buffer(obj);\n+\tif (!obj_buffer)\n+\t\treturn 0;\n+\tresolve_delta(nr, obj->type, obj_buffer->buffer,\n+\t\t      obj_buffer->size, delta_data, delta_size);\n+\treturn 1;\n+}\n+\n static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n \t\t\t       unsigned nr)\n {\n \tvoid *delta_data, *base;\n \tunsigned long base_size;\n \tunsigned char base_sha1[20];\n-\tstruct object *obj;\n \n \tif (type == OBJ_REF_DELTA) {\n \t\thashcpy(base_sha1, fill(20));\n@@ -300,7 +339,13 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n \t\t\tfree(delta_data);\n \t\t\treturn;\n \t\t}\n-\t\tif (!has_sha1_file(base_sha1)) {\n+\t\tif (has_sha1_file(base_sha1))\n+\t\t\t; /* Ok we have this one */\n+\t\telse if (resolve_against_held(nr, base_sha1,\n+\t\t\t\t\t      delta_data, delta_size))\n+\t\t\treturn; /* we are done */\n+\t\telse {\n+\t\t\t/* cannot resolve yet --- queue it */\n \t\t\thashcpy(obj_list[nr].sha1, null_sha1);\n \t\t\tadd_delta_to_list(nr, base_sha1, 0, delta_data, delta_size);\n \t\t\treturn;\n@@ -346,22 +391,18 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n \t\t\t}\n \t\t}\n \t\tif (!base_found) {\n-\t\t\t/* The delta base object is itself a delta that\n-\t\t\t   has not been\tresolved yet. */\n+\t\t\t/*\n+\t\t\t * The delta base object is itself a delta that\n+\t\t\t * has not been resolved yet.\n+\t\t\t */\n \t\t\thashcpy(obj_list[nr].sha1, null_sha1);\n \t\t\tadd_delta_to_list(nr, null_sha1, base_offset, delta_data, delta_size);\n \t\t\treturn;\n \t\t}\n \t}\n \n-\tobj = lookup_object(base_sha1);\n-\tif (obj) {\n-\t\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n-\t\tif (obj_buf) {\n-\t\t\tresolve_delta(nr, obj->type, obj_buf->buffer, obj_buf->size, delta_data, delta_size);\n-\t\t\treturn;\n-\t\t}\n-\t}\n+\tif (resolve_against_held(nr, base_sha1, delta_data, delta_size))\n+\t\treturn;\n \n \tbase = read_sha1_file(base_sha1, &type, &base_size);\n \tif (!base) {\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex b297393..b7610d1 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -274,7 +274,7 @@ test_expect_success \\\n      packname_4=$(git pack-objects test-4 <obj-list) &&\n      test 3 = $(ls test-4-*.pack | wc -l)'\n \n-test_expect_failure 'unpacking with --strict' '\n+test_expect_success 'unpacking with --strict' '\n \n \tgit config --unset pack.packsizelimit &&\n \tCOPYING=$(git hash-object -w ../../COPYING) &&\n-- \n1.5.4.3.529.gb25fb\n\n"}]}