{"thread":{"id":"26418","subject":"[PATCH 1/2] Add const to parse_{commit,tag}_buffer()","startedAt":"2011-02-05T10:52:20Z","lastAt":"2011-02-18T12:49:32Z","messageCount":9,"participants":["Nguyễn Thái Ngọc Duy","Thomas Rast","Nguyen Thai Ngoc Duy","Junio C Hamano","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"160435","messageId":"1296903141-27075-1-git-send-email-pclouds@gmail.com","threadId":"26418","inReplyTo":null,"subject":"[PATCH 1/2] Add const to parse_{commit,tag}_buffer()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-05T10:52:20Z","receivedAt":"2011-02-05T10:52:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n commit.c |    6 +++---\n commit.h |    2 +-\n tag.c    |    2 +-\n tag.h    |    2 +-\n 4 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 74d6601..ac337c7 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -245,10 +245,10 @@ int unregister_shallow(const unsigned char *sha1)\n \treturn 0;\n }\n \n-int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n+int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size)\n {\n-\tchar *tail = buffer;\n-\tchar *bufptr = buffer;\n+\tconst char *tail = buffer;\n+\tconst char *bufptr = buffer;\n \tunsigned char parent[20];\n \tstruct commit_list **pptr;\n \tstruct commit_graft *graft;\ndiff --git a/commit.h b/commit.h\nindex eb6c5af..659c87c 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -38,7 +38,7 @@ struct commit *lookup_commit_reference_gently(const unsigned char *sha1,\n \t\t\t\t\t      int quiet);\n struct commit *lookup_commit_reference_by_name(const char *name);\n \n-int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size);\n+int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size);\n int parse_commit(struct commit *item);\n \n /* Find beginning and length of commit subject. */\ndiff --git a/tag.c b/tag.c\nindex f789744..ecf7c1e 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -56,7 +56,7 @@ static unsigned long parse_tag_date(const char *buf, const char *tail)\n \treturn strtoul(dateptr, NULL, 10);\n }\n \n-int parse_tag_buffer(struct tag *item, void *data, unsigned long size)\n+int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n {\n \tunsigned char sha1[20];\n \tchar type[20];\ndiff --git a/tag.h b/tag.h\nindex 8522370..5ee88e6 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -13,7 +13,7 @@ struct tag {\n };\n \n extern struct tag *lookup_tag(const unsigned char *sha1);\n-extern int parse_tag_buffer(struct tag *item, void *data, unsigned long size);\n+extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long size);\n extern int parse_tag(struct tag *item);\n extern struct object *deref_tag(struct object *, const char *, int);\n extern size_t parse_signature(const char *buf, unsigned long size);\n-- \n1.7.3.4.878.g439c7\n"},{"id":"160436","messageId":"1296903141-27075-2-git-send-email-pclouds@gmail.com","threadId":"26418","inReplyTo":"1296903141-27075-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/2] Make hash-object more robust against malformed objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-05T10:52:21Z","receivedAt":"2011-02-05T10:52:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commits, trees and tags have structure. Don't let users feed git\nwith malformed ones. Sooner or later git will die() when\nencountering them.\n\nNote that this patch does not check semantics. A tree that points\nto non-existent objects is perfectly OK (and should be so, users\nmay choose to add commit first, then its associated tree for example)\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/hash-object.c  |    2 +-\n cache.h                |    2 +-\n read-cache.c           |    2 +-\n sha1_file.c            |   54 +++++++++++++++++++++++++++++++++++++++++------\n t/t1007-hash-object.sh |   13 +++++++++++\n 5 files changed, 63 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/hash-object.c b/builtin/hash-object.c\nindex 080af1a..c90acdd 100644\n--- a/builtin/hash-object.c\n+++ b/builtin/hash-object.c\n@@ -15,7 +15,7 @@ static void hash_fd(int fd, const char *type, int write_object, const char *path\n \tstruct stat st;\n \tunsigned char sha1[20];\n \tif (fstat(fd, &st) < 0 ||\n-\t    index_fd(sha1, fd, &st, write_object, type_from_string(type), path))\n+\t    index_fd(sha1, fd, &st, write_object, type_from_string(type), path, 1))\n \t\tdie(write_object\n \t\t    ? \"Unable to add %s to database\"\n \t\t    : \"Unable to hash %s\", path);\ndiff --git a/cache.h b/cache.h\nindex d83d68c..9186a56 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -501,7 +501,7 @@ extern int ie_match_stat(const struct index_state *, struct cache_entry *, struc\n extern int ie_modified(const struct index_state *, struct cache_entry *, struct stat *, unsigned int);\n \n extern int ce_path_match(const struct cache_entry *ce, const char **pathspec);\n-extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path);\n+extern int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object, enum object_type type, const char *path, int format_check);\n extern int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object);\n extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);\n \ndiff --git a/read-cache.c b/read-cache.c\nindex 4f2e890..fbc12f3 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -92,7 +92,7 @@ static int ce_compare_data(struct cache_entry *ce, struct stat *st)\n \n \tif (fd >= 0) {\n \t\tunsigned char sha1[20];\n-\t\tif (!index_fd(sha1, fd, st, 0, OBJ_BLOB, ce->name))\n+\t\tif (!index_fd(sha1, fd, st, 0, OBJ_BLOB, ce->name, 0))\n \t\t\tmatch = hashcmp(sha1, ce->sha1);\n \t\t/* index_fd() closed the file descriptor already */\n \t}\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d86a8db..58ca858 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -13,6 +13,7 @@\n #include \"commit.h\"\n #include \"tag.h\"\n #include \"tree.h\"\n+#include \"tree-walk.h\"\n #include \"refs.h\"\n #include \"pack-revindex.h\"\n #include \"sha1-lookup.h\"\n@@ -2471,8 +2472,37 @@ int has_sha1_file(const unsigned char *sha1)\n \treturn has_loose_object(sha1);\n }\n \n+static void check_tree(const void *buf, size_t size)\n+{\n+\tstruct tree_desc desc;\n+\tstruct name_entry entry;\n+\n+\tinit_tree_desc(&desc, buf, size);\n+\twhile (tree_entry(&desc, &entry))\n+\t\t/* do nothing\n+\t\t * tree_entry() will die() on malformed entries */\n+\t\t;\n+}\n+\n+static void check_commit(const void *buf, size_t size)\n+{\n+\tstruct commit c;\n+\tmemset(&c, 0, sizeof(c));\n+\tif (parse_commit_buffer(&c, buf, size))\n+\t\tdie(\"corrupt commit\");\n+}\n+\n+static void check_tag(const void *buf, size_t size)\n+{\n+\tstruct tag t;\n+\tmemset(&t, 0, sizeof(t));\n+\tif (parse_tag_buffer(&t, buf, size))\n+\t\tdie(\"corrupt tag\");\n+}\n+\n static int index_mem(unsigned char *sha1, void *buf, size_t size,\n-\t\t     int write_object, enum object_type type, const char *path)\n+\t\t     int write_object, enum object_type type,\n+\t\t     const char *path, int format_check)\n {\n \tint ret, re_allocated = 0;\n \n@@ -2490,6 +2520,14 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \t\t\tre_allocated = 1;\n \t\t}\n \t}\n+\tif (format_check) {\n+\t\tif (type == OBJ_TREE)\n+\t\t\tcheck_tree(buf, size);\n+\t\tif (type == OBJ_COMMIT)\n+\t\t\tcheck_commit(buf, size);\n+\t\tif (type == OBJ_TAG)\n+\t\t\tcheck_tag(buf, size);\n+\t}\n \n \tif (write_object)\n \t\tret = write_sha1_file(buf, size, typename(type), sha1);\n@@ -2503,7 +2541,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n #define SMALL_FILE_SIZE (32*1024)\n \n int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n-\t     enum object_type type, const char *path)\n+\t     enum object_type type, const char *path, int format_check)\n {\n \tint ret;\n \tsize_t size = xsize_t(st->st_size);\n@@ -2512,23 +2550,25 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,\n \t\tstruct strbuf sbuf = STRBUF_INIT;\n \t\tif (strbuf_read(&sbuf, fd, 4096) >= 0)\n \t\t\tret = index_mem(sha1, sbuf.buf, sbuf.len, write_object,\n-\t\t\t\t\ttype, path);\n+\t\t\t\t\ttype, path, format_check);\n \t\telse\n \t\t\tret = -1;\n \t\tstrbuf_release(&sbuf);\n \t} else if (!size) {\n-\t\tret = index_mem(sha1, NULL, size, write_object, type, path);\n+\t\tret = index_mem(sha1, NULL, size, write_object, type, path,\n+\t\t\t\tformat_check);\n \t} else if (size <= SMALL_FILE_SIZE) {\n \t\tchar *buf = xmalloc(size);\n \t\tif (size == read_in_full(fd, buf, size))\n \t\t\tret = index_mem(sha1, buf, size, write_object, type,\n-\t\t\t\t\tpath);\n+\t\t\t\t\tpath, format_check);\n \t\telse\n \t\t\tret = error(\"short read %s\", strerror(errno));\n \t\tfree(buf);\n \t} else {\n \t\tvoid *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);\n-\t\tret = index_mem(sha1, buf, size, write_object, type, path);\n+\t\tret = index_mem(sha1, buf, size, write_object, type, path,\n+\t\t\t\tformat_check);\n \t\tmunmap(buf, size);\n \t}\n \tclose(fd);\n@@ -2546,7 +2586,7 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write\n \t\tif (fd < 0)\n \t\t\treturn error(\"open(\\\"%s\\\"): %s\", path,\n \t\t\t\t     strerror(errno));\n-\t\tif (index_fd(sha1, fd, st, write_object, OBJ_BLOB, path) < 0)\n+\t\tif (index_fd(sha1, fd, st, write_object, OBJ_BLOB, path, 0) < 0)\n \t\t\treturn error(\"%s: failed to insert into database\",\n \t\t\t\t     path);\n \t\tbreak;\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex dd32432..6d52b82 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -188,4 +188,17 @@ for args in \"-w --stdin-paths\" \"--stdin-paths -w\"; do\n \tpop_repo\n done\n \n+test_expect_success 'corrupt tree' '\n+\techo abc >malformed-tree\n+\ttest_must_fail git hash-object -t tree malformed-tree\n+'\n+\n+test_expect_success 'corrupt commit' '\n+\ttest_must_fail git hash-object -t commit --stdin </dev/null\n+'\n+\n+test_expect_success 'corrupt tag' '\n+\ttest_must_fail git hash-object -t tag --stdin </dev/null\n+'\n+\n test_done\n-- \n1.7.3.4.878.g439c7\n"},{"id":"160945","messageId":"201102121242.21339.trast@student.ethz.ch","threadId":"26418","inReplyTo":"1296903141-27075-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/2] Make hash-object more robust against malformed objects","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-02-12T11:42:21Z","receivedAt":"2011-02-12T11:42:21Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n> Commits, trees and tags have structure. Don't let users feed git\n> with malformed ones. Sooner or later git will die() when\n> encountering them.\n> \n> Note that this patch does not check semantics. A tree that points\n> to non-existent objects is perfectly OK (and should be so, users\n> may choose to add commit first, then its associated tree for example)\n> \n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n\nThis patch makes t9350 fail under valgrind:\n\nexpecting success: \n\n        TAG=$(git hash-object -t tag -w tag-content) &&\n        git update-ref refs/tags/sonnenschein $TAG &&\n        git fast-export -C -C --signed-tags=strip --all > output &&\n        test $(grep -c \"^tag \" output) = 4 &&\n        ! grep \"Unspecified Tagger\" output &&\n        git fast-export -C -C --signed-tags=strip --all \\\n                --fake-missing-tagger > output &&\n        test $(grep -c \"^tag \" output) = 4 &&\n        grep \"Unspecified Tagger\" output\n\n\n==2862== Invalid read of size 1\n==2862==    at 0x4F0C34: prefixcmp (strbuf.c:9)\n==2862==    by 0x4F4FB3: parse_tag_buffer (tag.c:109)\n==2862==    by 0x4EC686: check_tag (sha1_file.c:2499)\n==2862==    by 0x4EC77F: index_mem (sha1_file.c:2529)\n==2862==    by 0x4EC934: index_fd (sha1_file.c:2563)\n==2862==    by 0x4379C9: hash_fd (hash-object.c:17)\n==2862==    by 0x437A86: hash_object (hash-object.c:33)\n==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)\n==2862==    by 0x404771: run_builtin (git.c:290)\n==2862==    by 0x4048FC: handle_internal_command (git.c:448)\n==2862==    by 0x4049E7: run_argv (git.c:492)\n==2862==    by 0x404B43: main (git.c:565)\n==2862==  Address 0x55abb77 is 0 bytes after a block of size 71 alloc'd\n==2862==    at 0x4C20E1C: malloc (vg_replace_malloc.c:195)\n==2862==    by 0x504A7C: xmalloc (wrapper.c:35)\n==2862==    by 0x4EC8E8: index_fd (sha1_file.c:2561)\n==2862==    by 0x4379C9: hash_fd (hash-object.c:17)\n==2862==    by 0x437A86: hash_object (hash-object.c:33)\n==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)\n==2862==    by 0x404771: run_builtin (git.c:290)\n==2862==    by 0x4048FC: handle_internal_command (git.c:448)\n==2862==    by 0x4049E7: run_argv (git.c:492)\n==2862==    by 0x404B43: main (git.c:565)\n==2862== \n==2862== Invalid read of size 1\n==2862==    at 0x4F0C50: prefixcmp (strbuf.c:10)\n==2862==    by 0x4F4FB3: parse_tag_buffer (tag.c:109)\n==2862==    by 0x4EC686: check_tag (sha1_file.c:2499)\n==2862==    by 0x4EC77F: index_mem (sha1_file.c:2529)\n==2862==    by 0x4EC934: index_fd (sha1_file.c:2563)\n==2862==    by 0x4379C9: hash_fd (hash-object.c:17)\n==2862==    by 0x437A86: hash_object (hash-object.c:33)\n==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)\n==2862==    by 0x404771: run_builtin (git.c:290)\n==2862==    by 0x4048FC: handle_internal_command (git.c:448)\n==2862==    by 0x4049E7: run_argv (git.c:492)\n==2862==    by 0x404B43: main (git.c:565)\n==2862==  Address 0x55abb77 is 0 bytes after a block of size 71 alloc'd\n==2862==    at 0x4C20E1C: malloc (vg_replace_malloc.c:195)\n==2862==    by 0x504A7C: xmalloc (wrapper.c:35)\n==2862==    by 0x4EC8E8: index_fd (sha1_file.c:2561)\n==2862==    by 0x4379C9: hash_fd (hash-object.c:17)\n==2862==    by 0x437A86: hash_object (hash-object.c:33)\n==2862==    by 0x437E82: cmd_hash_object (hash-object.c:126)\n==2862==    by 0x404771: run_builtin (git.c:290)\n==2862==    by 0x4048FC: handle_internal_command (git.c:448)\n==2862==    by 0x4049E7: run_argv (git.c:492)\n==2862==    by 0x404B43: main (git.c:565)\n==2862== \nnot ok - 15 cope with tagger-less tags\n#       \n#       \n#               TAG=$(git hash-object -t tag -w tag-content) &&\n#               git update-ref refs/tags/sonnenschein $TAG &&\n#               git fast-export -C -C --signed-tags=strip --all > output &&\n#               test $(grep -c \"^tag \" output) = 4 &&\n#               ! grep \"Unspecified Tagger\" output &&\n#               git fast-export -C -C --signed-tags=strip --all \\\n#                       --fake-missing-tagger > output &&\n#               test $(grep -c \"^tag \" output) = 4 &&\n#               grep \"Unspecified Tagger\" output\n#       \n#       \n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"160952","messageId":"20110212144706.GA25124@do","threadId":"26418","inReplyTo":"201102121242.21339.trast@student.ethz.ch","subject":"Re: [PATCH 2/2] Make hash-object more robust against malformed objects","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-12T14:47:06Z","receivedAt":"2011-02-12T14:47:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Feb 12, 2011 at 12:42:21PM +0100, Thomas Rast wrote:\n> This patch makes t9350 fail under valgrind:\n> \n> expecting success: \n> \n>         TAG=$(git hash-object -t tag -w tag-content) &&\n>         git update-ref refs/tags/sonnenschein $TAG &&\n>         git fast-export -C -C --signed-tags=strip --all > output &&\n>         test $(grep -c \"^tag \" output) = 4 &&\n>         ! grep \"Unspecified Tagger\" output &&\n>         git fast-export -C -C --signed-tags=strip --all \\\n>                 --fake-missing-tagger > output &&\n>         test $(grep -c \"^tag \" output) = 4 &&\n>         grep \"Unspecified Tagger\" output\n> \n> \n> ==2862== Invalid read of size 1\n> ==2862==    at 0x4F0C34: prefixcmp (strbuf.c:9)\n> ==2862==    by 0x4F4FB3: parse_tag_buffer (tag.c:109)\n\nNice. Does this fix it?\n\n--8<--\ndiff --git a/tag.c b/tag.c\nindex ecf7c1e..9318ae5 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -97,7 +97,9 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n \t\titem->tagged = NULL;\n \t}\n \n-\tif (prefixcmp(bufptr, \"tag \"))\n+\tif (bufptr + 5 < tail && !prefixcmp(bufptr, \"tag \"))\n+\t\t; \t\t/* good */\n+\telse\n \t\treturn -1;\n \tbufptr += 4;\n \tnl = memchr(bufptr, '\\n', tail - bufptr);\n@@ -106,7 +108,7 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n \titem->tag = xmemdupz(bufptr, nl - bufptr);\n \tbufptr = nl + 1;\n \n-\tif (!prefixcmp(bufptr, \"tagger \"))\n+\tif (bufptr + 8 < tail && !prefixcmp(bufptr, \"tagger \"))\n \t\titem->date = parse_tag_date(bufptr, tail);\n \telse\n \t\titem->date = 0;\n--8<--\n-- \nDuy\n"},{"id":"161042","messageId":"1297688571-1962-1-git-send-email-pclouds@gmail.com","threadId":"26418","inReplyTo":"20110212144706.GA25124@do","subject":"[PATCH] parse_tag_buffer(): do not prefixcmp() out of range","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-14T13:02:51Z","receivedAt":"2011-02-14T13:02:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a check (size < 64) at the beginning of the function, but\nthat only covers object+type lines.\n\nStrictly speaking the current code is still correct even if it\naccesses outside 'data' because 'tail' is used right after\nprefixcmp() calls.\n\nAnyway accessing out of range is not good. Avoid it.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Unfortunately I installed valgrind but could not reproduce t9350.15\n failure.\n\n Another option is to add prefixncmp(const char *, const char *,int).\n Probably not worth it.\n\n tag.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex ecf7c1e..9318ae5 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -97,7 +97,9 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n \t\titem->tagged = NULL;\n \t}\n \n-\tif (prefixcmp(bufptr, \"tag \"))\n+\tif (bufptr + 4 < tail && !prefixcmp(bufptr, \"tag \"))\n+\t\t; \t\t/* good */\n+\telse\n \t\treturn -1;\n \tbufptr += 4;\n \tnl = memchr(bufptr, '\\n', tail - bufptr);\n@@ -106,7 +108,7 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n \titem->tag = xmemdupz(bufptr, nl - bufptr);\n \tbufptr = nl + 1;\n \n-\tif (!prefixcmp(bufptr, \"tagger \"))\n+\tif (bufptr + 7 < tail && !prefixcmp(bufptr, \"tagger \"))\n \t\titem->date = parse_tag_date(bufptr, tail);\n \telse\n \t\titem->date = 0;\n-- \n1.7.4.74.g639db\n"},{"id":"161213","messageId":"7vsjvpm1xh.fsf@alter.siamese.dyndns.org","threadId":"26418","inReplyTo":"1297688571-1962-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-02-15T21:18:50Z","receivedAt":"2011-02-15T21:18:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> There is a check (size < 64) at the beginning of the function, but\n> that only covers object+type lines.\n>\n> Strictly speaking the current code is still correct even if it\n> accesses outside 'data' because 'tail' is used right after\n> prefixcmp() calls.\n\nWhat do you mean by this?  I don't get it.\n\n> diff --git a/tag.c b/tag.c\n> index ecf7c1e..9318ae5 100644\n> --- a/tag.c\n> +++ b/tag.c\n> @@ -97,7 +97,9 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n>  \t\titem->tagged = NULL;\n>  \t}\n>  \n> -\tif (prefixcmp(bufptr, \"tag \"))\n> +\tif (bufptr + 4 < tail && !prefixcmp(bufptr, \"tag \"))\n> +\t\t; \t\t/* good */\n> +\telse\n>  \t\treturn -1;\n>  \tbufptr += 4;\n>  \tnl = memchr(bufptr, '\\n', tail - bufptr);\n\nIf there weren't enough bytes between bufptr and tail, prefixcmp may still\nmatch with \"tag \" while later part of the matched string might be coming\nfrom trailing garbage outside our memory.  Unless we correctly fail the\nprefixcmp() part, memchr() would be fed negative value, no?\n"},{"id":"161257","messageId":"AANLkTin+zR81H0aahveMMOwKyxVV_wRoab=pqk51joop@mail.gmail.com","threadId":"26418","inReplyTo":"7vsjvpm1xh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-16T03:39:24Z","receivedAt":"2011-02-16T03:39:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/2/16 Junio C Hamano <gitster@pobox.com>:\n>> -     if (prefixcmp(bufptr, \"tag \"))\n>> +     if (bufptr + 4 < tail && !prefixcmp(bufptr, \"tag \"))\n>> +             ;               /* good */\n>> +     else\n>>               return -1;\n>>       bufptr += 4;\n>>       nl = memchr(bufptr, '\\n', tail - bufptr);\n>\n> If there weren't enough bytes between bufptr and tail, prefixcmp may still\n> match with \"tag \" while later part of the matched string might be coming\n> from trailing garbage outside our memory.  Unless we correctly fail the\n> prefixcmp() part, memchr() would be fed negative value, no?\n\nYes, memchr() would be fed negative, but prefixcmp() already steps\noutside allocated memory. I believe that caused valgrind error Thomas\nreported (although I couldn't reproduce it).\n\n> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n>\n>> There is a check (size < 64) at the beginning of the function, but\n>> that only covers object+type lines.\n>>\n>> Strictly speaking the current code is still correct even if it\n>> accesses outside 'data' because 'tail' is used right after\n>> prefixcmp() calls.\n>\n> What do you mean by this?  I don't get it.\n\nBecause memchr() would be fed negative, memchr() would fail so the\ncode is still correct.\n-- \nDuy\n"},{"id":"161416","messageId":"4D5D17F7.4010003@lsrfire.ath.cx","threadId":"26418","inReplyTo":"AANLkTin+zR81H0aahveMMOwKyxVV_wRoab=pqk51joop@mail.gmail.com","subject":"Re: [PATCH] parse_tag_buffer(): do not prefixcmp() out of range","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-02-17T12:43:35Z","receivedAt":"2011-02-17T12:43:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.02.2011 04:39, schrieb Nguyen Thai Ngoc Duy:\n> 2011/2/16 Junio C Hamano<gitster@pobox.com>:\n>>> -     if (prefixcmp(bufptr, \"tag \"))\n>>> +     if (bufptr + 4<  tail&&  !prefixcmp(bufptr, \"tag \"))\n>>> +             ;               /* good */\n>>> +     else\n>>>                return -1;\n>>>        bufptr += 4;\n>>>        nl = memchr(bufptr, '\\n', tail - bufptr);\n>>\n>> If there weren't enough bytes between bufptr and tail, prefixcmp may still\n>> match with \"tag \" while later part of the matched string might be coming\n>> from trailing garbage outside our memory.  Unless we correctly fail the\n>> prefixcmp() part, memchr() would be fed negative value, no?\n>\n> Yes, memchr() would be fed negative, but prefixcmp() already steps\n> outside allocated memory. I believe that caused valgrind error Thomas\n> reported (although I couldn't reproduce it).\n>\n>> Nguyễn Thái Ngọc Duy<pclouds@gmail.com>  writes:\n>>\n>>> There is a check (size<  64) at the beginning of the function, but\n>>> that only covers object+type lines.\n>>>\n>>> Strictly speaking the current code is still correct even if it\n>>> accesses outside 'data' because 'tail' is used right after\n>>> prefixcmp() calls.\n>>\n>> What do you mean by this?  I don't get it.\n>\n> Because memchr() would be fed negative, memchr() would fail so the\n> code is still correct.\n\nmemchr() won't notice if a negative value has been passed as third \nparameter because its type is size_t, which is unsigned.  Negative \nvalues are converted to big positive ones..\n\nRené\n"},{"id":"161540","messageId":"1298033372-29069-1-git-send-email-pclouds@gmail.com","threadId":"26418","inReplyTo":"4D5D17F7.4010003@lsrfire.ath.cx","subject":"[PATCH] parse_tag_buffer(): avoid out of bound access","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-02-18T12:49:32Z","receivedAt":"2011-02-18T12:49:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a check (size < 64) at the beginning of the function, but\nthat only covers object+type lines. Code for parsing \"tag\" and\n\"tagger\" may access outside buffer. Fix it.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n On Thu, Feb 17, 2011 at 7:43 PM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:\n > memchr() won't notice if a negative value has been passed as third parameter\n > because its type is size_t, which is unsigned.  Negative values are\n > converted to big positive ones..\n\n I did not notice that. Fixed commit message.\n\n tag.c |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex ecf7c1e..7d38cc0 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -97,7 +97,9 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n \t\titem->tagged = NULL;\n \t}\n \n-\tif (prefixcmp(bufptr, \"tag \"))\n+\tif (bufptr + 4 < tail && !prefixcmp(bufptr, \"tag \"))\n+\t\t; \t\t/* good */\n+\telse\n \t\treturn -1;\n \tbufptr += 4;\n \tnl = memchr(bufptr, '\\n', tail - bufptr);\n@@ -106,7 +108,7 @@ int parse_tag_buffer(struct tag *item, const void *data, unsigned long size)\n \titem->tag = xmemdupz(bufptr, nl - bufptr);\n \tbufptr = nl + 1;\n \n-\tif (!prefixcmp(bufptr, \"tagger \"))\n+\tif (bufptr + 7 < tail && !prefixcmp(bufptr, \"tagger \"))\n \t\titem->date = parse_tag_date(bufptr, tail);\n \telse\n \t\titem->date = 0;\n-- \n1.7.4.74.g639db\n"}]}