{"thread":{"id":"37436","subject":"[PATCH 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","startedAt":"2014-08-28T14:46:29Z","lastAt":"2014-09-13T09:08:18Z","messageCount":68,"participants":["Johannes Schindelin","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"248498","messageId":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":null,"subject":"[PATCH 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Johannes Schindelin","fromEmail":"schindelin@wisc.edu","sentAt":"2014-08-28T14:46:29Z","receivedAt":"2014-08-28T14:46:29Z","isPatch":true,"sender":{"key":"schindelin@wisc.edu","avatar":null},"body":"This patch series introduces detailed checking of tag objects when calling\ngit fsck, and also when transfer.fsckobjects is set to true.\n\nTo this end, the fsck machinery is reworked to accept the buffer and size\nof the object to check, and for commit and tag objects, we verify that the\nbuffers end in a NUL.\n\nThis work was sponsored by GitHub.\n\nJohannes Schindelin (5):\n Refactor type_from_string() to avoid die()ing in case of errors\n fsck: inspect tag objects more closely\n Add regression tests for stricter tag fsck'ing\n Refactor out fsck_object_buffer() accepting the object data\n Make sure that index-pack --strict fails upon invalid tag objects\n\n builtin/index-pack.c | 3 +-\n builtin/unpack-objects.c | 14 ++++--\n fsck.c | 109 ++++++++++++++++++++++++++++++++++++++++++++---\n fsck.h | 3 ++\n object.c | 13 +++++-\n object.h | 1 +\n t/t1450-fsck.sh | 39 +++++++++++++++++\n t/t5302-pack-index.sh | 19 +++++++++\n 8 files changed, 188 insertions(+), 13 deletions(-)\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"248484","messageId":"alpine.DEB.1.00.1408281646350.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","subject":"[PATCH 1/6] Refactor type_from_string() to avoid die()ing in case of errors","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-08-28T14:46:36Z","receivedAt":"2014-08-28T14:46:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In the next commits, we will enhance the fsck_tag() function to check\ntag objects more thoroughly. To this end, we need a function to verify\nthat a given string is a valid object type, but that does not die() in\nthe negative case.\n\nSigned-off-by: Johannes Schindelin \n---\n object.c | 13 ++++++++++++-\n object.h | 1 +\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/object.c b/object.c\nindex a16b9f9..5eee592 100644\n--- a/object.c\n+++ b/object.c\n@@ -33,13 +33,24 @@ const char *typename(unsigned int type)\n \treturn object_type_strings[type];\n }\n \n-int type_from_string(const char *str)\n+int type_from_string_gently(const char *str)\n {\n \tint i;\n \n \tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n \t\tif (!strcmp(str, object_type_strings[i]))\n \t\t\treturn i;\n+\n+\treturn -1;\n+}\n+\n+int type_from_string(const char *str)\n+{\n+\tint i = type_from_string_gently(str);\n+\n+\tif (i >= 0)\n+\t\treturn i;\n+\n \tdie(\"invalid object type \\\"%s\\\"\", str);\n }\n \ndiff --git a/object.h b/object.h\nindex 5e8d8ee..5c5d22f 100644\n--- a/object.h\n+++ b/object.h\n@@ -54,6 +54,7 @@ struct object {\n \n extern const char *typename(unsigned int type);\n extern int type_from_string(const char *str);\n+extern int type_from_string_gently(const char *str);\n \n /*\n * Return the current number of buckets in the object hashmap.\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"248485","messageId":"alpine.DEB.1.00.1408281646400.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","subject":"[PATCH 2/6] Accept object data in the fsck_object() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-08-28T14:46:42Z","receivedAt":"2014-08-28T14:46:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When fsck'ing an incoming pack, we need to fsck objects that cannot be\nread via read_sha1_file() because they are not local yet (and might even\nbe rejected if transfer.fsckobjects is set to 'true').\n\nFor commits, there is a hack in place: we basically cache commit\nobjects' buffers anyway, but the same is not true, say, for tag objects.\n\nBy refactoring fsck_object() to take the object buffer and size as\noptional arguments -- optional, because we still fall back to the\nprevious method to look at the cached commit objects if the caller\npasses NULL -- we prepare the machinery for the upcoming handling of tag\nobjects.\n\nThe assumption that such buffers are inherently NUL terminated is now\nwrong, of course, hence we pass the size of the buffer so that we can\nadd a sanity check later, to prevent running past the end of the buffer.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fsck.c | 2 +-\n builtin/index-pack.c | 3 ++-\n builtin/unpack-objects.c | 14 ++++++++++----\n fsck.c | 24 +++++++++++++++---------\n fsck.h | 4 +++-\n 5 files changed, 31 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex d42a27d..d9f4e6e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)\n \n \tif (fsck_walk(obj, mark_used, NULL))\n \t\tobjerror(obj, \"broken links\");\n-\tif (fsck_object(obj, check_strict, fsck_error_func))\n+\tif (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))\n \t\treturn -1;\n \n \tif (obj->type == OBJ_TREE) {\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 5568a5b..f2465ff 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n \t\t\tif (do_fsck_object &&\n-\t\t\t fsck_object(obj, 1, fsck_error_function))\n+\t\t\t fsck_object(obj, buf, size, 1,\n+\t\t\t\t fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 99cde45..855d94b 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -164,10 +164,10 @@ static unsigned nr_objects;\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+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)\n {\n \tunsigned char sha1[20];\n-\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n+\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@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)\n */\n static int check_object(struct object *obj, int type, void *data)\n {\n+\tstruct obj_buffer *obj_buf;\n+\n \tif (!obj)\n \t\treturn 1;\n \n@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, void *data)\n \t\treturn 0;\n \t}\n \n-\tif (fsck_object(obj, 1, fsck_error_function))\n+\tobj_buf = lookup_object_buffer(obj);\n+\tif (!obj_buf)\n+\t\tdie(\"Whoops! Cannot find object '%s'\", sha1_to_hex(obj->sha1));\n+\tif (fsck_object(obj, obj_buf->buffer, obj_buf->size, 1,\n+\t\t\tfsck_error_function))\n \t\tdie(\"Error in object\");\n \tif (fsck_walk(obj, check_object, NULL))\n \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n-\twrite_cached_object(obj);\n+\twrite_cached_object(obj, obj_buf);\n \treturn 0;\n }\n \ndiff --git a/fsck.c b/fsck.c\nindex 56156ff..dd77628 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n }\n \n static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n-\t\t\t fsck_error error_func)\n+\tunsigned long size, fsck_error error_func)\n {\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, fsck_error error_func)\n+static int fsck_commit(struct commit *commit, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n-\tconst char *buffer = get_commit_buffer(commit, NULL);\n-\tint ret = fsck_commit_buffer(commit, buffer, error_func);\n-\tunuse_commit_buffer(commit, buffer);\n+\tconst char *buffer = data ? data : get_commit_buffer(commit, &size);\n+\tint ret = fsck_commit_buffer(commit, buffer, size, error_func);\n+\tif (!data)\n+\t\tunuse_commit_buffer(commit, buffer);\n \treturn ret;\n }\n \n-static int fsck_tag(struct tag *tag, fsck_error error_func)\n+static int fsck_tag(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n \tstruct object *tagged = tag->tagged;\n \n@@ -339,7 +342,8 @@ static int fsck_tag(struct tag *tag, fsck_error error_func)\n \treturn 0;\n }\n \n-int fsck_object(struct object *obj, int strict, fsck_error error_func)\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func)\n {\n \tif (!obj)\n \t\treturn error_func(obj, FSCK_ERROR, \"no valid object to fsck\");\n@@ -349,9 +353,11 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)\n \tif (obj->type == OBJ_TREE)\n \t\treturn fsck_tree((struct tree *) obj, strict, error_func);\n \tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit((struct commit *) obj, error_func);\n+\t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag((struct tag *) obj, error_func);\n+\t\treturn fsck_tag((struct tag *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \n \treturn error_func(obj, FSCK_ERROR, \"unknown type '%d' (internal fsck error)\",\n \t\t\t obj->type);\ndiff --git a/fsck.h b/fsck.h\nindex 1e4f527..d1e6387 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -28,6 +28,8 @@ int fsck_error_function(struct object *obj, int type, const char *fmt, ...);\n * 0\t\teverything OK\n */\n int fsck_walk(struct object *obj, fsck_walk_func walk, void *data);\n-int fsck_object(struct object *obj, int strict, fsck_error error_func);\n+/* If NULL is passed for data, we assume the object is local and read it. */\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func);\n \n #endif\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"248483","messageId":"alpine.DEB.1.00.1408281646450.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","subject":"[PATCH 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-08-28T14:46:49Z","receivedAt":"2014-08-28T14:46:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"So far, we assumed that the buffer is NUL terminated, but this is not\na safe assumption, now that we opened the fsck_object() API to pass a\nbuffer directly.\n\nSo let's make sure that there is at least an empty line in the buffer.\nThat way, our checks would fail if the empty line was encountered\nprematurely, and consequently we can get away with the current string\ncomparisons even with non-NUL-terminated buffers are passed to\nfsck_object().\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex dd77628..db6aaa4 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \treturn retval;\n }\n \n+static int must_have_empty_line(const void *data, unsigned long size,\n+\tstruct object *obj, fsck_error error_func)\n+{\n+\tconst char *buffer = (const char *)data;\n+\tint i;\n+\n+\tfor (i = 0; i < size; i++) {\n+\t\tswitch (buffer[i]) {\n+\t\tcase '\\0':\n+\t\t\treturn error_func(obj, FSCK_ERROR,\n+\t\t\t\t\"invalid message: NUL at offset %d\", i);\n+\t\tcase '\\n':\n+\t\t\tif (i + 1 < size && buffer[i + 1] == '\\n')\n+\t\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\treturn error_func(obj, FSCK_ERROR, \"invalid buffer: missing empty line\");\n+}\n+\n static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n {\n \tchar *end;\n@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \tunsigned parent_count, parent_line_count = 0;\n \tint err;\n \n+\tif (must_have_empty_line(buffer, size, &commit->object, error_func))\n+\t\treturn -1;\n+\n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"248486","messageId":"alpine.DEB.1.00.1408281646530.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","subject":"[PATCH 4/6] fsck: check tag objects' headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-08-28T14:46:55Z","receivedAt":"2014-08-28T14:46:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We inspect commit objects pretty much in detail in git-fsck, but we just\nglanced over the tag objects. Let's be stricter.\n\nThis work was sponsored by GitHub Inc.\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 87 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex db6aaa4..d30946b 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -6,6 +6,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"fsck.h\"\n+#include \"refs.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -355,6 +356,90 @@ static int fsck_commit(struct commit *commit, const char *data,\n \treturn ret;\n }\n \n+static int fsck_tag_buffer(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n+{\n+\tunsigned char commit_sha1[20];\n+\tint ret = 0;\n+\tconst char *buffer;\n+\tchar *tmp = NULL, *eol;\n+\n+\tif (data)\n+\t\tbuffer = data;\n+\telse {\n+\t\tenum object_type type;\n+\n+\t\tbuffer = tmp = read_sha1_file(tag->object.sha1, &type, &size);\n+\t\tif (!buffer)\n+\t\t\treturn error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"cannot read tag object\");\n+\n+\t\tif (type != OBJ_TAG) {\n+\t\t\tret = error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"expected tag got %s\",\n+\t\t\t typename(type));\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+\tif (must_have_empty_line(buffer, size, &tag->object, error_func))\n+\t\tgoto done;\n+\n+\tif (!skip_prefix(buffer, \"object \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'object' line\");\n+\t\tgoto done;\n+\t}\n+\tif (get_sha1_hex(buffer, commit_sha1) || buffer[40] != '\\n') {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'object' line format - bad sha1\");\n+\t\tgoto done;\n+\t}\n+\tbuffer += 41;\n+\n+\tif (!skip_prefix(buffer, \"type \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'type' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\t*eol = '\\0';\n+\tif (type_from_string_gently(buffer) < 0)\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'type' value\");\n+\t*eol = '\\n';\n+\tif (ret)\n+\t\tgoto done;\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'tag' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\t*eol = '\\0';\n+\tif (check_refname_format(buffer, REFNAME_ALLOW_ONELEVEL))\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'tag' name: %s\", buffer);\n+\t*eol = '\\n';\n+\tif (ret)\n+\t\tgoto done;\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n+\t\t/* early tags do not contain 'tagger' lines; warn only */\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid format - expected 'tagger' line\");\n+\t}\n+\tret = fsck_ident(&buffer, &tag->object, error_func);\n+\n+done:\n+\tfree(tmp);\n+\treturn ret;\n+}\n+\n static int fsck_tag(struct tag *tag, const char *data,\n \tunsigned long size, fsck_error error_func)\n {\n@@ -362,7 +447,8 @@ static int fsck_tag(struct tag *tag, const char *data,\n \n \tif (!tagged)\n \t\treturn error_func(&tag->object, FSCK_ERROR, \"could not load tagged object\");\n-\treturn 0;\n+\n+\treturn fsck_tag_buffer(tag, data, size, error_func);\n }\n \n int fsck_object(struct object *obj, void *data, unsigned long size,\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"248487","messageId":"alpine.DEB.1.00.1408281646570.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","subject":"[PATCH 5/6] Add regression tests for stricter tag fsck'ing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-08-28T14:46:59Z","receivedAt":"2014-08-28T14:46:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The intent of the two new test cases is to catch general breakages in\nthe fsck_tag() function, not so much to test it extensively, trying to\nstrike the proper balance between thoroughness and speed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t1450-fsck.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8c739c9..16b3e4a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -194,6 +194,45 @@ test_expect_success 'tag pointing to something else than its type' '\n \ttest_must_fail git fsck --tags\n '\n \n+test_expect_success 'tag with incorrect tag name' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tcat >wrong-tag <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag wrong name format\n+\ttagger T A Gger 1234567890 -0000\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object -t tag -w --stdin .git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\tgit fsck --tags 2>out &&\n+\tcat out &&\n+\tgrep \"invalid .tag. name\" out\n+'\n+\n+test_expect_success 'tag with missing tagger' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tcat >wrong-tag <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag gutentag\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object -t tag -w --stdin .git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\tgit fsck --tags 2>out &&\n+\tcat out &&\n+\tgrep \"expected .tagger. line\" out\n+'\n+\n test_expect_success 'cleaned up' '\n \tgit fsck >actual 2>&1 &&\n \ttest_cmp empty actual\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"248488","messageId":"alpine.DEB.1.00.1408281647030.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","subject":"[PATCH 6/6] Make sure that index-pack --strict fails upon invalid tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-08-28T14:47:06Z","receivedAt":"2014-08-28T14:47:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"One of the most important use cases for the strict tag object checking\nis when transfer.fsckobjects is set to true to catch invalid objects\nearly on. This new regression test essentially tests the same code path\nby directly calling 'index-pack --strict' on a pack containing an\ninvalid tag object.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5302-pack-index.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 4bbb718..80bd3ee 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object store' '\n test -f .git/objects/pack/pack-${pack1}.idx\n '\n \n+test_expect_success 'index-pack --strict fails upon invalid tag' '\n+ sha=$(git rev-parse HEAD) &&\n+ cat >wrong-tag < err &&\n+ grep \"invalid .tag. name\" err\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"248520","messageId":"xmqqzjeo8hvm.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408281646350.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH 1/6] Refactor type_from_string() to avoid die()ing in case of errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T20:43:41Z","receivedAt":"2014-08-28T20:43:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> In the next commits, we will enhance the fsck_tag() function to check\n> tag objects more thoroughly. To this end, we need a function to verify\n> that a given string is a valid object type, but that does not die() in\n> the negative case.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> object.c | 13 ++++++++++++-\n> object.h | 1 +\n> 2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/object.c b/object.c\n> index a16b9f9..5eee592 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -33,13 +33,24 @@ const char *typename(unsigned int type)\n> \treturn object_type_strings[type];\n> }\n> \n> -int type_from_string(const char *str)\n> +int type_from_string_gently(const char *str)\n> {\n> \tint i;\n> \n> \tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n> \t\tif (!strcmp(str, object_type_strings[i]))\n> \t\t\treturn i;\n> +\n> +\treturn -1;\n> +}\n> +\n> +int type_from_string(const char *str)\n> +{\n> +\tint i = type_from_string_gently(str);\n> +\n> +\tif (i >= 0)\n> +\t\treturn i;\n> +\n> \tdie(\"invalid object type \\\"%s\\\"\", str);\n> }\n\nHmph. The above is not wrong per-se, but I would have done\n\n\tint type_from_string_gently(const char *str, int gentle);\n #define type_from_string(str) type_from_string_gently((str), 1)\n\nand added two lines to renamed type_from_string() function\n\n+\tif (gently)\n+ \treturn -1;\n"},{"id":"248521","messageId":"xmqqvbpc8hon.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408281646400.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/6] Accept object data in the fsck_object() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T20:47:52Z","receivedAt":"2014-08-28T20:47:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> When fsck'ing an incoming pack, we need to fsck objects that cannot be\n> read via read_sha1_file() because they are not local yet (and might even\n> be rejected if transfer.fsckobjects is set to 'true').\n>\n> For commits, there is a hack in place: we basically cache commit\n> objects' buffers anyway, but the same is not true, say, for tag objects.\n>\n> By refactoring fsck_object() to take the object buffer and size as\n> optional arguments -- optional, because we still fall back to the\n> previous method to look at the cached commit objects if the caller\n> passes NULL -- we prepare the machinery for the upcoming handling of tag\n> objects.\n>\n> The assumption that such buffers are inherently NUL terminated is now\n> wrong, of course, hence we pass the size of the buffer so that we can\n> add a sanity check later, to prevent running past the end of the buffer.\n\nA nice side effect may be that we can now check (and perhaps warn) a\ncommit buffer with a NUL inside, perhaps? I am not suggesting to\nadd such a check to this series, but mentioning the possibilty here\nmay have a merit.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/fsck.c | 2 +-\n> builtin/index-pack.c | 3 ++-\n> builtin/unpack-objects.c | 14 ++++++++++----\n> fsck.c | 24 +++++++++++++++---------\n> fsck.h | 4 +++-\n> 5 files changed, 31 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index d42a27d..d9f4e6e 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)\n> \n> \tif (fsck_walk(obj, mark_used, NULL))\n> \t\tobjerror(obj, \"broken links\");\n> -\tif (fsck_object(obj, check_strict, fsck_error_func))\n> +\tif (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))\n> \t\treturn -1;\n> \n> \tif (obj->type == OBJ_TREE) {\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index 5568a5b..f2465ff 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n> \t\t\tif (!obj)\n> \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n> \t\t\tif (do_fsck_object &&\n> -\t\t\t fsck_object(obj, 1, fsck_error_function))\n> +\t\t\t fsck_object(obj, buf, size, 1,\n> +\t\t\t\t fsck_error_function))\n> \t\t\t\tdie(_(\"Error in object\"));\n> \t\t\tif (fsck_walk(obj, mark_link, NULL))\n> \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\n> diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\n> index 99cde45..855d94b 100644\n> --- a/builtin/unpack-objects.c\n> +++ b/builtin/unpack-objects.c\n> @@ -164,10 +164,10 @@ static unsigned nr_objects;\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> +static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)\n> {\n> \tunsigned char sha1[20];\n> -\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n> +\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> @@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)\n> */\n> static int check_object(struct object *obj, int type, void *data)\n> {\n> +\tstruct obj_buffer *obj_buf;\n> +\n> \tif (!obj)\n> \t\treturn 1;\n> \n> @@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, void *data)\n> \t\treturn 0;\n> \t}\n> \n> -\tif (fsck_object(obj, 1, fsck_error_function))\n> +\tobj_buf = lookup_object_buffer(obj);\n> +\tif (!obj_buf)\n> +\t\tdie(\"Whoops! Cannot find object '%s'\", sha1_to_hex(obj->sha1));\n> +\tif (fsck_object(obj, obj_buf->buffer, obj_buf->size, 1,\n> +\t\t\tfsck_error_function))\n> \t\tdie(\"Error in object\");\n> \tif (fsck_walk(obj, check_object, NULL))\n> \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n> -\twrite_cached_object(obj);\n> +\twrite_cached_object(obj, obj_buf);\n> \treturn 0;\n> }\n> \n> diff --git a/fsck.c b/fsck.c\n> index 56156ff..dd77628 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n> }\n> \n> static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n> -\t\t\t fsck_error error_func)\n> +\tunsigned long size, fsck_error error_func)\n> {\n> \tunsigned char tree_sha1[20], sha1[20];\n> \tstruct commit_graft *graft;\n> @@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n> \treturn 0;\n> }\n> \n> -static int fsck_commit(struct commit *commit, fsck_error error_func)\n> +static int fsck_commit(struct commit *commit, const char *data,\n> +\tunsigned long size, fsck_error error_func)\n> {\n> -\tconst char *buffer = get_commit_buffer(commit, NULL);\n> -\tint ret = fsck_commit_buffer(commit, buffer, error_func);\n> -\tunuse_commit_buffer(commit, buffer);\n> +\tconst char *buffer = data ? data : get_commit_buffer(commit, &size);\n> +\tint ret = fsck_commit_buffer(commit, buffer, size, error_func);\n> +\tif (!data)\n> +\t\tunuse_commit_buffer(commit, buffer);\n> \treturn ret;\n> }\n> \n> -static int fsck_tag(struct tag *tag, fsck_error error_func)\n> +static int fsck_tag(struct tag *tag, const char *data,\n> +\tunsigned long size, fsck_error error_func)\n> {\n> \tstruct object *tagged = tag->tagged;\n> \n> @@ -339,7 +342,8 @@ static int fsck_tag(struct tag *tag, fsck_error error_func)\n> \treturn 0;\n> }\n> \n> -int fsck_object(struct object *obj, int strict, fsck_error error_func)\n> +int fsck_object(struct object *obj, void *data, unsigned long size,\n> +\tint strict, fsck_error error_func)\n> {\n> \tif (!obj)\n> \t\treturn error_func(obj, FSCK_ERROR, \"no valid object to fsck\");\n> @@ -349,9 +353,11 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)\n> \tif (obj->type == OBJ_TREE)\n> \t\treturn fsck_tree((struct tree *) obj, strict, error_func);\n> \tif (obj->type == OBJ_COMMIT)\n> -\t\treturn fsck_commit((struct commit *) obj, error_func);\n> +\t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n> +\t\t\tsize, error_func);\n> \tif (obj->type == OBJ_TAG)\n> -\t\treturn fsck_tag((struct tag *) obj, error_func);\n> +\t\treturn fsck_tag((struct tag *) obj, (const char *) data,\n> +\t\t\tsize, error_func);\n> \n> \treturn error_func(obj, FSCK_ERROR, \"unknown type '%d' (internal fsck error)\",\n> \t\t\t obj->type);\n> diff --git a/fsck.h b/fsck.h\n> index 1e4f527..d1e6387 100644\n> --- a/fsck.h\n> +++ b/fsck.h\n> @@ -28,6 +28,8 @@ int fsck_error_function(struct object *obj, int type, const char *fmt, ...);\n> * 0\t\teverything OK\n> */\n> int fsck_walk(struct object *obj, fsck_walk_func walk, void *data);\n> -int fsck_object(struct object *obj, int strict, fsck_error error_func);\n> +/* If NULL is passed for data, we assume the object is local and read it. */\n> +int fsck_object(struct object *obj, void *data, unsigned long size,\n> +\tint strict, fsck_error error_func);\n> \n> #endif\n"},{"id":"248522","messageId":"xmqqr4008h4p.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408281646450.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T20:59:50Z","receivedAt":"2014-08-28T20:59:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> So far, we assumed that the buffer is NUL terminated, but this is not\n> a safe assumption, now that we opened the fsck_object() API to pass a\n> buffer directly.\n>\n> So let's make sure that there is at least an empty line in the buffer.\n> That way, our checks would fail if the empty line was encountered\n> prematurely, and consequently we can get away with the current string\n> comparisons even with non-NUL-terminated buffers are passed to\n> fsck_object().\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n\nHeh, I probably should have read this one before commenting on 2/6.\n\nIt makes me feel somewhat uneasy to insist that there must be a\nblank line in the commit object, even though from the very first\nimplementation of \"commit-tree\" I think we have always had a blank\nthere at the end of the header, even when you feed nothing as the\nmessage to it.\n\nI think the new check is OK, but the message should be s/empty\nline/end of header/ or something. It is not like we require an\nempty line in the log message proper.\n\n> fsck.c | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n>\n> diff --git a/fsck.c b/fsck.c\n> index dd77628..db6aaa4 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n> \treturn retval;\n> }\n> \n> +static int must_have_empty_line(const void *data, unsigned long size,\n> +\tstruct object *obj, fsck_error error_func)\n> +{\n> +\tconst char *buffer = (const char *)data;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < size; i++) {\n> +\t\tswitch (buffer[i]) {\n> +\t\tcase '\\0':\n> +\t\t\treturn error_func(obj, FSCK_ERROR,\n> +\t\t\t\t\"invalid message: NUL at offset %d\", i);\n> +\t\tcase '\\n':\n> +\t\t\tif (i + 1 < size && buffer[i + 1] == '\\n')\n> +\t\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\n> +\treturn error_func(obj, FSCK_ERROR, \"invalid buffer: missing empty line\");\n> +}\n> +\n> static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n> {\n> \tchar *end;\n> @@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n> \tunsigned parent_count, parent_line_count = 0;\n> \tint err;\n> \n> +\tif (must_have_empty_line(buffer, size, &commit->object, error_func))\n> +\t\treturn -1;\n> +\n> \tif (!skip_prefix(buffer, \"tree \", &buffer))\n> \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n> \tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n"},{"id":"248523","messageId":"xmqqlhq88fyb.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408281646530.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T21:25:16Z","receivedAt":"2014-08-28T21:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> We inspect commit objects pretty much in detail in git-fsck, but we just\n> glanced over the tag objects. Let's be stricter.\n>\n> This work was sponsored by GitHub Inc.\n\nIs it only this commit, or all of these patches in the series?\nDoes GitHub want their name sprinkled over all changes they sponsor?\n\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> fsck.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 87 insertions(+), 1 deletion(-)\n>\n> diff --git a/fsck.c b/fsck.c\n> index db6aaa4..d30946b 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -6,6 +6,7 @@\n> #include \"commit.h\"\n> #include \"tag.h\"\n> #include \"fsck.h\"\n> +#include \"refs.h\"\n> \n> static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n> {\n> @@ -355,6 +356,90 @@ static int fsck_commit(struct commit *commit, const char *data,\n> \treturn ret;\n> }\n> \n> +static int fsck_tag_buffer(struct tag *tag, const char *data,\n> +\tunsigned long size, fsck_error error_func)\n> +{\n> +\tunsigned char commit_sha1[20];\n> +\tint ret = 0;\n> +\tconst char *buffer;\n> +\tchar *tmp = NULL, *eol;\n\nCall it \"to_free\" or something; naming it 'tmp' would tempt people\nwho later touch this code to reuse it temporarily to hold something\nunrelated (I know they will notice that mistake later, but noticing\nmistake after wasting time is too late).\n\n> +\tif (data)\n> +\t\tbuffer = data;\n> +\telse {\n> +\t\tenum object_type type;\n> +\n> +\t\tbuffer = tmp = read_sha1_file(tag->object.sha1, &type, &size);\n> +\t\tif (!buffer)\n> +\t\t\treturn error_func(&tag->object, FSCK_ERROR,\n> +\t\t\t\t\"cannot read tag object\");\n> +\n> +\t\tif (type != OBJ_TAG) {\n> +\t\t\tret = error_func(&tag->object, FSCK_ERROR,\n> +\t\t\t\t\"expected tag got %s\",\n> +\t\t\t typename(type));\n> +\t\t\tgoto done;\n> +\t\t}\n> +\t}\n> +\n> +\tif (must_have_empty_line(buffer, size, &tag->object, error_func))\n> +\t\tgoto done;\n> +\n> +\tif (!skip_prefix(buffer, \"object \", &buffer)) {\n> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'object' line\");\n> +\t\tgoto done;\n> +\t}\n> +\tif (get_sha1_hex(buffer, commit_sha1) || buffer[40] != '\\n') {\n\nThis code is not making the mistake to assume that tagged objects\nare always commits, so let's not call it commit_sha1; I'd suggest \njust calling it sha1[] (or even \"tmp\" or \"junk\", as the parsed value\nis not used).\n\n> +\t*eol = '\\0';\n> +\tif (type_from_string_gently(buffer) < 0)\n> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'type' value\");\n> +\t*eol = '\\n';\n> +\tif (ret)\n> +\t\tgoto done;\n\nI can see that it is reverted back to '\\n' immediately after calling\ntype_from_string_gently(), but it is a bit unfortunate that \"const\nchar *data\" needs to be touched this way.\n\nSince the callee is introduced in this series, perhaps it can be\nmade to take a counted string?\n\n> +\tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'tag' line\");\n> +\t\tgoto done;\n> +\t}\n> +\teol = strchr(buffer, '\\n');\n> +\tif (!eol) {\n> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n> +\t\tgoto done;\n> +\t}\n> +\t*eol = '\\0';\n> +\tif (check_refname_format(buffer, REFNAME_ALLOW_ONELEVEL))\n> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'tag' name: %s\", buffer);\n> +\t*eol = '\\n';\n\nI actually think this check is harmful. It often matches the name\nof the ref, but there is nothing inherently \"refname like\" in the\ntag name proper.\n\nAnd I think it is unnecessary. Don't we already warn if it does not\nmatch the name of the ref we use to point at it? It would mean that\nanything that does not conform to the check-refname-format will get\na warning one way or the other, either it is pointed at by a ref\nwhose name is kosher and does not match, or a ref itself has a name\nthat does not pass check-refname-format.\n\n(goes and looks)\n\nYikes. I assumed too much. We do not seem to do much checking on\nrefs in that\n\n (1) After \"git rev-parse HEAD >.git/refs/heads/ee..oo\"\n \"fsck\" does not complain about the malformed ee..oo branch;\n\n (2) After \"git tag -a foo -m foo && cp .git/refs/tags/foo .git/refs/tags/bar\"\n \"fsck\" does not complain that refs/tags/bar talks about \"foo\"\n\nBut these two are something we would want to fix in a larger context\nwithin \"git fack\" anyway, so my comments above still stand.\n\n> +\tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n> +\t\t/* early tags do not contain 'tagger' lines; warn only */\n> +\t\terror_func(&tag->object, FSCK_WARN, \"invalid format - expected 'tagger' line\");\n\nNice. Even nicer that this explains why people should not touch\n'ret' here.\n\n> +\t}\n> +\tret = fsck_ident(&buffer, &tag->object, error_func);\n> +\n> +done:\n> +\tfree(tmp);\n> +\treturn ret;\n> +}\n> +\n> static int fsck_tag(struct tag *tag, const char *data,\n> \tunsigned long size, fsck_error error_func)\n> {\n> @@ -362,7 +447,8 @@ static int fsck_tag(struct tag *tag, const char *data,\n> \n> \tif (!tagged)\n> \t\treturn error_func(&tag->object, FSCK_ERROR, \"could not load tagged object\");\n> -\treturn 0;\n> +\n> +\treturn fsck_tag_buffer(tag, data, size, error_func);\n> }\n> \n> int fsck_object(struct object *obj, void *data, unsigned long size,\n"},{"id":"248524","messageId":"xmqqegw08fft.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"xmqqlhq88fyb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T21:36:22Z","receivedAt":"2014-08-28T21:36:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\tif (check_refname_format(buffer, REFNAME_ALLOW_ONELEVEL))\n>> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'tag' name: %s\", buffer);\n>> +\t*eol = '\\n';\n>\n> I actually think this check is harmful.\n\nLet me take this one back; we do a moral equivalent when we create a\ntag, like this:\n\n\tstrbuf_addf(sb, \"refs/tags/%s\", name);\n return check_refname_format(sb->buf, 0);\n\nSo validating using check_refname_format() is indeed a very good\nthing to do.\n\nAs you have length and buffer here, I would suggest updating this\npart of your patch to print into a strbuf\n\n\tstrbuf_addf(&sb, \"refs/tags/%.*s\", (eol - buffer), buffer);\n \tif (check_refname_format(sb.buf))\n \tret = ...\n\nand keep the constness of the incoming data.\n\n\t\n"},{"id":"248598","messageId":"20140829230546.GC24834@peff.net","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408281646400.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH 2/6] Accept object data in the fsck_object() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-29T23:05:46Z","receivedAt":"2014-08-29T23:05:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 28, 2014 at 04:46:42PM +0200, Johannes Schindelin wrote:\n\n> When fsck'ing an incoming pack, we need to fsck objects that cannot be\n> read via read_sha1_file() because they are not local yet (and might even\n> be rejected if transfer.fsckobjects is set to 'true').\n> \n> For commits, there is a hack in place: we basically cache commit\n> objects' buffers anyway, but the same is not true, say, for tag objects.\n> \n> By refactoring fsck_object() to take the object buffer and size as\n> optional arguments -- optional, because we still fall back to the\n> previous method to look at the cached commit objects if the caller\n> passes NULL -- we prepare the machinery for the upcoming handling of tag\n> objects.\n> \n> The assumption that such buffers are inherently NUL terminated is now\n> wrong, of course, hence we pass the size of the buffer so that we can\n> add a sanity check later, to prevent running past the end of the buffer.\n\nAfter this, can we get rid of passing a \"struct object\" to the fsck code\nentirely? It should be able to operate only on a type/buffer pair, along\nwith possibly a sha1 for error reporting.\n\nThe buffer management in index-pack's sha1_object function is\nsignificantly complicated by attaching the temporary buffer to a \"struct\ncommit\", only to then have to detach it a few lines later. It is the\nsole reason we had to add \"detach_commit_buffer\" a while ago.\n\n-Peff\n"},{"id":"248599","messageId":"20140829231034.GD24834@peff.net","threadId":"37436","inReplyTo":"xmqqvbpc8hon.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/6] Accept object data in the fsck_object() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-29T23:10:34Z","receivedAt":"2014-08-29T23:10:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 28, 2014 at 01:47:52PM -0700, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > When fsck'ing an incoming pack, we need to fsck objects that cannot be\n> > read via read_sha1_file() because they are not local yet (and might even\n> > be rejected if transfer.fsckobjects is set to 'true').\n> >\n> > For commits, there is a hack in place: we basically cache commit\n> > objects' buffers anyway, but the same is not true, say, for tag objects.\n> >\n> > By refactoring fsck_object() to take the object buffer and size as\n> > optional arguments -- optional, because we still fall back to the\n> > previous method to look at the cached commit objects if the caller\n> > passes NULL -- we prepare the machinery for the upcoming handling of tag\n> > objects.\n> >\n> > The assumption that such buffers are inherently NUL terminated is now\n> > wrong, of course, hence we pass the size of the buffer so that we can\n> > add a sanity check later, to prevent running past the end of the buffer.\n> \n> A nice side effect may be that we can now check (and perhaps warn) a\n> commit buffer with a NUL inside, perhaps? I am not suggesting to\n> add such a check to this series, but mentioning the possibilty here\n> may have a merit.\n\nI think that is a good check to add at some point. I demonstrated quite\na while ago that you can get up to some mischief by \"hiding\" bytes after\na NUL in the commit message. If we ever see feasible collision attacks\nagainst sha1, this is going to be an obvious vector for hiding random\nbytes to achieve the collisions.\n\nThe downside is that git's data model in theory promises to store\narbitrary bytes in people's commit messages. IMHO we have gotten far\nenough from that in practice that I do not think anybody is seriously\ndoing it (you could not really use most of the history inspection tools\nin a reasonable way; you would have to write a parallel set of tools\nthat never shows log messages, and then extracts and does something\nseparate with the commit content).\n\n-Peff\n"},{"id":"248600","messageId":"20140829232715.GE24834@peff.net","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408281646450.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-29T23:27:15Z","receivedAt":"2014-08-29T23:27:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 28, 2014 at 04:46:49PM +0200, Johannes Schindelin wrote:\n\n> So far, we assumed that the buffer is NUL terminated, but this is not\n> a safe assumption, now that we opened the fsck_object() API to pass a\n> buffer directly.\n> \n> So let's make sure that there is at least an empty line in the buffer.\n> That way, our checks would fail if the empty line was encountered\n> prematurely, and consequently we can get away with the current string\n> comparisons even with non-NUL-terminated buffers are passed to\n> fsck_object().\n\nHmm. So having looked through all of the code, I _think_ this is enough\nbecause all of the parsing:\n\n - is left-to-right, and stops when it sees something unexpected, which\n would include a double newline\n\n - uses strcspn for the string-dependent calls, and each call has \"\\n\"\n in its list of stop-characters.\n\nBut it seems kind of flaky and easy for later code to get wrong.\n\nWould it be that hard to just allocate an extra NUL at the end of\nobjects we inflate in index-pack? All of the regular object code paths\nprovide the extra NUL-termination, and many parts of git depend on that.\nI do not think we call into any other reusable code paths besides fsck()\nfrom index-pack, but if we were to add the NUL, we would future-proof\nagainst these \"weird\" object buffers getting passed elsewhere.\n\n-Peff\n"},{"id":"248601","messageId":"20140829234327.GF24834@peff.net","threadId":"37436","inReplyTo":"xmqqlhq88fyb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-29T23:43:27Z","receivedAt":"2014-08-29T23:43:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 28, 2014 at 02:25:16PM -0700, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > We inspect commit objects pretty much in detail in git-fsck, but we just\n> > glanced over the tag objects. Let's be stricter.\n> >\n> > This work was sponsored by GitHub Inc.\n> \n> Is it only this commit, or all of these patches in the series?\n> Does GitHub want their name sprinkled over all changes they sponsor?\n\nGitHub does not really care either way. I think it is well-known that we\nsponsor some git development (i.e., pretty much everything I and Michael\nwork on), and we do not need that fact sprinkled in the commit history.\n\nBut we are also happy for that fact to be transparent if it changes\npeople's opinions on whether the patch is a good idea (i.e., to know\nthat Johannes has some motive beyond just \"I think this is the right\nthing to do\"; I hope he _also_ thinks it is the right thing to do or\nwould not post the series, of course).\n\nPersonally, I think the cover letter is a good place for such things.\n\n-Peff\n"},{"id":"248602","messageId":"20140829234641.GG24834@peff.net","threadId":"37436","inReplyTo":"xmqqegw08fft.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-29T23:46:41Z","receivedAt":"2014-08-29T23:46:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 28, 2014 at 02:36:22PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> >> +\tif (check_refname_format(buffer, REFNAME_ALLOW_ONELEVEL))\n> >> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'tag' name: %s\", buffer);\n> >> +\t*eol = '\\n';\n> >\n> > I actually think this check is harmful.\n> \n> Let me take this one back; we do a moral equivalent when we create a\n> tag, like this:\n> \n> \tstrbuf_addf(sb, \"refs/tags/%s\", name);\n> return check_refname_format(sb->buf, 0);\n\nHmm. But that is because \"git tag\" always makes one type of tag: one in\nwhich the \"tag\" field is the same as the refname in which we store it.\nSo the name must be a valid refname there to meet the ref storage\nrequirement, and therefore the tag name must, too.\n\nBut is that something we necessarily need or want to enforce? Is it OK\nfor me to have refs/tags/foo pointing to a tag object that is not\nrelated to \"foo\" (either semantically or syntactically)?\n\nI dunno. I cannot think of a reason you would want to do such a thing,\nbut this seems like outlawing it because git does not generate it, not\nbecause it is necessarily a problematic thing to be doing.\n\n-Peff\n"},{"id":"248696","messageId":"xmqqwq9o2s6l.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"20140829234641.GG24834@peff.net","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-31T22:46:42Z","receivedAt":"2014-08-31T22:46:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmm. But that is because \"git tag\" always makes one type of tag: one in\n> which the \"tag\" field is the same as the refname in which we store it.\n> So the name must be a valid refname there to meet the ref storage\n> requirement, and therefore the tag name must, too.\n>\n> But is that something we necessarily need or want to enforce? Is it OK\n> for me to have refs/tags/foo pointing to a tag object that is not\n> related to \"foo\" (either semantically or syntactically)?\n>\n> I dunno. I cannot think of a reason you would want to do such a thing,\n> but this seems like outlawing it because git does not generate it, not\n> because it is necessarily a problematic thing to be doing.\n\nThanks for straightening me out.\n\nIf \"git fsck\" were a tool to validate that the objects and refs are\nin line with how \"git-core\" plumbing and Porcelain toolset uses the\nunderlying Git data model, it makes sense to insist a tag has a name\nthat is suitable for a refname, and the tag is pointed by a ref in\n\"refs/tags/\" followed by its name. The rules such a \"git fsck\" should\nimplement would be stricter than what the underlying Git data model\ncould represent and existing Git tools could handle (i.e. a commit\nwith broken ident line may not be usable with \"shortlog -e\" and would\nbe flagged as corrupt).\n\nBut tightening rules in that direction may risk hindering future\nprogress in an unnecessary way. We may want to be a bit lenient\nwhen we see something _unusual_ but not necessarily _wrong_, and the\nline between them would be blurry in places, as Git is an evolving\nsoftware. It is good to warn about an unsual ones, but we probably\nwould not want to error on them.\n\nThis tightening may be too strict without a very good reason. For\nexample, a tentative signed tag (e.g. \"for-linus\") often used in a\npull request to have it recorded in the resulting merge by the\nintegrator does not inherently need to be named at all; the ref is\nonly necessary as a means to transfer the signature from the\ncontributor to the integrator, and once merged, there is no need for\nthe tag to have any name. When we try to improve the workflow to\nintegrate authenticated work done on the side branch, we may come up\nwith a way to do so _without_ having to actually have a tag name\n(i.e. the \"tag\" contributor creates for such a purpose may not be\ndone by \"git tag -s\" when asking the result to be pulled but do\nsomething different, and it may be perfectly fine for such a\ntentative tag to lack the \"tag \" name line), but still allows us to\nrecord the same merge-tag in the resulting merge commit.\n\nSo...\n"},{"id":"248726","messageId":"xmqqiol527co.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"20140829234327.GF24834@peff.net","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-02T18:41:11Z","receivedAt":"2014-09-02T18:41:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 28, 2014 at 02:25:16PM -0700, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> > We inspect commit objects pretty much in detail in git-fsck, but we just\n>> > glanced over the tag objects. Let's be stricter.\n>> >\n>> > This work was sponsored by GitHub Inc.\n>> \n>> Is it only this commit, or all of these patches in the series?\n>> Does GitHub want their name sprinkled over all changes they sponsor?\n>\n> GitHub does not really care either way. I think it is well-known that we\n> sponsor some git development (i.e., pretty much everything I and Michael\n> work on), and we do not need that fact sprinkled in the commit history.\n>\n> But we are also happy for that fact to be transparent if it changes\n> people's opinions on whether the patch is a good idea (i.e., to know\n> that Johannes has some motive beyond just \"I think this is the right\n> thing to do\"; I hope he _also_ thinks it is the right thing to do or\n> would not post the series, of course).\n>\n> Personally, I think the cover letter is a good place for such things.\n\nYeah, and I saw it explained in the cover. It looked somewhat out\nof place to see it duplicated only for this patch in the 6 patch\nseries and I was wondering if there was something special about this\nspecific patch, hence my question.\n"},{"id":"248810","messageId":"20140903213848.GA28769@peff.net","threadId":"37436","inReplyTo":"xmqqiol527co.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-03T21:38:49Z","receivedAt":"2014-09-03T21:38:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 02, 2014 at 11:41:11AM -0700, Junio C Hamano wrote:\n\n> > Personally, I think the cover letter is a good place for such things.\n> \n> Yeah, and I saw it explained in the cover. It looked somewhat out\n> of place to see it duplicated only for this patch in the 6 patch\n> series and I was wondering if there was something special about this\n> specific patch, hence my question.\n\nAh. For some reason the cover letter did not make it to me[1]. I agree\nthat what is there is fine, and it can be dropped from the individual\ncommit message.\n\n-Peff\n\n[1] It did make it to gmane, so I guess the problem was on my end.\n"},{"id":"248819","messageId":"20140903222937.GA30560@peff.net","threadId":"37436","inReplyTo":"xmqqwq9o2s6l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-03T22:29:37Z","receivedAt":"2014-09-03T22:29:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 31, 2014 at 03:46:42PM -0700, Junio C Hamano wrote:\n\n> If \"git fsck\" were a tool to validate that the objects and refs are\n> in line with how \"git-core\" plumbing and Porcelain toolset uses the\n> underlying Git data model, it makes sense to insist a tag has a name\n> that is suitable for a refname, and the tag is pointed by a ref in\n> \"refs/tags/\" followed by its name. The rules such a \"git fsck\" should\n> implement would be stricter than what the underlying Git data model\n> could represent and existing Git tools could handle (i.e. a commit\n> with broken ident line may not be usable with \"shortlog -e\" and would\n> be flagged as corrupt).\n\nThis is a bit of an aside, but why do we have the \"tag\" line in the tag\nobject in the first place?\n\nIt is part of the object contents, and therefore is part of the\nsignature (which the refname is not). That's somewhat redundant with the\ntag message itself. E.g., the git v2.0.4 tag says:\n\n object 32f56600bb6ac6fc57183e79d2c1515dfa56672f\n type commit\n tag v2.0.4\n tagger Junio C Hamano 1406755201 -0700\n\n Git 2.0.4\n -----BEGIN PGP SIGNATURE-----\n ...\n\nImagine an evil person pushed the signed v2.0.4 tag to refs/tags/v2.1.0\n(perhaps because there is a bug in v2.0.4, and they want you to run the\nwrong version so they can exploit it). You can check with \"git show\"\nthat the \"tag\" field is actually v2.0.4, but then you could similarly\ncheck that the message says \"Git 2.0.4\".\n\nThe main advantage of the \"tag\" field is that it is machine-readable,\nand that your verification process can check that \"git verify-tag\nv2.1.0\" actually returns a tag that says \"tag v2.1.0\". But I do not\nthink we do that verification at all. I wonder if that is something we\nshould add support for.\n\nYou gave examples later in your email of tags that would not necessarily\ncare about this tag field (and anyway, if \"for-linus\" is used over and\nover, it is subject to these sorts of replays), so I do not think it is\nsomething we would want unconditionally in verify-tag.\n\nI think this may need to be filed under \"possible policy flags for\nverifying\" that we discussed earlier (i.e., in the same boat as \"does\nthe committer ident match the commit signature\", as it is a\nporcelain-ish policy, not an integral part of the plumbing).\n\nSo this is mostly food for thought at this point.\n\n-Peff\n"},{"id":"248821","messageId":"xmqq7g1kwb39.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"20140903222937.GA30560@peff.net","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-03T23:14:34Z","receivedAt":"2014-09-03T23:14:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This is a bit of an aside, but why do we have the \"tag\" line in the tag\n> object in the first place?\n\nhttp://thread.gmane.org/gmane.linux.kernel/297998/focus=1410\n\n> It is part of the object contents, and therefore is part of the\n> signature (which the refname is not). That's somewhat redundant with the\n> tag message itself. E.g., the git v2.0.4 tag says:\n>\n> object 32f56600bb6ac6fc57183e79d2c1515dfa56672f\n> type commit\n> tag v2.0.4\n> tagger Junio C Hamano 1406755201 -0700\n>\n> Git 2.0.4\n> -----BEGIN PGP SIGNATURE-----\n> ...\n\nYes, usually we write a moral equivalent in a human readable form as\nthe tag message, but the mapping \"s/^v/Git /\" between the tag name\nand the message is purely by convention, and I suspect some old tags\nI have may even have used \"s/^v/Git v/\" or \"s/^v/git v/\" or a\nsimilar inconsistent mapping.\n\n> The main advantage of the \"tag\" field is that it is machine-readable,\n> and that your verification process can check that \"git verify-tag\n> v2.1.0\" actually returns a tag that says \"tag v2.1.0\". But I do not\n> think we do that verification at all. I wonder if that is something we\n> should add support for.\n\nYes. That essentially boils down to \"refs/tags/$tag\" must have \"tag $tag\"\nline (the reverse may not have to be true if the hierarchy is\noutside refs/tags/, though).\n"},{"id":"248825","messageId":"20140904020454.GB32310@peff.net","threadId":"37436","inReplyTo":"xmqq7g1kwb39.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 4/6] fsck: check tag objects' headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-04T02:04:54Z","receivedAt":"2014-09-04T02:04:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 03, 2014 at 04:14:34PM -0700, Junio C Hamano wrote:\n\n> > The main advantage of the \"tag\" field is that it is machine-readable,\n> > and that your verification process can check that \"git verify-tag\n> > v2.1.0\" actually returns a tag that says \"tag v2.1.0\". But I do not\n> > think we do that verification at all. I wonder if that is something we\n> > should add support for.\n> \n> Yes. That essentially boils down to \"refs/tags/$tag\" must have \"tag $tag\"\n> line (the reverse may not have to be true if the hierarchy is\n> outside refs/tags/, though).\n\nExactly. But to just bring it back to the original series, I do not\nthink this is a thing that fsck should care about. It is something that\nverify-tag should care about (but again, not unconditionally, and\nprobably not by default).\n\n-Peff\n"},{"id":"249123","messageId":"alpine.DEB.1.00.1409101552250.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1408171840040.990@s15462909.onlinehome-server.info","subject":"[PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:52:33Z","receivedAt":"2014-09-10T13:52:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series introduces detailed checking of tag objects when calling\ngit fsck, and also when transfer.fsckobjects is set to true.\n\nTo this end, the fsck machinery is reworked to accept the buffer and size\nof the object to check, and for commit and tag objects, we verify that the\nbuffers contain an end of header (i.e. an empty line) to guarantee that our\nchecks do not run beyond the buffer.\n\nThis work was sponsored by GitHub.\n\nChanges since v1:\n\n- Let type_from_string_gently() accept an optional string length\n\n- Renamed 'tmp' to 'to_free'\n\n- Renamed must_have_empty_line() to require_end_of_header()\n\n- Renamed 'commit_sha1' in fsck_tag_buffer to 'sha1'\n\n- Avoided temporarily inserting NUL characters into the buffer\n\n- Demoted problematic 'tag' value to a mere warning\n\n- Mentioned GitHub only in the cover letter, i.e. keeping it out of the git log\n\nStill unaddressed:\n\n- getting rid of struct object altogether in fsck (I felt this was quite a big\n task, getting much more familiar with the non-tag code paths, and I did not\n want to delay this patch series up any further)\n\n- ensuring that index-pack passes only NUL-terminated buffers to fsck (again,\n I am not familiar enough with the code, and IIRC the problematic unit test\n that revealed that these buffers are not always NUL-terminated exercised the\n unpack-objects code path, not index-pack, again nothing I wanted to let\n delay this patch series any further).\n\nJohannes Schindelin (6):\n Refactor type_from_string() to avoid die()ing in case of errors\n Accept object data in the fsck_object() function\n Make sure fsck_commit_buffer() does not run out of the buffer\n fsck: check tag objects' headers\n Add regression tests for stricter tag fsck'ing\n Make sure that index-pack --strict fails upon invalid tag objects\n\n builtin/fsck.c | 2 +-\n builtin/index-pack.c | 3 +-\n builtin/unpack-objects.c | 14 +++--\n fsck.c | 132 +++++++++++++++++++++++++++++++++++++++++++----\n fsck.h | 4 +-\n object.c | 11 +++-\n object.h | 3 +-\n t/t1450-fsck.sh | 39 ++++++++++++++\n t/t5302-pack-index.sh | 19 +++++++\n 9 files changed, 207 insertions(+), 20 deletions(-)\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249128","messageId":"alpine.DEB.1.00.1409101552400.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"cover.1410356761.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/6] Refactor type_from_string() to avoid die()ing in case of errors","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:52:44Z","receivedAt":"2014-09-10T13:52:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In the next commits, we will enhance the fsck_tag() function to check\ntag objects more thoroughly. To this end, we need a function to verify\nthat a given string is a valid object type, but that does not die() in\nthe negative case.\n\nWhile at it, prepare type_from_string() for counted strings, i.e. strings\nwith an explicitly specified length rather than a NUL termination.\n\nSigned-off-by: Johannes Schindelin \n---\n object.c | 11 +++++++++--\n object.h | 3 ++-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex a16b9f9..aedac24 100644\n--- a/object.c\n+++ b/object.c\n@@ -33,13 +33,20 @@ const char *typename(unsigned int type)\n \treturn object_type_strings[type];\n }\n \n-int type_from_string(const char *str)\n+int type_from_string_gently(const char *str, ssize_t len, int gentle)\n {\n \tint i;\n \n+\tif (len < 0)\n+\t\tlen = strlen(str);\n+\n \tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n-\t\tif (!strcmp(str, object_type_strings[i]))\n+\t\tif (!strncmp(str, object_type_strings[i], len))\n \t\t\treturn i;\n+\n+\tif (gentle)\n+\t\treturn -1;\n+\n \tdie(\"invalid object type \\\"%s\\\"\", str);\n }\n \ndiff --git a/object.h b/object.h\nindex 5e8d8ee..e028ced 100644\n--- a/object.h\n+++ b/object.h\n@@ -53,7 +53,8 @@ struct object {\n };\n \n extern const char *typename(unsigned int type);\n-extern int type_from_string(const char *str);\n+extern int type_from_string_gently(const char *str, ssize_t, int gentle);\n+#define type_from_string(str) type_from_string_gently(str, -1, 0)\n \n /*\n * Return the current number of buckets in the object hashmap.\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249126","messageId":"alpine.DEB.1.00.1409101552480.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"cover.1410356761.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/6] Accept object data in the fsck_object() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:52:51Z","receivedAt":"2014-09-10T13:52:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When fsck'ing an incoming pack, we need to fsck objects that cannot be\nread via read_sha1_file() because they are not local yet (and might even\nbe rejected if transfer.fsckobjects is set to 'true').\n\nFor commits, there is a hack in place: we basically cache commit\nobjects' buffers anyway, but the same is not true, say, for tag objects.\n\nBy refactoring fsck_object() to take the object buffer and size as\noptional arguments -- optional, because we still fall back to the\nprevious method to look at the cached commit objects if the caller\npasses NULL -- we prepare the machinery for the upcoming handling of tag\nobjects.\n\nThe assumption that such buffers are inherently NUL terminated is now\nwrong, of course, hence we pass the size of the buffer so that we can\nadd a sanity check later, to prevent running past the end of the buffer.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fsck.c | 2 +-\n builtin/index-pack.c | 3 ++-\n builtin/unpack-objects.c | 14 ++++++++++----\n fsck.c | 24 +++++++++++++++---------\n fsck.h | 4 +++-\n 5 files changed, 31 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex d42a27d..d9f4e6e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)\n \n \tif (fsck_walk(obj, mark_used, NULL))\n \t\tobjerror(obj, \"broken links\");\n-\tif (fsck_object(obj, check_strict, fsck_error_func))\n+\tif (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))\n \t\treturn -1;\n \n \tif (obj->type == OBJ_TREE) {\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 5568a5b..f2465ff 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n \t\t\tif (do_fsck_object &&\n-\t\t\t fsck_object(obj, 1, fsck_error_function))\n+\t\t\t fsck_object(obj, buf, size, 1,\n+\t\t\t\t fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 99cde45..855d94b 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -164,10 +164,10 @@ static unsigned nr_objects;\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+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)\n {\n \tunsigned char sha1[20];\n-\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n+\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@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)\n */\n static int check_object(struct object *obj, int type, void *data)\n {\n+\tstruct obj_buffer *obj_buf;\n+\n \tif (!obj)\n \t\treturn 1;\n \n@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, void *data)\n \t\treturn 0;\n \t}\n \n-\tif (fsck_object(obj, 1, fsck_error_function))\n+\tobj_buf = lookup_object_buffer(obj);\n+\tif (!obj_buf)\n+\t\tdie(\"Whoops! Cannot find object '%s'\", sha1_to_hex(obj->sha1));\n+\tif (fsck_object(obj, obj_buf->buffer, obj_buf->size, 1,\n+\t\t\tfsck_error_function))\n \t\tdie(\"Error in object\");\n \tif (fsck_walk(obj, check_object, NULL))\n \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n-\twrite_cached_object(obj);\n+\twrite_cached_object(obj, obj_buf);\n \treturn 0;\n }\n \ndiff --git a/fsck.c b/fsck.c\nindex 56156ff..dd77628 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n }\n \n static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n-\t\t\t fsck_error error_func)\n+\tunsigned long size, fsck_error error_func)\n {\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, fsck_error error_func)\n+static int fsck_commit(struct commit *commit, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n-\tconst char *buffer = get_commit_buffer(commit, NULL);\n-\tint ret = fsck_commit_buffer(commit, buffer, error_func);\n-\tunuse_commit_buffer(commit, buffer);\n+\tconst char *buffer = data ? data : get_commit_buffer(commit, &size);\n+\tint ret = fsck_commit_buffer(commit, buffer, size, error_func);\n+\tif (!data)\n+\t\tunuse_commit_buffer(commit, buffer);\n \treturn ret;\n }\n \n-static int fsck_tag(struct tag *tag, fsck_error error_func)\n+static int fsck_tag(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n \tstruct object *tagged = tag->tagged;\n \n@@ -339,7 +342,8 @@ static int fsck_tag(struct tag *tag, fsck_error error_func)\n \treturn 0;\n }\n \n-int fsck_object(struct object *obj, int strict, fsck_error error_func)\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func)\n {\n \tif (!obj)\n \t\treturn error_func(obj, FSCK_ERROR, \"no valid object to fsck\");\n@@ -349,9 +353,11 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)\n \tif (obj->type == OBJ_TREE)\n \t\treturn fsck_tree((struct tree *) obj, strict, error_func);\n \tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit((struct commit *) obj, error_func);\n+\t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag((struct tag *) obj, error_func);\n+\t\treturn fsck_tag((struct tag *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \n \treturn error_func(obj, FSCK_ERROR, \"unknown type '%d' (internal fsck error)\",\n \t\t\t obj->type);\ndiff --git a/fsck.h b/fsck.h\nindex 1e4f527..d1e6387 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -28,6 +28,8 @@ int fsck_error_function(struct object *obj, int type, const char *fmt, ...);\n * 0\t\teverything OK\n */\n int fsck_walk(struct object *obj, fsck_walk_func walk, void *data);\n-int fsck_object(struct object *obj, int strict, fsck_error error_func);\n+/* If NULL is passed for data, we assume the object is local and read it. */\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func);\n \n #endif\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249124","messageId":"alpine.DEB.1.00.1409101552560.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"cover.1410356761.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:52:58Z","receivedAt":"2014-09-10T13:52:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"So far, we assumed that the buffer is NUL terminated, but this is not\na safe assumption, now that we opened the fsck_object() API to pass a\nbuffer directly.\n\nSo let's make sure that there is at least an empty line in the buffer.\nThat way, our checks would fail if the empty line was encountered\nprematurely, and consequently we can get away with the current string\ncomparisons even with non-NUL-terminated buffers are passed to\nfsck_object().\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex dd77628..9dd7d12 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \treturn retval;\n }\n \n+static int require_end_of_header(const void *data, unsigned long size,\n+\tstruct object *obj, fsck_error error_func)\n+{\n+\tconst char *buffer = (const char *)data;\n+\tint i;\n+\n+\tfor (i = 0; i < size; i++) {\n+\t\tswitch (buffer[i]) {\n+\t\tcase '\\0':\n+\t\t\treturn error_func(obj, FSCK_ERROR,\n+\t\t\t\t\"invalid message: NUL at offset %d\", i);\n+\t\tcase '\\n':\n+\t\t\tif (i + 1 < size && buffer[i + 1] == '\\n')\n+\t\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\treturn error_func(obj, FSCK_ERROR, \"invalid buffer: missing empty line\");\n+}\n+\n static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n {\n \tchar *end;\n@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \tunsigned parent_count, parent_line_count = 0;\n \tint err;\n \n+\tif (require_end_of_header(buffer, size, &commit->object, error_func))\n+\t\treturn -1;\n+\n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249129","messageId":"alpine.DEB.1.00.1409101553020.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"cover.1410356761.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 4/6] fsck: check tag objects' headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:53:04Z","receivedAt":"2014-09-10T13:53:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We inspect commit objects pretty much in detail in git-fsck, but we just\nglanced over the tag objects. Let's be stricter.\n\nSince we do not want to limit 'tag' lines unduly, values that would fail\nthe refname check only result in warnings, not errors.\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 84 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 9dd7d12..8341a30 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -6,6 +6,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"fsck.h\"\n+#include \"refs.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -355,6 +356,87 @@ static int fsck_commit(struct commit *commit, const char *data,\n \treturn ret;\n }\n \n+static int fsck_tag_buffer(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n+{\n+\tunsigned char sha1[20];\n+\tint ret = 0;\n+\tconst char *buffer;\n+\tchar *to_free = NULL, *eol;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (data)\n+\t\tbuffer = data;\n+\telse {\n+\t\tenum object_type type;\n+\n+\t\tbuffer = to_free =\n+\t\t\tread_sha1_file(tag->object.sha1, &type, &size);\n+\t\tif (!buffer)\n+\t\t\treturn error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"cannot read tag object\");\n+\n+\t\tif (type != OBJ_TAG) {\n+\t\t\tret = error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"expected tag got %s\",\n+\t\t\t typename(type));\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+\tif (require_end_of_header(buffer, size, &tag->object, error_func))\n+\t\tgoto done;\n+\n+\tif (!skip_prefix(buffer, \"object \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'object' line\");\n+\t\tgoto done;\n+\t}\n+\tif (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n') {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'object' line format - bad sha1\");\n+\t\tgoto done;\n+\t}\n+\tbuffer += 41;\n+\n+\tif (!skip_prefix(buffer, \"type \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'type' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\tif (type_from_string_gently(buffer, eol - buffer, 1) < 0)\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'type' value\");\n+\tif (ret)\n+\t\tgoto done;\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'tag' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n+\tif (check_refname_format(sb.buf, 0))\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid 'tag' name: %s\", buffer);\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n+\t\t/* early tags do not contain 'tagger' lines; warn only */\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid format - expected 'tagger' line\");\n+\t}\n+\tret = fsck_ident(&buffer, &tag->object, error_func);\n+\n+done:\n+\tfree(to_free);\n+\treturn ret;\n+}\n+\n static int fsck_tag(struct tag *tag, const char *data,\n \tunsigned long size, fsck_error error_func)\n {\n@@ -362,7 +444,8 @@ static int fsck_tag(struct tag *tag, const char *data,\n \n \tif (!tagged)\n \t\treturn error_func(&tag->object, FSCK_ERROR, \"could not load tagged object\");\n-\treturn 0;\n+\n+\treturn fsck_tag_buffer(tag, data, size, error_func);\n }\n \n int fsck_object(struct object *obj, void *data, unsigned long size,\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249127","messageId":"alpine.DEB.1.00.1409101553080.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"cover.1410356761.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 5/6] Add regression tests for stricter tag fsck'ing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:53:10Z","receivedAt":"2014-09-10T13:53:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The intent of the two new test cases is to catch general breakages in\nthe fsck_tag() function, not so much to test it extensively, trying to\nstrike the proper balance between thoroughness and speed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t1450-fsck.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 1 file changed, 39 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8c739c9..16b3e4a 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -194,6 +194,45 @@ test_expect_success 'tag pointing to something else than its type' '\n \ttest_must_fail git fsck --tags\n '\n \n+test_expect_success 'tag with incorrect tag name' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tcat >wrong-tag <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag wrong name format\n+\ttagger T A Gger 1234567890 -0000\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object -t tag -w --stdin .git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\tgit fsck --tags 2>out &&\n+\tcat out &&\n+\tgrep \"invalid .tag. name\" out\n+'\n+\n+test_expect_success 'tag with missing tagger' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tcat >wrong-tag <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag gutentag\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object -t tag -w --stdin .git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\tgit fsck --tags 2>out &&\n+\tcat out &&\n+\tgrep \"expected .tagger. line\" out\n+'\n+\n test_expect_success 'cleaned up' '\n \tgit fsck >actual 2>&1 &&\n \ttest_cmp empty actual\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249125","messageId":"alpine.DEB.1.00.1409101553140.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"cover.1410356761.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 6/6] Make sure that index-pack --strict fails upon invalid tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:53:15Z","receivedAt":"2014-09-10T13:53:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"One of the most important use cases for the strict tag object checking\nis when transfer.fsckobjects is set to true to catch invalid objects\nearly on. This new regression test essentially tests the same code path\nby directly calling 'index-pack --strict' on a pack containing an\ninvalid tag object.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5302-pack-index.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 4bbb718..80bd3ee 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object store' '\n test -f .git/objects/pack/pack-${pack1}.idx\n '\n \n+test_expect_success 'index-pack --strict fails upon invalid tag' '\n+ sha=$(git rev-parse HEAD) &&\n+ cat >wrong-tag < err &&\n+ grep \"invalid .tag. name\" err\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249130","messageId":"alpine.DEB.1.00.1409101556070.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101552250.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-10T13:58:28Z","receivedAt":"2014-09-10T13:58:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 10 Sep 2014, Johannes Schindelin wrote:\n\n> Still unaddressed:\n> \n> - getting rid of struct object altogether in fsck (I felt this was quite a big\n> task, getting much more familiar with the non-tag code paths, and I did not\n> want to delay this patch series up any further)\n> \n> - ensuring that index-pack passes only NUL-terminated buffers to fsck (again,\n> I am not familiar enough with the code, and IIRC the problematic unit test\n> that revealed that these buffers are not always NUL-terminated exercised the\n> unpack-objects code path, not index-pack, again nothing I wanted to let\n> delay this patch series any further).\n\nTo clarify: I am planning to address these issues later this year, but\nconsider them not critical enough to finish the fsck-tag patch series.\n\nPlease let me know if you disagree.\n\nCiao,\nDscho\n"},{"id":"249141","messageId":"xmqqlhprgylt.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101552560.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-10T17:43:42Z","receivedAt":"2014-09-10T17:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> So far, we assumed that the buffer is NUL terminated, but this is not\n> a safe assumption, now that we opened the fsck_object() API to pass a\n> buffer directly.\n>\n> So let's make sure that there is at least an empty line in the buffer.\n> That way, our checks would fail if the empty line was encountered\n> prematurely, and consequently we can get away with the current string\n> comparisons even with non-NUL-terminated buffers are passed to\n> fsck_object().\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> fsck.c | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n>\n> diff --git a/fsck.c b/fsck.c\n> index dd77628..9dd7d12 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n> \treturn retval;\n> }\n> \n> +static int require_end_of_header(const void *data, unsigned long size,\n> +\tstruct object *obj, fsck_error error_func)\n> +{\n> +\tconst char *buffer = (const char *)data;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < size; i++) {\n> +\t\tswitch (buffer[i]) {\n> +\t\tcase '\\0':\n> +\t\t\treturn error_func(obj, FSCK_ERROR,\n> +\t\t\t\t\"invalid message: NUL at offset %d\", i);\n\nIsn't this \"invalid header\"? After all we haven't escaped this loop\nand haven't seen the message part of the commit object (and it is\nthe same if you are going to later reuse this for tag objects).\n\n> +\t\tcase '\\n':\n> +\t\t\tif (i + 1 < size && buffer[i + 1] == '\\n')\n> +\t\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\n> +\treturn error_func(obj, FSCK_ERROR, \"invalid buffer: missing empty line\");\n\nI think you meant \"missing end of header\" here.\n\nOther than these small nits, the patch looks good from a cursory\nread.\n\n> +}\n> +\n> static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n> {\n> \tchar *end;\n> @@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n> \tunsigned parent_count, parent_line_count = 0;\n> \tint err;\n> \n> +\tif (require_end_of_header(buffer, size, &commit->object, error_func))\n> +\t\treturn -1;\n> +\n> \tif (!skip_prefix(buffer, \"tree \", &buffer))\n> \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n> \tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n"},{"id":"249142","messageId":"xmqqfvfzgy6x.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101553020.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 4/6] fsck: check tag objects' headers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-10T17:52:38Z","receivedAt":"2014-09-10T17:52:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +\tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'tag' line\");\n> +\t\tgoto done;\n> +\t}\n> +\teol = strchr(buffer, '\\n');\n> +\tif (!eol) {\n> +\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n> +\t\tgoto done;\n> +\t}\n> +\tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n> +\tif (check_refname_format(sb.buf, 0))\n> +\t\terror_func(&tag->object, FSCK_WARN, \"invalid 'tag' name: %s\", buffer);\n> +\tbuffer = eol + 1;\n> +\n> +\tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n> +\t\t/* early tags do not contain 'tagger' lines; warn only */\n> +\t\terror_func(&tag->object, FSCK_WARN, \"invalid format - expected 'tagger' line\");\n> +\t}\n> +\tret = fsck_ident(&buffer, &tag->object, error_func);\n\nShouldn't this relate to the above conditional, to skip ident check\non early tags that lack tagger lines?\n\n> +\n> +done:\n> +\tfree(to_free);\n\nstrbuf_release(&sb) here?\n\n> +\treturn ret;\n> +}\n> +\n"},{"id":"249143","messageId":"xmqqbnqngy0z.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101553080.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 5/6] Add regression tests for stricter tag fsck'ing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-10T17:56:12Z","receivedAt":"2014-09-10T17:56:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n> +\tgit fsck --tags 2>out &&\n\nI wonder what the command does with or without --tags option\n(applies to both tests added by this patch)?\n\nDoes running \"fsck\" without the option not to report broken tags\ndetected by the new checks added in this series? Should it?\n"},{"id":"249148","messageId":"CAPig+cRtq6+nx+MN89g9hOFosvjPYMJbVJ7FBSzniqhu+_rMKA@mail.gmail.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101552560.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-09-10T20:45:27Z","receivedAt":"2014-09-10T20:45:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 10, 2014 at 9:52 AM, Johannes Schindelin\n wrote:\n> So far, we assumed that the buffer is NUL terminated, but this is not\n> a safe assumption, now that we opened the fsck_object() API to pass a\n> buffer directly.\n>\n> So let's make sure that there is at least an empty line in the buffer.\n> That way, our checks would fail if the empty line was encountered\n> prematurely, and consequently we can get away with the current string\n> comparisons even with non-NUL-terminated buffers are passed to\n> fsck_object().\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> fsck.c | 23 +++++++++++++++++++++++\n> 1 file changed, 23 insertions(+)\n>\n> diff --git a/fsck.c b/fsck.c\n> index dd77628..9dd7d12 100644\n> --- a/fsck.c\n> +++ b/fsck.c\n> @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n> return retval;\n> }\n>\n> +static int require_end_of_header(const void *data, unsigned long size,\n> + struct object *obj, fsck_error error_func)\n> +{\n> + const char *buffer = (const char *)data;\n> + int i;\n> +\n> + for (i = 0; i < size; i++) {\n\nShould 'i' have type 'unsigned long', rather than 'int', to be\nconsistent with the type of 'size'?\n\n> + switch (buffer[i]) {\n> + case '\\0':\n> + return error_func(obj, FSCK_ERROR,\n> + \"invalid message: NUL at offset %d\", i);\n> + case '\\n':\n> + if (i + 1 < size && buffer[i + 1] == '\\n')\n> + return 0;\n> + }\n> + }\n> +\n> + return error_func(obj, FSCK_ERROR, \"invalid buffer: missing empty line\");\n> +}\n> +\n> static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n> {\n> char *end;\n> @@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n> unsigned parent_count, parent_line_count = 0;\n> int err;\n>\n> + if (require_end_of_header(buffer, size, &commit->object, error_func))\n> + return -1;\n> +\n> if (!skip_prefix(buffer, \"tree \", &buffer))\n> return error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n> if (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n> --\n> 2.0.0.rc3.9669.g840d1f9\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"249150","messageId":"xmqq7g1bgp5r.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101552250.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-10T21:07:44Z","receivedAt":"2014-09-10T21:07:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> This patch series introduces detailed checking of tag objects when calling\n> git fsck, and also when transfer.fsckobjects is set to true.\n>\n> To this end, the fsck machinery is reworked to accept the buffer and size\n> of the object to check, and for commit and tag objects, we verify that the\n> buffers contain an end of header (i.e. an empty line) to guarantee that our\n> checks do not run beyond the buffer.\n\nOverall they looked sensible and I am trying to queue them on 'pu'\nfor today's pushout.\n\nThanks.\n"},{"id":"249153","messageId":"xmqqsijzf9ij.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"xmqq7g1bgp5r.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-10T21:31:00Z","receivedAt":"2014-09-10T21:31:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Schindelin writes:\n>\n>> This patch series introduces detailed checking of tag objects when calling\n>> git fsck, and also when transfer.fsckobjects is set to true.\n>>\n>> To this end, the fsck machinery is reworked to accept the buffer and size\n>> of the object to check, and for commit and tag objects, we verify that the\n>> buffers contain an end of header (i.e. an empty line) to guarantee that our\n>> checks do not run beyond the buffer.\n>\n> Overall they looked sensible and I am trying to queue them on 'pu'\n> for today's pushout.\n>\n> Thanks.\n\nI was expecting to see interesting interactions with the \"oops, fsck\nwas not exiting with non-zero status in some cases\" fix by Peff with\nthe patch 5/6 of this series that adds two new tests to t1450, but\nbecause the breakage in these two cases are both only warning-events\nand not error-events, I didn't prefix \"git fsck\" with test_must_fail\nafter all.\n\nAre there some more serious kind of breakages that the new code\nactually catches as errors which the new tests are not exercising?\n\nThe second test however did need the fix I mentioned in the review\nto skip ident validation when we know the buffer does not point at a\npotential ident to pass, though (see below, which is what I queued\non the tip of your series as a \"SQUASH???\" single fix-up patch).\n\nPlease do an equivalent in the individual patches that introduce\nthese buglets in a reroll (i.e. not like I did here, which was done\nthis way only because I needed to make sure that day's integration\nresult passes the test suite with minimum effort on my end ;-)).\n\nThanks.\n\n fsck.c | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 8341a30..6e6ea25 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -248,14 +248,14 @@ static int require_end_of_header(const void *data, unsigned long size,\n \t\tswitch (buffer[i]) {\n \t\tcase '\\0':\n \t\t\treturn error_func(obj, FSCK_ERROR,\n-\t\t\t\t\"invalid message: NUL at offset %d\", i);\n+\t\t\t\t\"invalid header: NUL at offset %d\", i);\n \t\tcase '\\n':\n \t\t\tif (i + 1 < size && buffer[i + 1] == '\\n')\n \t\t\t\treturn 0;\n \t\t}\n \t}\n \n-\treturn error_func(obj, FSCK_ERROR, \"invalid buffer: missing empty line\");\n+\treturn error_func(obj, FSCK_ERROR, \"invalid buffer: missing end of header\");\n }\n \n static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n@@ -426,14 +426,16 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,\n \t\terror_func(&tag->object, FSCK_WARN, \"invalid 'tag' name: %s\", buffer);\n \tbuffer = eol + 1;\n \n-\tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n+\tif (!skip_prefix(buffer, \"tagger \", &buffer))\n \t\t/* early tags do not contain 'tagger' lines; warn only */\n-\t\terror_func(&tag->object, FSCK_WARN, \"invalid format - expected 'tagger' line\");\n-\t}\n-\tret = fsck_ident(&buffer, &tag->object, error_func);\n+\t\terror_func(&tag->object, FSCK_WARN,\n+\t\t\t \"invalid format - expected 'tagger' line\");\n+\telse\n+\t\tret = fsck_ident(&buffer, &tag->object, error_func);\n \n done:\n \tfree(to_free);\n+\tstrbuf_release(&sb);\n \treturn ret;\n }\n \n-- \n2.1.0-451-g8daadf2\n"},{"id":"249154","messageId":"xmqqoaunf8fv.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101553140.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 6/6] Make sure that index-pack --strict fails upon invalid tag objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-10T21:54:12Z","receivedAt":"2014-09-10T21:54:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> +test_expect_success 'index-pack --strict fails upon invalid tag' '\n> + sha=$(git rev-parse HEAD) &&\n> + cat >wrong-tag < +object $sha\n> +type commit\n> +tag guten tag\n> +\n> +This is an invalid tag.\n> +EOF\n\nMissing tagger is merely a warning event (thanks for a good comment\nin the code, by the way, to explain the reason why it is so is\nbecause of annotated tags in earlier versions of Git). tag names\nthat are not ref-formed is only a warning event in this reroll.\n\nSo this is not really \"invalid\" in the sense that index-pack can\nnotice it as an error, no?\n\n> +\n> + tag=$(git hash-object -t tag -w --stdin + pack1=$(echo $tag | git pack-objects tag-test) &&\n> + echo remove tag object &&\n> + thirtyeight=${tag#??} &&\n> + rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight &&\n> + test_must_fail git index-pack --strict tag-test-${pack1}.pack 2> err &&\n\nI had to drop \"must fail\" from this one (will amend the \"SQUASH???\"\nagain).\n\nPerhaps you would want to add a real error, e.g. a tag with tagger\nwhose ident does not pass fsck-ident or something?\n\n> + grep \"invalid .tag. name\" err\n> +'\n> +\n> test_done\n"},{"id":"249219","messageId":"alpine.DEB.1.00.1409111352130.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"xmqqlhprgylt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T11:59:00Z","receivedAt":"2014-09-11T11:59:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 10 Sep 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > diff --git a/fsck.c b/fsck.c\n> > index dd77628..9dd7d12 100644\n> > --- a/fsck.c\n> > +++ b/fsck.c\n> > @@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n> > \treturn retval;\n> > }\n> > \n> > +static int require_end_of_header(const void *data, unsigned long size,\n> > +\tstruct object *obj, fsck_error error_func)\n> > +{\n> > +\tconst char *buffer = (const char *)data;\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < size; i++) {\n> > +\t\tswitch (buffer[i]) {\n> > +\t\tcase '\\0':\n> > +\t\t\treturn error_func(obj, FSCK_ERROR,\n> > +\t\t\t\t\"invalid message: NUL at offset %d\", i);\n> \n> Isn't this \"invalid header\"? After all we haven't escaped this loop\n> and haven't seen the message part of the commit object (and it is\n> the same if you are going to later reuse this for tag objects).\n\nMy reasoning for keeping it saying \"message\" was that a message consists\nof a header and a body. I will change it to \"unterminated header\" instead,\nalso in the error message when no NUL was found.\n\nCiao,\nDscho\n"},{"id":"249220","messageId":"alpine.DEB.1.00.1409111614420.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"xmqqbnqngy0z.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 5/6] Add regression tests for stricter tag fsck'ing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:15:46Z","receivedAt":"2014-09-11T14:15:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 10 Sep 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > +\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n> > +\tgit fsck --tags 2>out &&\n> \n> I wonder what the command does with or without --tags option\n> (applies to both tests added by this patch)?\n> \n> Does running \"fsck\" without the option not to report broken tags\n> detected by the new checks added in this series? Should it?\n\nfsck fails perfectly fine without the --tags option; this option just\ntriggers that fsck will show also the objects referenced by the tag\nobjects (which is nice for debugging).\n\nCiao,\nDscho\n"},{"id":"249221","messageId":"alpine.DEB.1.00.1409111616380.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"xmqqsijzf9ij.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:20:15Z","receivedAt":"2014-09-11T14:20:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 10 Sep 2014, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Johannes Schindelin writes:\n> >\n> >> This patch series introduces detailed checking of tag objects when calling\n> >> git fsck, and also when transfer.fsckobjects is set to true.\n> >>\n> >> To this end, the fsck machinery is reworked to accept the buffer and size\n> >> of the object to check, and for commit and tag objects, we verify that the\n> >> buffers contain an end of header (i.e. an empty line) to guarantee that our\n> >> checks do not run beyond the buffer.\n> >\n> > Overall they looked sensible and I am trying to queue them on 'pu'\n> > for today's pushout.\n> >\n> > Thanks.\n> \n> I was expecting to see interesting interactions with the \"oops, fsck\n> was not exiting with non-zero status in some cases\" fix by Peff with\n> the patch 5/6 of this series that adds two new tests to t1450, but\n> because the breakage in these two cases are both only warning-events\n> and not error-events, I didn't prefix \"git fsck\" with test_must_fail\n> after all.\n\nThere were a couple of issues, thanks for pointing out that I missed\nsomething. Short story: hash-object, fsck *and* pack-objects parse tag\nobjects as they are encountered. Therefore, it would be a bit hard to test\nbecause we would have to emulate broken hash-object and pack-objects to\ngenerate a pack containing an invalid tag object.\n\nAs a compromise, I now test for the warnings to make sure that the code\npath is triggered, but do not explicitly test with a pack that would make\nindex-pack --strict *fail*.\n\nOkay?\n\nCiao,\nDscho\n\nP.S.: I squashed your changes in slightly modified form.\n"},{"id":"249222","messageId":"alpine.DEB.1.00.1409111620420.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"xmqqoaunf8fv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 6/6] Make sure that index-pack --strict fails upon invalid tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:22:10Z","receivedAt":"2014-09-11T14:22:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 10 Sep 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > + tag=$(git hash-object -t tag -w --stdin > + pack1=$(echo $tag | git pack-objects tag-test) &&\n> > + echo remove tag object &&\n> > + thirtyeight=${tag#??} &&\n> > + rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight &&\n> > + test_must_fail git index-pack --strict tag-test-${pack1}.pack 2> err &&\n> \n> I had to drop \"must fail\" from this one (will amend the \"SQUASH???\"\n> again).\n\nFunny. It failed here, but for the wrong reason: index-pack --strict\nfailed here because the object referenced by the tag object was not in the\npack.\n\nCiao,\nDscho\n"},{"id":"249223","messageId":"cover.1410445430.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409101552250.990@s15462909.onlinehome-server.info","subject":"[PATCH v3 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:26:11Z","receivedAt":"2014-09-11T14:26:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series introduces detailed checking of tag objects when calling\ngit fsck, and also when transfer.fsckobjects is set to true.\n\nTo this end, the fsck machinery is reworked to accept the buffer and size\nof the object to check, and for commit and tag objects, we verify that the\nbuffers contain an end of header (i.e. an empty line) to guarantee that our\nchecks do not run beyond the buffer.\n\nThis work was sponsored by GitHub.\n\nChanges since v2:\n\n- replaced 'invalid message' with 'unterminated header'\n\n- avoided comparison between int and unsigned long (thanks, Eric Sunshine)\n\n- made ident parsing conditional on finding the optional 'tagger' line\n\n- added forgotten strbuf_release()\n\nStill unaddressed:\n\n- getting rid of struct object altogether in fsck (I felt this was quite a big\n task, getting much more familiar with the non-tag code paths, and I did not\n want to delay this patch series up any further)\n\n- ensuring that index-pack passes only NUL-terminated buffers to fsck (again,\n I am not familiar enough with the code, and IIRC the problematic unit test\n that revealed that these buffers are not always NUL-terminated exercised the\n unpack-objects code path, not index-pack, again nothing I wanted to let\n delay this patch series any further).\n\nJohannes Schindelin (6):\n Refactor type_from_string() to avoid die()ing in case of errors\n Accept object data in the fsck_object() function\n Make sure fsck_commit_buffer() does not run out of the buffer\n fsck: check tag objects' headers\n Add regression tests for stricter tag fsck'ing\n Make sure that index-pack --strict checks tag objects\n\n builtin/fsck.c | 2 +-\n builtin/index-pack.c | 3 +-\n builtin/unpack-objects.c | 14 +++--\n fsck.c | 133 +++++++++++++++++++++++++++++++++++++++++++----\n fsck.h | 4 +-\n object.c | 11 +++-\n object.h | 3 +-\n t/t1450-fsck.sh | 20 +++++++\n t/t5302-pack-index.sh | 19 +++++++\n 9 files changed, 189 insertions(+), 20 deletions(-)\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249225","messageId":"6d088d41c538161f1ff1d548fdb2472acea6508e.1410445431.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410445430.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/6] Refactor type_from_string() to avoid die()ing in case of errors","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:26:19Z","receivedAt":"2014-09-11T14:26:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In the next commits, we will enhance the fsck_tag() function to check\ntag objects more thoroughly. To this end, we need a function to verify\nthat a given string is a valid object type, but that does not die() in\nthe negative case.\n\nWhile at it, prepare type_from_string() for counted strings, i.e. strings\nwith an explicitly specified length rather than a NUL termination.\n\nSigned-off-by: Johannes Schindelin \n---\n object.c | 11 +++++++++--\n object.h | 3 ++-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex a16b9f9..aedac24 100644\n--- a/object.c\n+++ b/object.c\n@@ -33,13 +33,20 @@ const char *typename(unsigned int type)\n \treturn object_type_strings[type];\n }\n \n-int type_from_string(const char *str)\n+int type_from_string_gently(const char *str, ssize_t len, int gentle)\n {\n \tint i;\n \n+\tif (len < 0)\n+\t\tlen = strlen(str);\n+\n \tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n-\t\tif (!strcmp(str, object_type_strings[i]))\n+\t\tif (!strncmp(str, object_type_strings[i], len))\n \t\t\treturn i;\n+\n+\tif (gentle)\n+\t\treturn -1;\n+\n \tdie(\"invalid object type \\\"%s\\\"\", str);\n }\n \ndiff --git a/object.h b/object.h\nindex 5e8d8ee..e028ced 100644\n--- a/object.h\n+++ b/object.h\n@@ -53,7 +53,8 @@ struct object {\n };\n \n extern const char *typename(unsigned int type);\n-extern int type_from_string(const char *str);\n+extern int type_from_string_gently(const char *str, ssize_t, int gentle);\n+#define type_from_string(str) type_from_string_gently(str, -1, 0)\n \n /*\n * Return the current number of buckets in the object hashmap.\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249224","messageId":"2f4f1b74e17c8b8b6fa551cddc9e5e07652e032c.1410445431.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410445430.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/6] Accept object data in the fsck_object() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:26:27Z","receivedAt":"2014-09-11T14:26:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When fsck'ing an incoming pack, we need to fsck objects that cannot be\nread via read_sha1_file() because they are not local yet (and might even\nbe rejected if transfer.fsckobjects is set to 'true').\n\nFor commits, there is a hack in place: we basically cache commit\nobjects' buffers anyway, but the same is not true, say, for tag objects.\n\nBy refactoring fsck_object() to take the object buffer and size as\noptional arguments -- optional, because we still fall back to the\nprevious method to look at the cached commit objects if the caller\npasses NULL -- we prepare the machinery for the upcoming handling of tag\nobjects.\n\nThe assumption that such buffers are inherently NUL terminated is now\nwrong, of course, hence we pass the size of the buffer so that we can\nadd a sanity check later, to prevent running past the end of the buffer.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fsck.c | 2 +-\n builtin/index-pack.c | 3 ++-\n builtin/unpack-objects.c | 14 ++++++++++----\n fsck.c | 24 +++++++++++++++---------\n fsck.h | 4 +++-\n 5 files changed, 31 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex d42a27d..d9f4e6e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)\n \n \tif (fsck_walk(obj, mark_used, NULL))\n \t\tobjerror(obj, \"broken links\");\n-\tif (fsck_object(obj, check_strict, fsck_error_func))\n+\tif (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))\n \t\treturn -1;\n \n \tif (obj->type == OBJ_TREE) {\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 5568a5b..f2465ff 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n \t\t\tif (do_fsck_object &&\n-\t\t\t fsck_object(obj, 1, fsck_error_function))\n+\t\t\t fsck_object(obj, buf, size, 1,\n+\t\t\t\t fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 99cde45..855d94b 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -164,10 +164,10 @@ static unsigned nr_objects;\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+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)\n {\n \tunsigned char sha1[20];\n-\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n+\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@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)\n */\n static int check_object(struct object *obj, int type, void *data)\n {\n+\tstruct obj_buffer *obj_buf;\n+\n \tif (!obj)\n \t\treturn 1;\n \n@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, void *data)\n \t\treturn 0;\n \t}\n \n-\tif (fsck_object(obj, 1, fsck_error_function))\n+\tobj_buf = lookup_object_buffer(obj);\n+\tif (!obj_buf)\n+\t\tdie(\"Whoops! Cannot find object '%s'\", sha1_to_hex(obj->sha1));\n+\tif (fsck_object(obj, obj_buf->buffer, obj_buf->size, 1,\n+\t\t\tfsck_error_function))\n \t\tdie(\"Error in object\");\n \tif (fsck_walk(obj, check_object, NULL))\n \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n-\twrite_cached_object(obj);\n+\twrite_cached_object(obj, obj_buf);\n \treturn 0;\n }\n \ndiff --git a/fsck.c b/fsck.c\nindex 56156ff..dd77628 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n }\n \n static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n-\t\t\t fsck_error error_func)\n+\tunsigned long size, fsck_error error_func)\n {\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, fsck_error error_func)\n+static int fsck_commit(struct commit *commit, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n-\tconst char *buffer = get_commit_buffer(commit, NULL);\n-\tint ret = fsck_commit_buffer(commit, buffer, error_func);\n-\tunuse_commit_buffer(commit, buffer);\n+\tconst char *buffer = data ? data : get_commit_buffer(commit, &size);\n+\tint ret = fsck_commit_buffer(commit, buffer, size, error_func);\n+\tif (!data)\n+\t\tunuse_commit_buffer(commit, buffer);\n \treturn ret;\n }\n \n-static int fsck_tag(struct tag *tag, fsck_error error_func)\n+static int fsck_tag(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n \tstruct object *tagged = tag->tagged;\n \n@@ -339,7 +342,8 @@ static int fsck_tag(struct tag *tag, fsck_error error_func)\n \treturn 0;\n }\n \n-int fsck_object(struct object *obj, int strict, fsck_error error_func)\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func)\n {\n \tif (!obj)\n \t\treturn error_func(obj, FSCK_ERROR, \"no valid object to fsck\");\n@@ -349,9 +353,11 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)\n \tif (obj->type == OBJ_TREE)\n \t\treturn fsck_tree((struct tree *) obj, strict, error_func);\n \tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit((struct commit *) obj, error_func);\n+\t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag((struct tag *) obj, error_func);\n+\t\treturn fsck_tag((struct tag *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \n \treturn error_func(obj, FSCK_ERROR, \"unknown type '%d' (internal fsck error)\",\n \t\t\t obj->type);\ndiff --git a/fsck.h b/fsck.h\nindex 1e4f527..d1e6387 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -28,6 +28,8 @@ int fsck_error_function(struct object *obj, int type, const char *fmt, ...);\n * 0\t\teverything OK\n */\n int fsck_walk(struct object *obj, fsck_walk_func walk, void *data);\n-int fsck_object(struct object *obj, int strict, fsck_error error_func);\n+/* If NULL is passed for data, we assume the object is local and read it. */\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func);\n \n #endif\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249226","messageId":"2f5a78c03de27bcf31151380710a504b72338c57.1410445431.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410445430.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:26:33Z","receivedAt":"2014-09-11T14:26:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"So far, we assumed that the buffer is NUL terminated, but this is not\na safe assumption, now that we opened the fsck_object() API to pass a\nbuffer directly.\n\nSo let's make sure that there is at least an empty line in the buffer.\nThat way, our checks would fail if the empty line was encountered\nprematurely, and consequently we can get away with the current string\ncomparisons even with non-NUL-terminated buffers are passed to\nfsck_object().\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex dd77628..73da6f8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \treturn retval;\n }\n \n+static int require_end_of_header(const void *data, unsigned long size,\n+\tstruct object *obj, fsck_error error_func)\n+{\n+\tconst char *buffer = (const char *)data;\n+\tunsigned long i;\n+\n+\tfor (i = 0; i < size; i++) {\n+\t\tswitch (buffer[i]) {\n+\t\tcase '\\0':\n+\t\t\treturn error_func(obj, FSCK_ERROR,\n+\t\t\t\t\"unterminated header: NUL at offset %d\", i);\n+\t\tcase '\\n':\n+\t\t\tif (i + 1 < size && buffer[i + 1] == '\\n')\n+\t\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\treturn error_func(obj, FSCK_ERROR, \"unterminated header\");\n+}\n+\n static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n {\n \tchar *end;\n@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \tunsigned parent_count, parent_line_count = 0;\n \tint err;\n \n+\tif (require_end_of_header(buffer, size, &commit->object, error_func))\n+\t\treturn -1;\n+\n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249229","messageId":"d55da50e2ebdf70a36db58114f81bf2e2acae709.1410445431.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410445430.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 4/6] fsck: check tag objects' headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:26:38Z","receivedAt":"2014-09-11T14:26:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We inspect commit objects pretty much in detail in git-fsck, but we just\nglanced over the tag objects. Let's be stricter.\n\nSince we do not want to limit 'tag' lines unduly, values that would fail\nthe refname check only result in warnings, not errors.\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 85 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 73da6f8..2fffa43 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -6,6 +6,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"fsck.h\"\n+#include \"refs.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -355,6 +356,88 @@ static int fsck_commit(struct commit *commit, const char *data,\n \treturn ret;\n }\n \n+static int fsck_tag_buffer(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n+{\n+\tunsigned char sha1[20];\n+\tint ret = 0;\n+\tconst char *buffer;\n+\tchar *to_free = NULL, *eol;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (data)\n+\t\tbuffer = data;\n+\telse {\n+\t\tenum object_type type;\n+\n+\t\tbuffer = to_free =\n+\t\t\tread_sha1_file(tag->object.sha1, &type, &size);\n+\t\tif (!buffer)\n+\t\t\treturn error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"cannot read tag object\");\n+\n+\t\tif (type != OBJ_TAG) {\n+\t\t\tret = error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"expected tag got %s\",\n+\t\t\t typename(type));\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+\tif (require_end_of_header(buffer, size, &tag->object, error_func))\n+\t\tgoto done;\n+\n+\tif (!skip_prefix(buffer, \"object \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'object' line\");\n+\t\tgoto done;\n+\t}\n+\tif (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n') {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'object' line format - bad sha1\");\n+\t\tgoto done;\n+\t}\n+\tbuffer += 41;\n+\n+\tif (!skip_prefix(buffer, \"type \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'type' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\tif (type_from_string_gently(buffer, eol - buffer, 1) < 0)\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'type' value\");\n+\tif (ret)\n+\t\tgoto done;\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'tag' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n+\tif (check_refname_format(sb.buf, 0))\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid 'tag' name: %s\", buffer);\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tagger \", &buffer))\n+\t\t/* early tags do not contain 'tagger' lines; warn only */\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid format - expected 'tagger' line\");\n+\telse\n+\t\tret = fsck_ident(&buffer, &tag->object, error_func);\n+\n+done:\n+\tstrbuf_release(&sb);\n+\tfree(to_free);\n+\treturn ret;\n+}\n+\n static int fsck_tag(struct tag *tag, const char *data,\n \tunsigned long size, fsck_error error_func)\n {\n@@ -362,7 +445,8 @@ static int fsck_tag(struct tag *tag, const char *data,\n \n \tif (!tagged)\n \t\treturn error_func(&tag->object, FSCK_ERROR, \"could not load tagged object\");\n-\treturn 0;\n+\n+\treturn fsck_tag_buffer(tag, data, size, error_func);\n }\n \n int fsck_object(struct object *obj, void *data, unsigned long size,\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249228","messageId":"d88ae0c81b7b2ca32abe2abb2133ba3a5d6256dd.1410445431.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410445430.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 5/6] Add regression tests for stricter tag fsck'ing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:26:41Z","receivedAt":"2014-09-11T14:26:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The intent of the new test case is to catch general breakages in\nthe fsck_tag() function, not so much to test it extensively, trying to\nstrike the proper balance between thoroughness and speed.\n\nWhile it *would* have been nice to test the code path where fsck_object()\nencounters an invalid tag object, this is not possible using git fsck: tag\nobjects are parsed already before fsck'ing (and the parser already fails\nupon such objects).\n\nEven worse: we would not even be able write out invalid tag objects\nbecause git hash-object parses those objects, too, unless we resorted to\nreally ugly hacks such as using something like this in the unit tests\n(essentially depending on Perl *and* Compress::Zlib):\n\n\thash_invalid_object () {\n\t\tcontents=\"$(printf '%s %d\\0%s' \"$1\" ${#2} \"$2\")\" &&\n\t\tsha1=$(echo \"$contents\" | test-sha1) &&\n\t\tsuffix=${sha1#??} &&\n\t\tmkdir -p .git/objects/${sha1%$suffix} &&\n\t\techo \"$contents\" |\n\t\tperl -MCompress::Zlib -e 'undef $/; print compress(<>)' \\\n\t\t\t> .git/objects/${sha1%$suffix}/$suffix &&\n\t\techo $sha1\n\t}\n\nSigned-off-by: Johannes Schindelin \n---\n t/t1450-fsck.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8c739c9..2b6a6f2 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -194,6 +194,26 @@ test_expect_success 'tag pointing to something else than its type' '\n \ttest_must_fail git fsck --tags\n '\n \n+test_expect_success 'tag with incorrect tag name & missing tagger' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tcat >wrong-tag <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag wrong name format\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object -t tag -w --stdin .git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\tgit fsck --tags 2>out &&\n+\tcat out &&\n+\tgrep \"invalid .tag. name\" out &&\n+\tgrep \"expected .tagger. line\" out\n+'\n+\n test_expect_success 'cleaned up' '\n \tgit fsck >actual 2>&1 &&\n \ttest_cmp empty actual\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249227","messageId":"2738eace005dce9002c1a1f5e87ad63aebdf83ef.1410445431.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410445430.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 6/6] Make sure that index-pack --strict checks tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T14:26:45Z","receivedAt":"2014-09-11T14:26:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"One of the most important use cases for the strict tag object checking\nis when transfer.fsckobjects is set to true to catch invalid objects\nearly on. This new regression test essentially tests the same code path\nby directly calling 'index-pack --strict' on a pack containing an\ntag object without a 'tagger' line.\n\nTechnically, this test is not enough: it only exercises a code path that\n*warns*, not one that *fails*. The reason is that it would be exquisitely\nconvoluted to test that: not only hash-object, but also pack-index\nactually *parse* tag objects when encountering them. Therefore we would\nhave to actively *break* pack-index in order to test this. Or rewrite\nboth hash-object and pack-index in shell script. Ain't gonna happen.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5302-pack-index.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 4bbb718..4d033df 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object store' '\n test -f .git/objects/pack/pack-${pack1}.idx\n '\n \n+test_expect_success 'index-pack --strict warns upon missing tagger in tag' '\n+ sha=$(git rev-parse HEAD) &&\n+ cat >wrong-tag < err &&\n+ grep \"^error:.* expected .tagger. line\" err\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249233","messageId":"xmqqegvif6gx.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409111352130.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T16:49:02Z","receivedAt":"2014-09-11T16:49:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> > +\tfor (i = 0; i < size; i++) {\n>> > +\t\tswitch (buffer[i]) {\n>> > +\t\tcase '\\0':\n>> > +\t\t\treturn error_func(obj, FSCK_ERROR,\n>> > +\t\t\t\t\"invalid message: NUL at offset %d\", i);\n>> \n>> Isn't this \"invalid header\"? After all we haven't escaped this loop\n>> and haven't seen the message part of the commit object (and it is\n>> the same if you are going to later reuse this for tag objects).\n>\n> My reasoning for keeping it saying \"message\" was that a message consists\n> of a header and a body. I will change it to \"unterminated header\" instead,\n> also in the error message when no NUL was found.\n\nBecause end users think of a \"message\" in the context of discussing\neither a commit or a tag as what they give as the value to the \"-m\"\noption, the object payload consists of a header and a body the\nlatter of which *is* the message to them. Choosing a word that is\nnot \"message\" is a good way to avoid potential confusion here.\n"},{"id":"249234","messageId":"xmqqa966f6eg.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"alpine.DEB.1.00.1409111620420.990@s15462909.onlinehome-server.info","subject":"Re: [PATCH v2 6/6] Make sure that index-pack --strict fails upon invalid tag objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T16:50:31Z","receivedAt":"2014-09-11T16:50:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi,\n>\n> On Wed, 10 Sep 2014, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> > + tag=$(git hash-object -t tag -w --stdin > > + pack1=$(echo $tag | git pack-objects tag-test) &&\n>> > + echo remove tag object &&\n>> > + thirtyeight=${tag#??} &&\n>> > + rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight &&\n>> > + test_must_fail git index-pack --strict tag-test-${pack1}.pack 2> err &&\n>> \n>> I had to drop \"must fail\" from this one (will amend the \"SQUASH???\"\n>> again).\n>\n> Funny. It failed here, but for the wrong reason: index-pack --strict\n> failed here because the object referenced by the tag object was not in the\n> pack.\n\nThat is strange. It failed with the version you sent to the list\nfor me for a different reason---it tried to verify the ident that\ndid not exist in the tested object (which we fixed in the squash).\n"},{"id":"249236","messageId":"alpine.DEB.1.00.1409111903280.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"xmqqa966f6eg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 6/6] Make sure that index-pack --strict fails upon invalid tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-11T17:04:25Z","receivedAt":"2014-09-11T17:04:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 11 Sep 2014, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > On Wed, 10 Sep 2014, Junio C Hamano wrote:\n> >\n> >> Johannes Schindelin writes:\n> >> \n> >> > + tag=$(git hash-object -t tag -w --stdin >> > + pack1=$(echo $tag | git pack-objects tag-test) &&\n> >> > + echo remove tag object &&\n> >> > + thirtyeight=${tag#??} &&\n> >> > + rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight &&\n> >> > + test_must_fail git index-pack --strict tag-test-${pack1}.pack 2> err &&\n> >> \n> >> I had to drop \"must fail\" from this one (will amend the \"SQUASH???\"\n> >> again).\n> >\n> > Funny. It failed here, but for the wrong reason: index-pack --strict\n> > failed here because the object referenced by the tag object was not in the\n> > pack.\n> \n> That is strange. It failed with the version you sent to the list\n> for me for a different reason---it tried to verify the ident that\n> did not exist in the tested object (which we fixed in the squash).\n\nHmm. It is very well possible that I ran the tests in the middle of a\nrebase, i.e. without my changes to t5302. Will pay more attention next\ntime, sorry!\n\nCiao,\nDscho\n"},{"id":"249243","messageId":"xmqqha0edony.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"2738eace005dce9002c1a1f5e87ad63aebdf83ef.1410445431.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 6/6] Make sure that index-pack --strict checks tag objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T17:58:57Z","receivedAt":"2014-09-11T17:58:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> One of the most important use cases for the strict tag object checking\n> is when transfer.fsckobjects is set to true to catch invalid objects\n> early on. This new regression test essentially tests the same code path\n> by directly calling 'index-pack --strict' on a pack containing an\n> tag object without a 'tagger' line.\n>\n> Technically, this test is not enough: it only exercises a code path that\n> *warns*, not one that *fails*. The reason is that it would be exquisitely\n> convoluted to test that: not only hash-object, but also pack-index\n> actually *parse* tag objects when encountering them. Therefore we would\n> have to actively *break* pack-index in order to test this. Or rewrite\n> both hash-object and pack-index in shell script. Ain't gonna happen.\n\nIt is perfectly OK to feel and even say \"I am not going to do that\nin this series\" here, but is not very welcome to cast such a\nnegative \"Ain't gonna happen.\" attitude in stone in the log message\nin our history.\n\nWhen our toolset has become too tight without leaving enough escape\nhatch to hinder further development, it is very sensible to at least\nthink about adding a new \"--for-debug\" option to hash-object and\npack-objects that allows us to deliberately create broken\ndatastreams to test against.\n\nI think peff recently added helpers to t5303 to allow us test\ncorrupt pack streams, which is a way different from what you\nenvisioned above (i.e. \"actively break pack-index\") to test\nbreakages like the ones you were trying to test here.\n\nHaving said all that, I do think the series that added new warnings,\nerrors and a test for the new warnings is an improvement without a\ntest for the new errors. So let's queue this, see if somebody comes\nup a way to check for these error detection codepath on top of this\nseries.\n\nThanks.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t5302-pack-index.sh | 19 +++++++++++++++++++\n> 1 file changed, 19 insertions(+)\n>\n> diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\n> index 4bbb718..4d033df 100755\n> --- a/t/t5302-pack-index.sh\n> +++ b/t/t5302-pack-index.sh\n> @@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object store' '\n> test -f .git/objects/pack/pack-${pack1}.idx\n> '\n> \n> +test_expect_success 'index-pack --strict warns upon missing tagger in tag' '\n> + sha=$(git rev-parse HEAD) &&\n> + cat >wrong-tag < +object $sha\n> +type commit\n> +tag guten tag\n> +\n> +This is an invalid tag.\n> +EOF\n> +\n> + tag=$(git hash-object -t tag -w --stdin + pack1=$(echo $tag $sha | git pack-objects tag-test) &&\n> + echo remove tag object &&\n> + thirtyeight=${tag#??} &&\n> + rm -f .git/objects/${tag%$thirtyeight}/$thirtyeight &&\n> + git index-pack --strict tag-test-${pack1}.pack 2> err &&\n\ns/2> err/2>err/;\n\n> + grep \"^error:.* expected .tagger. line\" err\n> +'\n> +\n> test_done\n"},{"id":"249259","messageId":"xmqqoauldfij.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"xmqqha0edony.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 6/6] Make sure that index-pack --strict checks tag objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T21:16:36Z","receivedAt":"2014-09-11T21:16:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> When our toolset has become too tight without leaving enough escape\n> hatch to hinder further development, it is very sensible to at least\n> think about adding a new \"--for-debug\" option to hash-object and\n> pack-objects that allows us to deliberately create broken\n> datastreams to test against.\n>\n> I think peff recently added helpers to t5303 to allow us test\n> corrupt pack streams, which is a way different from what you\n> envisioned above (i.e. \"actively break pack-index\") to test\n> breakages like the ones you were trying to test here.\n>\n> Having said all that, I do think the series that added new warnings,\n> errors and a test for the new warnings is an improvement without a\n> test for the new errors. So let's queue this, see if somebody comes\n> up a way to check for these error detection codepath on top of this\n> series.\n\nIt wasn't too painful to do one of them, and the result looks rather\nnice. It lets us add this patch on top of your series.\n\n-- >8 --\nSubject: [PATCH] t1450: make sure fsck detects a malformed tagger line\n\nWith \"hash-object --literally\", write a tag object that is not\nsupposed to pass one of the new checks added to \"fsck\", and make\nsure that the new check catches the breakage.\n\nSigned-off-by: Junio C Hamano \n---\n t/t1450-fsck.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 9118253..6ecb844 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -234,6 +234,25 @@ test_expect_success 'tag with incorrect tag name & missing tagger' '\n \tgrep \"expected .tagger. line\" out\n '\n \n+test_expect_success 'tag with bad tagger' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tcat >wrong-tag <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag not-quite-wrong\n+\ttagger Bad Tagger Name\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object --literally -t tag -w --stdin .git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\ttest_must_fail git fsck --tags 2>out &&\n+\tgrep \"error in tag .*: invalid author/committer\" out\n+'\n+\n test_expect_success 'cleaned up' '\n \tgit fsck >actual 2>&1 &&\n \ttest_cmp empty actual\n-- \n2.1.0-459-g1bc3b2b\n"},{"id":"249260","messageId":"1410470243-26552-1-git-send-email-gitster@pobox.com","threadId":"37436","inReplyTo":"xmqqoauldfij.fsf@gitster.dls.corp.google.com","subject":"[PATCH 0/3] hash-object --literally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T21:17:20Z","receivedAt":"2014-09-11T21:17:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Our toolset may have become too tight without leaving enough escape\nhatch to hinder further development. \"hash-object\" makes minimum\nsanity checks by default for a very good reason, but it means that\nwe cannot deliberately create broken datastreams to test against\nfsck and other codepaths that are supposed to detect and report such\nbroken data that we may encounter in the field, perhaps created by\nthird-party tools.\n\nThese changes teach a new \"--literally\" option to \"hash-object\" to\nallow us throw any garbage to create a broken loose object. You can\neven do something horrible like\n\n\tgit hash-object -t bogus --literally -w --stdin \" option or does not complain even if the\ncontents look unreasonable. But that is for another day (hint,\nhint).\n\nThe second patch is optional. I thought I may want to pass this as\na new HASH_LITERALLY bit down the callchain to index_fd(), but I\ndecided against it, as that will allow other codepaths to create\nbroken datastream, spreading this debugging aid a bit too widely for\nmy taste.\n\nJunio C Hamano (3):\n hash-object: reduce file-scope statics\n hash-object: pass 'write_object' as a flag\n hash-object: add --literally option\n\n builtin/hash-object.c | 103 ++++++++++++++++++++++++++++++--------------------\n 1 file changed, 61 insertions(+), 42 deletions(-)\n\n-- \n2.1.0-459-g1bc3b2b\n"},{"id":"249262","messageId":"1410470243-26552-2-git-send-email-gitster@pobox.com","threadId":"37436","inReplyTo":"1410470243-26552-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/3] hash-object: reduce file-scope statics","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T21:17:21Z","receivedAt":"2014-09-11T21:17:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Most of the knobs that affect helper functions called from\ncmd_hash_object() were passed to them as parameters already, and the\nonly effect of having them as file-scope statics was to make the\nreader wonder if the parameters are hiding the file-scope global\nvalues by accident. Adjust their initialisation and make them\nfunction-local variables.\n\nThe only exception was no_filters hash_stdin_paths() peeked from the\nfile-scope global, which was converted to a parameter to the helper\nfunction.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/hash-object.c | 52 +++++++++++++++++++++++----------------------------\n 1 file changed, 23 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex d7fcf4c..40008e2 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -36,9 +36,7 @@ static void hash_object(const char *path, const char *type, int write_object,\n \thash_fd(fd, type, write_object, vpath);\n }\n \n-static int no_filters;\n-\n-static void hash_stdin_paths(const char *type, int write_objects)\n+static void hash_stdin_paths(const char *type, int write_objects, int no_filters)\n {\n \tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n \n@@ -50,42 +48,38 @@ static void hash_stdin_paths(const char *type, int write_objects)\n \t\t\tstrbuf_swap(&buf, &nbuf);\n \t\t}\n \t\thash_object(buf.buf, type, write_objects,\n-\t\t no_filters ? NULL : buf.buf);\n+\t\t\t no_filters ? NULL : buf.buf);\n \t}\n \tstrbuf_release(&buf);\n \tstrbuf_release(&nbuf);\n }\n \n-static const char * const hash_object_usage[] = {\n-\tN_(\"git hash-object [-t ] [-w] [--path=|--no-filters] [--stdin] [--] ...\"),\n-\tN_(\"git hash-object --stdin-paths < \"),\n-\tNULL\n-};\n-\n-static const char *type;\n-static int write_object;\n-static int hashstdin;\n-static int stdin_paths;\n-static const char *vpath;\n-\n-static const struct option hash_object_options[] = {\n-\tOPT_STRING('t', NULL, &type, N_(\"type\"), N_(\"object type\")),\n-\tOPT_BOOL('w', NULL, &write_object, N_(\"write the object into the object database\")),\n-\tOPT_COUNTUP( 0 , \"stdin\", &hashstdin, N_(\"read the object from stdin\")),\n-\tOPT_BOOL( 0 , \"stdin-paths\", &stdin_paths, N_(\"read file names from stdin\")),\n-\tOPT_BOOL( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n-\tOPT_STRING( 0 , \"path\", &vpath, N_(\"file\"), N_(\"process file as it were from this path\")),\n-\tOPT_END()\n-};\n-\n int cmd_hash_object(int argc, const char **argv, const char *prefix)\n {\n+\tstatic const char * const hash_object_usage[] = {\n+\t\tN_(\"git hash-object [-t ] [-w] [--path=|--no-filters] [--stdin] [--] ...\"),\n+\t\tN_(\"git hash-object --stdin-paths < \"),\n+\t\tNULL\n+\t};\n+\tconst char *type = blob_type;\n+\tint hashstdin = 0;\n+\tint stdin_paths = 0;\n+\tint write_object = 0;\n+\tint no_filters = 0;\n+\tconst char *vpath = NULL;\n+\tconst struct option hash_object_options[] = {\n+\t\tOPT_STRING('t', NULL, &type, N_(\"type\"), N_(\"object type\")),\n+\t\tOPT_BOOL('w', NULL, &write_object, N_(\"write the object into the object database\")),\n+\t\tOPT_COUNTUP( 0 , \"stdin\", &hashstdin, N_(\"read the object from stdin\")),\n+\t\tOPT_BOOL( 0 , \"stdin-paths\", &stdin_paths, N_(\"read file names from stdin\")),\n+\t\tOPT_BOOL( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n+\t\tOPT_STRING( 0 , \"path\", &vpath, N_(\"file\"), N_(\"process file as it were from this path\")),\n+\t\tOPT_END()\n+\t};\n \tint i;\n \tint prefix_length = -1;\n \tconst char *errstr = NULL;\n \n-\ttype = blob_type;\n-\n \targc = parse_options(argc, argv, NULL, hash_object_options,\n \t\t\t hash_object_usage, 0);\n \n@@ -131,7 +125,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (stdin_paths)\n-\t\thash_stdin_paths(type, write_object);\n+\t\thash_stdin_paths(type, write_object, no_filters);\n \n \treturn 0;\n }\n-- \n2.1.0-459-g1bc3b2b\n"},{"id":"249263","messageId":"1410470243-26552-3-git-send-email-gitster@pobox.com","threadId":"37436","inReplyTo":"1410470243-26552-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] hash-object: pass 'write_object' as a flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T21:17:22Z","receivedAt":"2014-09-11T21:17:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of forcing callers of lower level functions write\n(write_object ? HASH_WRITE_OBJECT : 0), prepare the flag to be\npassed down in the callchain from the command line parser.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/hash-object.c | 32 +++++++++++++++-----------------\n 1 file changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 40008e2..1fb07ee 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -10,33 +10,31 @@\n #include \"parse-options.h\"\n #include \"exec_cmd.h\"\n \n-static void hash_fd(int fd, const char *type, int write_object, const char *path)\n+static void hash_fd(int fd, const char *type, const char *path, unsigned flags)\n {\n \tstruct stat st;\n \tunsigned char sha1[20];\n-\tunsigned flags = (HASH_FORMAT_CHECK |\n-\t\t\t (write_object ? HASH_WRITE_OBJECT : 0));\n \n \tif (fstat(fd, &st) < 0 ||\n \t index_fd(sha1, fd, &st, type_from_string(type), path, flags))\n-\t\tdie(write_object\n+\t\tdie((flags & HASH_WRITE_OBJECT)\n \t\t ? \"Unable to add %s to database\"\n \t\t : \"Unable to hash %s\", path);\n \tprintf(\"%s\\n\", sha1_to_hex(sha1));\n \tmaybe_flush_or_die(stdout, \"hash to stdout\");\n }\n \n-static void hash_object(const char *path, const char *type, int write_object,\n-\t\t\tconst char *vpath)\n+static void hash_object(const char *path, const char *type, const char *vpath,\n+\t\t\tunsigned flags)\n {\n \tint fd;\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0)\n \t\tdie_errno(\"Cannot open '%s'\", path);\n-\thash_fd(fd, type, write_object, vpath);\n+\thash_fd(fd, type, vpath, flags);\n }\n \n-static void hash_stdin_paths(const char *type, int write_objects, int no_filters)\n+static void hash_stdin_paths(const char *type, int no_filters, unsigned flags)\n {\n \tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n \n@@ -47,8 +45,7 @@ static void hash_stdin_paths(const char *type, int write_objects, int no_filters\n \t\t\t\tdie(\"line is badly quoted\");\n \t\t\tstrbuf_swap(&buf, &nbuf);\n \t\t}\n-\t\thash_object(buf.buf, type, write_objects,\n-\t\t\t no_filters ? NULL : buf.buf);\n+\t\thash_object(buf.buf, type, no_filters ? NULL : buf.buf, flags);\n \t}\n \tstrbuf_release(&buf);\n \tstrbuf_release(&nbuf);\n@@ -64,12 +61,13 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \tconst char *type = blob_type;\n \tint hashstdin = 0;\n \tint stdin_paths = 0;\n-\tint write_object = 0;\n \tint no_filters = 0;\n+\tunsigned flags = HASH_FORMAT_CHECK;\n \tconst char *vpath = NULL;\n \tconst struct option hash_object_options[] = {\n \t\tOPT_STRING('t', NULL, &type, N_(\"type\"), N_(\"object type\")),\n-\t\tOPT_BOOL('w', NULL, &write_object, N_(\"write the object into the object database\")),\n+\t\tOPT_BIT('w', NULL, &flags, N_(\"write the object into the object database\"),\n+\t\t\tHASH_WRITE_OBJECT),\n \t\tOPT_COUNTUP( 0 , \"stdin\", &hashstdin, N_(\"read the object from stdin\")),\n \t\tOPT_BOOL( 0 , \"stdin-paths\", &stdin_paths, N_(\"read file names from stdin\")),\n \t\tOPT_BOOL( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n@@ -83,7 +81,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, NULL, hash_object_options,\n \t\t\t hash_object_usage, 0);\n \n-\tif (write_object) {\n+\tif (flags & HASH_WRITE_OBJECT) {\n \t\tprefix = setup_git_directory();\n \t\tprefix_length = prefix ? strlen(prefix) : 0;\n \t\tif (vpath && prefix)\n@@ -113,19 +111,19 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (hashstdin)\n-\t\thash_fd(0, type, write_object, vpath);\n+\t\thash_fd(0, type, vpath, flags);\n \n \tfor (i = 0 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \n \t\tif (0 <= prefix_length)\n \t\t\targ = prefix_filename(prefix, prefix_length, arg);\n-\t\thash_object(arg, type, write_object,\n-\t\t\t no_filters ? NULL : vpath ? vpath : arg);\n+\t\thash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,\n+\t\t\t flags);\n \t}\n \n \tif (stdin_paths)\n-\t\thash_stdin_paths(type, write_object, no_filters);\n+\t\thash_stdin_paths(type, no_filters, flags);\n \n \treturn 0;\n }\n-- \n2.1.0-459-g1bc3b2b\n"},{"id":"249261","messageId":"1410470243-26552-4-git-send-email-gitster@pobox.com","threadId":"37436","inReplyTo":"1410470243-26552-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] hash-object: add --literally option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-11T21:17:23Z","receivedAt":"2014-09-11T21:17:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This is allows \"hash-object --stdin\" to just hash any garbage into a\n\"loose object\" that may not pass the standard object parsing check\nor fsck, so that different kind of corrupt objects third-party tools\nmay create can be imitated in our test suite. That would in turn\nallow us to test features that catch these corrupt objects.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/hash-object.c | 45 ++++++++++++++++++++++++++++++++++++---------\n 1 file changed, 36 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 1fb07ee..6158363 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -10,13 +10,36 @@\n #include \"parse-options.h\"\n #include \"exec_cmd.h\"\n \n-static void hash_fd(int fd, const char *type, const char *path, unsigned flags)\n+/*\n+ * This is to create corrupt objects for debugging and as such it\n+ * needs to bypass the data conversion performed by, and the type\n+ * limitation imposed by, index_fd() and its callees.\n+ */\n+static int hash_literally(unsigned char *sha1, int fd, const char *type, unsigned flags)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (strbuf_read(&buf, fd, 4096) < 0)\n+\t\tret = -1;\n+\telse if (flags & HASH_WRITE_OBJECT)\n+\t\tret = write_sha1_file(buf.buf, buf.len, type, sha1);\n+\telse\n+\t\tret = hash_sha1_file(buf.buf, buf.len, type, sha1);\n+\tstrbuf_release(&buf);\n+\treturn ret;\n+}\n+\n+static void hash_fd(int fd, const char *type, const char *path, unsigned flags,\n+\t\t int literally)\n {\n \tstruct stat st;\n \tunsigned char sha1[20];\n \n \tif (fstat(fd, &st) < 0 ||\n-\t index_fd(sha1, fd, &st, type_from_string(type), path, flags))\n+\t (literally\n+\t ? hash_literally(sha1, fd, type, flags)\n+\t : index_fd(sha1, fd, &st, type_from_string(type), path, flags)))\n \t\tdie((flags & HASH_WRITE_OBJECT)\n \t\t ? \"Unable to add %s to database\"\n \t\t : \"Unable to hash %s\", path);\n@@ -25,16 +48,17 @@ static void hash_fd(int fd, const char *type, const char *path, unsigned flags)\n }\n \n static void hash_object(const char *path, const char *type, const char *vpath,\n-\t\t\tunsigned flags)\n+\t\t\tunsigned flags, int literally)\n {\n \tint fd;\n \tfd = open(path, O_RDONLY);\n \tif (fd < 0)\n \t\tdie_errno(\"Cannot open '%s'\", path);\n-\thash_fd(fd, type, vpath, flags);\n+\thash_fd(fd, type, vpath, flags, literally);\n }\n \n-static void hash_stdin_paths(const char *type, int no_filters, unsigned flags)\n+static void hash_stdin_paths(const char *type, int no_filters, unsigned flags,\n+\t\t\t int literally)\n {\n \tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n \n@@ -45,7 +69,8 @@ static void hash_stdin_paths(const char *type, int no_filters, unsigned flags)\n \t\t\t\tdie(\"line is badly quoted\");\n \t\t\tstrbuf_swap(&buf, &nbuf);\n \t\t}\n-\t\thash_object(buf.buf, type, no_filters ? NULL : buf.buf, flags);\n+\t\thash_object(buf.buf, type, no_filters ? NULL : buf.buf, flags,\n+\t\t\t literally);\n \t}\n \tstrbuf_release(&buf);\n \tstrbuf_release(&nbuf);\n@@ -62,6 +87,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \tint hashstdin = 0;\n \tint stdin_paths = 0;\n \tint no_filters = 0;\n+\tint literally = 0;\n \tunsigned flags = HASH_FORMAT_CHECK;\n \tconst char *vpath = NULL;\n \tconst struct option hash_object_options[] = {\n@@ -71,6 +97,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t\tOPT_COUNTUP( 0 , \"stdin\", &hashstdin, N_(\"read the object from stdin\")),\n \t\tOPT_BOOL( 0 , \"stdin-paths\", &stdin_paths, N_(\"read file names from stdin\")),\n \t\tOPT_BOOL( 0 , \"no-filters\", &no_filters, N_(\"store file as is without filters\")),\n+\t\tOPT_BOOL( 0, \"literally\", &literally, N_(\"just hash any random garbage to create corrupt objects for debugging Git\")),\n \t\tOPT_STRING( 0 , \"path\", &vpath, N_(\"file\"), N_(\"process file as it were from this path\")),\n \t\tOPT_END()\n \t};\n@@ -111,7 +138,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (hashstdin)\n-\t\thash_fd(0, type, vpath, flags);\n+\t\thash_fd(0, type, vpath, flags, literally);\n \n \tfor (i = 0 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n@@ -119,11 +146,11 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix)\n \t\tif (0 <= prefix_length)\n \t\t\targ = prefix_filename(prefix, prefix_length, arg);\n \t\thash_object(arg, type, no_filters ? NULL : vpath ? vpath : arg,\n-\t\t\t flags);\n+\t\t\t flags, literally);\n \t}\n \n \tif (stdin_paths)\n-\t\thash_stdin_paths(type, no_filters, flags);\n+\t\thash_stdin_paths(type, no_filters, flags, literally);\n \n \treturn 0;\n }\n-- \n2.1.0-459-g1bc3b2b\n"},{"id":"249287","messageId":"alpine.DEB.1.00.1409120952380.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"xmqqoauldfij.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 6/6] Make sure that index-pack --strict checks tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:04:30Z","receivedAt":"2014-09-12T08:04:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 11 Sep 2014, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > When our toolset has become too tight without leaving enough escape\n> > hatch to hinder further development, it is very sensible to at least\n> > think about adding a new \"--for-debug\" option to hash-object and\n> > pack-objects that allows us to deliberately create broken\n> > datastreams to test against.\n> >\n> > [...]\n> \n> It wasn't too painful to do one of them, and the result looks rather\n> nice.\n\nI was loathe to make it easier for interested parties to create invalid\nGit objects and to push them onto servers that cannot yet benefit from my\npatch series. At the very least, I would have preferred to put such\nfunctionality into test-* executables (where I searched for that\nfunctionality in the first place), i.e. outside the distributed binaries.\n\nBut since you already did the work and it does the job, I won't worry\nabout it.\n\nA bigger worry is that the additional test verifies that fsck catches the\ninvalid tag object and exits, when we really want to be certain that \"git\nfetch --strict\" will abort on such an object. So the test is still\nindirect, although I admit that it is closer now to what we want.\n\nVersion 4 of the patch series (without your hash-object --literally patch\nbecause mailed patch series cannot declare on what branches from 'pu' they\nrely, I always base my patch series on 'next' for that reason [*1*])\ncoming up.\n\nCiao,\nDscho\n\nFootnote *1*: As always, I push my patch series to a topic branch on\nGitHub. The one corresponding to the upcoming patch series is in\nhttps://github.com/dscho/git/compare/next...fsck-tag, the one with your\nadditional test in\nhttps://github.com/dscho/git/compare/next...fsck-tag-plus (the latter\nbeing a thicket rather than a linear topic branch).\n"},{"id":"249288","messageId":"cover.1410509168.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410445430.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:07:43Z","receivedAt":"2014-09-12T08:07:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This patch series introduces detailed checking of tag objects when\ncalling git fsck, and also when transfer.fsckobjects is set to true.\n\nTo this end, the fsck machinery is reworked to accept the buffer and\nsize of the object to check, and for commit and tag objects, we verify\nthat the buffers contain an end of header (i.e. an empty line) to\nguarantee that our checks do not run beyond the buffer.\n\nThis work was sponsored by GitHub.\n\nChanges since v3:\n\n- removed undesired negativity from commit message\n\n- removed space in '2> err'\n\nJohannes Schindelin (6):\n Refactor type_from_string() to avoid die()ing in case of errors\n Accept object data in the fsck_object() function\n Make sure fsck_commit_buffer() does not run out of the buffer\n fsck: check tag objects' headers\n Add regression tests for stricter tag fsck'ing\n Make sure that index-pack --strict checks tag objects\n\n builtin/fsck.c | 2 +-\n builtin/index-pack.c | 3 +-\n builtin/unpack-objects.c | 14 +++--\n fsck.c | 133 +++++++++++++++++++++++++++++++++++++++++++----\n fsck.h | 4 +-\n object.c | 11 +++-\n object.h | 3 +-\n t/t1450-fsck.sh | 20 +++++++\n t/t5302-pack-index.sh | 19 +++++++\n 9 files changed, 189 insertions(+), 20 deletions(-)\n\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249289","messageId":"7683fa48644b0958f8e87fbb5db51e362ea40e85.1410509168.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410509168.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 1/6] Refactor type_from_string() to avoid die()ing in case of errors","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:07:49Z","receivedAt":"2014-09-12T08:07:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"In the next commits, we will enhance the fsck_tag() function to check\ntag objects more thoroughly. To this end, we need a function to verify\nthat a given string is a valid object type, but that does not die() in\nthe negative case.\n\nWhile at it, prepare type_from_string() for counted strings, i.e. strings\nwith an explicitly specified length rather than a NUL termination.\n\nSigned-off-by: Johannes Schindelin \n---\n object.c | 11 +++++++++--\n object.h | 3 ++-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex a16b9f9..aedac24 100644\n--- a/object.c\n+++ b/object.c\n@@ -33,13 +33,20 @@ const char *typename(unsigned int type)\n \treturn object_type_strings[type];\n }\n \n-int type_from_string(const char *str)\n+int type_from_string_gently(const char *str, ssize_t len, int gentle)\n {\n \tint i;\n \n+\tif (len < 0)\n+\t\tlen = strlen(str);\n+\n \tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n-\t\tif (!strcmp(str, object_type_strings[i]))\n+\t\tif (!strncmp(str, object_type_strings[i], len))\n \t\t\treturn i;\n+\n+\tif (gentle)\n+\t\treturn -1;\n+\n \tdie(\"invalid object type \\\"%s\\\"\", str);\n }\n \ndiff --git a/object.h b/object.h\nindex 5e8d8ee..e028ced 100644\n--- a/object.h\n+++ b/object.h\n@@ -53,7 +53,8 @@ struct object {\n };\n \n extern const char *typename(unsigned int type);\n-extern int type_from_string(const char *str);\n+extern int type_from_string_gently(const char *str, ssize_t, int gentle);\n+#define type_from_string(str) type_from_string_gently(str, -1, 0)\n \n /*\n * Return the current number of buckets in the object hashmap.\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249290","messageId":"7bccb22f4c3e5aa45d3552e7acce1c360393a610.1410509168.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410509168.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 2/6] Accept object data in the fsck_object() function","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:07:55Z","receivedAt":"2014-09-12T08:07:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When fsck'ing an incoming pack, we need to fsck objects that cannot be\nread via read_sha1_file() because they are not local yet (and might even\nbe rejected if transfer.fsckobjects is set to 'true').\n\nFor commits, there is a hack in place: we basically cache commit\nobjects' buffers anyway, but the same is not true, say, for tag objects.\n\nBy refactoring fsck_object() to take the object buffer and size as\noptional arguments -- optional, because we still fall back to the\nprevious method to look at the cached commit objects if the caller\npasses NULL -- we prepare the machinery for the upcoming handling of tag\nobjects.\n\nThe assumption that such buffers are inherently NUL terminated is now\nwrong, of course, hence we pass the size of the buffer so that we can\nadd a sanity check later, to prevent running past the end of the buffer.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/fsck.c | 2 +-\n builtin/index-pack.c | 3 ++-\n builtin/unpack-objects.c | 14 ++++++++++----\n fsck.c | 24 +++++++++++++++---------\n fsck.h | 4 +++-\n 5 files changed, 31 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex d42a27d..d9f4e6e 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -298,7 +298,7 @@ static int fsck_obj(struct object *obj)\n \n \tif (fsck_walk(obj, mark_used, NULL))\n \t\tobjerror(obj, \"broken links\");\n-\tif (fsck_object(obj, check_strict, fsck_error_func))\n+\tif (fsck_object(obj, NULL, 0, check_strict, fsck_error_func))\n \t\treturn -1;\n \n \tif (obj->type == OBJ_TREE) {\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 5568a5b..f2465ff 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -773,7 +773,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n \t\t\tif (do_fsck_object &&\n-\t\t\t fsck_object(obj, 1, fsck_error_function))\n+\t\t\t fsck_object(obj, buf, size, 1,\n+\t\t\t\t fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 99cde45..855d94b 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -164,10 +164,10 @@ static unsigned nr_objects;\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+static void write_cached_object(struct object *obj, struct obj_buffer *obj_buf)\n {\n \tunsigned char sha1[20];\n-\tstruct obj_buffer *obj_buf = lookup_object_buffer(obj);\n+\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@@ -180,6 +180,8 @@ static void write_cached_object(struct object *obj)\n */\n static int check_object(struct object *obj, int type, void *data)\n {\n+\tstruct obj_buffer *obj_buf;\n+\n \tif (!obj)\n \t\treturn 1;\n \n@@ -198,11 +200,15 @@ static int check_object(struct object *obj, int type, void *data)\n \t\treturn 0;\n \t}\n \n-\tif (fsck_object(obj, 1, fsck_error_function))\n+\tobj_buf = lookup_object_buffer(obj);\n+\tif (!obj_buf)\n+\t\tdie(\"Whoops! Cannot find object '%s'\", sha1_to_hex(obj->sha1));\n+\tif (fsck_object(obj, obj_buf->buffer, obj_buf->size, 1,\n+\t\t\tfsck_error_function))\n \t\tdie(\"Error in object\");\n \tif (fsck_walk(obj, check_object, NULL))\n \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n-\twrite_cached_object(obj);\n+\twrite_cached_object(obj, obj_buf);\n \treturn 0;\n }\n \ndiff --git a/fsck.c b/fsck.c\nindex 56156ff..dd77628 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -277,7 +277,7 @@ static int fsck_ident(const char **ident, struct object *obj, fsck_error error_f\n }\n \n static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n-\t\t\t fsck_error error_func)\n+\tunsigned long size, fsck_error error_func)\n {\n \tunsigned char tree_sha1[20], sha1[20];\n \tstruct commit_graft *graft;\n@@ -322,15 +322,18 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, fsck_error error_func)\n+static int fsck_commit(struct commit *commit, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n-\tconst char *buffer = get_commit_buffer(commit, NULL);\n-\tint ret = fsck_commit_buffer(commit, buffer, error_func);\n-\tunuse_commit_buffer(commit, buffer);\n+\tconst char *buffer = data ? data : get_commit_buffer(commit, &size);\n+\tint ret = fsck_commit_buffer(commit, buffer, size, error_func);\n+\tif (!data)\n+\t\tunuse_commit_buffer(commit, buffer);\n \treturn ret;\n }\n \n-static int fsck_tag(struct tag *tag, fsck_error error_func)\n+static int fsck_tag(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n {\n \tstruct object *tagged = tag->tagged;\n \n@@ -339,7 +342,8 @@ static int fsck_tag(struct tag *tag, fsck_error error_func)\n \treturn 0;\n }\n \n-int fsck_object(struct object *obj, int strict, fsck_error error_func)\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func)\n {\n \tif (!obj)\n \t\treturn error_func(obj, FSCK_ERROR, \"no valid object to fsck\");\n@@ -349,9 +353,11 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)\n \tif (obj->type == OBJ_TREE)\n \t\treturn fsck_tree((struct tree *) obj, strict, error_func);\n \tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit((struct commit *) obj, error_func);\n+\t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag((struct tag *) obj, error_func);\n+\t\treturn fsck_tag((struct tag *) obj, (const char *) data,\n+\t\t\tsize, error_func);\n \n \treturn error_func(obj, FSCK_ERROR, \"unknown type '%d' (internal fsck error)\",\n \t\t\t obj->type);\ndiff --git a/fsck.h b/fsck.h\nindex 1e4f527..d1e6387 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -28,6 +28,8 @@ int fsck_error_function(struct object *obj, int type, const char *fmt, ...);\n * 0\t\teverything OK\n */\n int fsck_walk(struct object *obj, fsck_walk_func walk, void *data);\n-int fsck_object(struct object *obj, int strict, fsck_error error_func);\n+/* If NULL is passed for data, we assume the object is local and read it. */\n+int fsck_object(struct object *obj, void *data, unsigned long size,\n+\tint strict, fsck_error error_func);\n \n #endif\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249291","messageId":"c69f1e2649fd0a79e8362702203633251bcc9ea4.1410509168.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410509168.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 3/6] Make sure fsck_commit_buffer() does not run out of the buffer","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:07:59Z","receivedAt":"2014-09-12T08:07:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"So far, we assumed that the buffer is NUL terminated, but this is not\na safe assumption, now that we opened the fsck_object() API to pass a\nbuffer directly.\n\nSo let's make sure that there is at least an empty line in the buffer.\nThat way, our checks would fail if the empty line was encountered\nprematurely, and consequently we can get away with the current string\ncomparisons even with non-NUL-terminated buffers are passed to\nfsck_object().\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex dd77628..73da6f8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -237,6 +237,26 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \treturn retval;\n }\n \n+static int require_end_of_header(const void *data, unsigned long size,\n+\tstruct object *obj, fsck_error error_func)\n+{\n+\tconst char *buffer = (const char *)data;\n+\tunsigned long i;\n+\n+\tfor (i = 0; i < size; i++) {\n+\t\tswitch (buffer[i]) {\n+\t\tcase '\\0':\n+\t\t\treturn error_func(obj, FSCK_ERROR,\n+\t\t\t\t\"unterminated header: NUL at offset %d\", i);\n+\t\tcase '\\n':\n+\t\t\tif (i + 1 < size && buffer[i + 1] == '\\n')\n+\t\t\t\treturn 0;\n+\t\t}\n+\t}\n+\n+\treturn error_func(obj, FSCK_ERROR, \"unterminated header\");\n+}\n+\n static int fsck_ident(const char **ident, struct object *obj, fsck_error error_func)\n {\n \tchar *end;\n@@ -284,6 +304,9 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \tunsigned parent_count, parent_line_count = 0;\n \tint err;\n \n+\tif (require_end_of_header(buffer, size, &commit->object, error_func))\n+\t\treturn -1;\n+\n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n \t\treturn error_func(&commit->object, FSCK_ERROR, \"invalid format - expected 'tree' line\");\n \tif (get_sha1_hex(buffer, tree_sha1) || buffer[40] != '\\n')\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249292","messageId":"517cb11bee7784006c86a3d21ffff2fa1def7ecc.1410509168.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410509168.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 4/6] fsck: check tag objects' headers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:08:05Z","receivedAt":"2014-09-12T08:08:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We inspect commit objects pretty much in detail in git-fsck, but we just\nglanced over the tag objects. Let's be stricter.\n\nSince we do not want to limit 'tag' lines unduly, values that would fail\nthe refname check only result in warnings, not errors.\n\nSigned-off-by: Johannes Schindelin \n---\n fsck.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 85 insertions(+), 1 deletion(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 73da6f8..2fffa43 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -6,6 +6,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"fsck.h\"\n+#include \"refs.h\"\n \n static int fsck_walk_tree(struct tree *tree, fsck_walk_func walk, void *data)\n {\n@@ -355,6 +356,88 @@ static int fsck_commit(struct commit *commit, const char *data,\n \treturn ret;\n }\n \n+static int fsck_tag_buffer(struct tag *tag, const char *data,\n+\tunsigned long size, fsck_error error_func)\n+{\n+\tunsigned char sha1[20];\n+\tint ret = 0;\n+\tconst char *buffer;\n+\tchar *to_free = NULL, *eol;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (data)\n+\t\tbuffer = data;\n+\telse {\n+\t\tenum object_type type;\n+\n+\t\tbuffer = to_free =\n+\t\t\tread_sha1_file(tag->object.sha1, &type, &size);\n+\t\tif (!buffer)\n+\t\t\treturn error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"cannot read tag object\");\n+\n+\t\tif (type != OBJ_TAG) {\n+\t\t\tret = error_func(&tag->object, FSCK_ERROR,\n+\t\t\t\t\"expected tag got %s\",\n+\t\t\t typename(type));\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+\tif (require_end_of_header(buffer, size, &tag->object, error_func))\n+\t\tgoto done;\n+\n+\tif (!skip_prefix(buffer, \"object \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'object' line\");\n+\t\tgoto done;\n+\t}\n+\tif (get_sha1_hex(buffer, sha1) || buffer[40] != '\\n') {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'object' line format - bad sha1\");\n+\t\tgoto done;\n+\t}\n+\tbuffer += 41;\n+\n+\tif (!skip_prefix(buffer, \"type \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'type' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\tif (type_from_string_gently(buffer, eol - buffer, 1) < 0)\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid 'type' value\");\n+\tif (ret)\n+\t\tgoto done;\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - expected 'tag' line\");\n+\t\tgoto done;\n+\t}\n+\teol = strchr(buffer, '\\n');\n+\tif (!eol) {\n+\t\tret = error_func(&tag->object, FSCK_ERROR, \"invalid format - unexpected end after 'type' line\");\n+\t\tgoto done;\n+\t}\n+\tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n+\tif (check_refname_format(sb.buf, 0))\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid 'tag' name: %s\", buffer);\n+\tbuffer = eol + 1;\n+\n+\tif (!skip_prefix(buffer, \"tagger \", &buffer))\n+\t\t/* early tags do not contain 'tagger' lines; warn only */\n+\t\terror_func(&tag->object, FSCK_WARN, \"invalid format - expected 'tagger' line\");\n+\telse\n+\t\tret = fsck_ident(&buffer, &tag->object, error_func);\n+\n+done:\n+\tstrbuf_release(&sb);\n+\tfree(to_free);\n+\treturn ret;\n+}\n+\n static int fsck_tag(struct tag *tag, const char *data,\n \tunsigned long size, fsck_error error_func)\n {\n@@ -362,7 +445,8 @@ static int fsck_tag(struct tag *tag, const char *data,\n \n \tif (!tagged)\n \t\treturn error_func(&tag->object, FSCK_ERROR, \"could not load tagged object\");\n-\treturn 0;\n+\n+\treturn fsck_tag_buffer(tag, data, size, error_func);\n }\n \n int fsck_object(struct object *obj, void *data, unsigned long size,\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249293","messageId":"69c1f8b3499986a7cd6148db0159fef0ee2d6366.1410509168.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410509168.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 5/6] Add regression tests for stricter tag fsck'ing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:08:12Z","receivedAt":"2014-09-12T08:08:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The intent of the new test case is to catch general breakages in\nthe fsck_tag() function, not so much to test it extensively, trying to\nstrike the proper balance between thoroughness and speed.\n\nWhile it *would* have been nice to test the code path where fsck_object()\nencounters an invalid tag object, this is not possible using git fsck: tag\nobjects are parsed already before fsck'ing (and the parser already fails\nupon such objects).\n\nEven worse: we would not even be able write out invalid tag objects\nbecause git hash-object parses those objects, too, unless we resorted to\nreally ugly hacks such as using something like this in the unit tests\n(essentially depending on Perl *and* Compress::Zlib):\n\n\thash_invalid_object () {\n\t\tcontents=\"$(printf '%s %d\\0%s' \"$1\" ${#2} \"$2\")\" &&\n\t\tsha1=$(echo \"$contents\" | test-sha1) &&\n\t\tsuffix=${sha1#??} &&\n\t\tmkdir -p .git/objects/${sha1%$suffix} &&\n\t\techo \"$contents\" |\n\t\tperl -MCompress::Zlib -e 'undef $/; print compress(<>)' \\\n\t\t\t> .git/objects/${sha1%$suffix}/$suffix &&\n\t\techo $sha1\n\t}\n\nSigned-off-by: Johannes Schindelin \n---\n t/t1450-fsck.sh | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 8c739c9..2b6a6f2 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -194,6 +194,26 @@ test_expect_success 'tag pointing to something else than its type' '\n \ttest_must_fail git fsck --tags\n '\n \n+test_expect_success 'tag with incorrect tag name & missing tagger' '\n+\tsha=$(git rev-parse HEAD) &&\n+\tcat >wrong-tag <<-EOF &&\n+\tobject $sha\n+\ttype commit\n+\ttag wrong name format\n+\n+\tThis is an invalid tag.\n+\tEOF\n+\n+\ttag=$(git hash-object -t tag -w --stdin .git/refs/tags/wrong &&\n+\ttest_when_finished \"git update-ref -d refs/tags/wrong\" &&\n+\tgit fsck --tags 2>out &&\n+\tcat out &&\n+\tgrep \"invalid .tag. name\" out &&\n+\tgrep \"expected .tagger. line\" out\n+'\n+\n test_expect_success 'cleaned up' '\n \tgit fsck >actual 2>&1 &&\n \ttest_cmp empty actual\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249294","messageId":"71bf293f1e6fd292c8ae5725a5a71d5f21eea19d.1410509169.git.johannes.schindelin@gmx.de","threadId":"37436","inReplyTo":"cover.1410509168.git.johannes.schindelin@gmx.de","subject":"[PATCH v4 6/6] Make sure that index-pack --strict checks tag objects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-12T08:08:16Z","receivedAt":"2014-09-12T08:08:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"One of the most important use cases for the strict tag object checking\nis when transfer.fsckobjects is set to true to catch invalid objects\nearly on. This new regression test essentially tests the same code path\nby directly calling 'index-pack --strict' on a pack containing an\ntag object without a 'tagger' line.\n\nTechnically, this test is not enough: it only exercises a code path that\n*warns*, not one that *fails*. The reason is that hash-object and\npack-objects both insist on parsing the tag objects and would fail on\ninvalid tag objects at this time.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5302-pack-index.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex 4bbb718..61bc8da 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -243,4 +243,23 @@ test_expect_success 'running index-pack in the object store' '\n test -f .git/objects/pack/pack-${pack1}.idx\n '\n \n+test_expect_success 'index-pack --strict warns upon missing tagger in tag' '\n+ sha=$(git rev-parse HEAD) &&\n+ cat >wrong-tag <err &&\n+ grep \"^error:.* expected .tagger. line\" err\n+'\n+\n test_done\n-- \n2.0.0.rc3.9669.g840d1f9\n"},{"id":"249316","messageId":"xmqqwq98af97.fsf@gitster.dls.corp.google.com","threadId":"37436","inReplyTo":"cover.1410509168.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v4 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-12T18:02:44Z","receivedAt":"2014-09-12T18:02:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. I think this is ready for 'next' and then down to 'master'\nfor the next release.\n"},{"id":"249340","messageId":"alpine.DEB.1.00.1409131108090.990@s15462909.onlinehome-server.info","threadId":"37436","inReplyTo":"xmqqwq98af97.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v4 0/6] Improve tag checking in fsck and with transfer.fsckobjects","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2014-09-13T09:08:18Z","receivedAt":"2014-09-13T09:08:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 12 Sep 2014, Junio C Hamano wrote:\n\n> Thanks. I think this is ready for 'next' and then down to 'master'\n> for the next release.\n\nThank you!\nDscho\n"}]}