{"thread":{"id":"11678","subject":"[PATCH] parse_commit_buffer: don't parse invalid commits","startedAt":"2008-01-19T17:35:23Z","lastAt":"2008-01-20T16:11:05Z","messageCount":3,"participants":["Martin Koegler","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"65980","messageId":"1200764123283-git-send-email-mkoegler@auto.tuwien.ac.at","threadId":"11678","inReplyTo":null,"subject":"[PATCH] parse_commit_buffer: don't parse invalid commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-19T17:35:23Z","receivedAt":"2008-01-19T17:35:23Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"* check, that the tree line ends with \\n\n* prevent parse_commit_date from reading beyond the buffer,\n  - if author line does not end with \\n\n  - if committer line does not contain >\n* verify in parse_commit_date, that the commiter line ends with \\n\n\nSigned-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>\n---\n commit.c |   27 ++++++++++++++++++++-------\n 1 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex f074811..8b8fb04 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -48,19 +48,32 @@ struct commit *lookup_commit(const unsigned char *sha1)\n \treturn check_commit(obj, sha1, 0);\n }\n \n-static unsigned long parse_commit_date(const char *buf)\n+static unsigned long parse_commit_date(const char *buf, const char *tail)\n {\n \tunsigned long date;\n+\tconst char *dateptr;\n \n+\tif (buf + 6 >= tail)\n+\t\treturn 0;\n \tif (memcmp(buf, \"author\", 6))\n \t\treturn 0;\n-\twhile (*buf++ != '\\n')\n+\twhile (buf < tail && *buf++ != '\\n')\n \t\t/* nada */;\n+\tif (buf + 9 >= tail)\n+\t\treturn 0;\n \tif (memcmp(buf, \"committer\", 9))\n \t\treturn 0;\n-\twhile (*buf++ != '>')\n+\twhile (buf < tail && *buf++ != '>')\n \t\t/* nada */;\n-\tdate = strtoul(buf, NULL, 10);\n+\tif (buf >= tail)\n+\t\treturn 0;\n+\tdateptr = buf;\n+\twhile (buf < tail && *buf++ != '\\n')\n+\t\t/* nada */;\n+\tif (buf >= tail)\n+\t\treturn 0;\n+\t/* dateptr < buf && buf[-1] == '\\n', so strtoul will stop at buf-1 */\n+\tdate = strtoul(dateptr, NULL, 10);\n \tif (date == ULONG_MAX)\n \t\tdate = 0;\n \treturn date;\n@@ -236,9 +249,9 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t\treturn 0;\n \titem->object.parsed = 1;\n \ttail += size;\n-\tif (tail <= bufptr + 5 || memcmp(bufptr, \"tree \", 5))\n+\tif (tail <= bufptr + 46 || memcmp(bufptr, \"tree \", 5) || bufptr[45] != '\\n')\n \t\treturn error(\"bogus commit object %s\", sha1_to_hex(item->object.sha1));\n-\tif (tail <= bufptr + 45 || get_sha1_hex(bufptr + 5, parent) < 0)\n+\tif (get_sha1_hex(bufptr + 5, parent) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t     sha1_to_hex(item->object.sha1));\n \titem->tree = lookup_tree(parent);\n@@ -275,7 +288,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t\t\tn_refs++;\n \t\t}\n \t}\n-\titem->date = parse_commit_date(bufptr);\n+\titem->date = parse_commit_date(bufptr, tail);\n \n \tif (track_object_refs) {\n \t\tunsigned i = 0;\n-- \ngitgui.0.9.1.g39b9\n"},{"id":"65986","messageId":"7vprvx8ueo.fsf@gitster.siamese.dyndns.org","threadId":"11678","inReplyTo":"1200764123283-git-send-email-mkoegler@auto.tuwien.ac.at","subject":"Re: [PATCH] parse_commit_buffer: don't parse invalid commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-19T19:52:47Z","receivedAt":"2008-01-19T19:52:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:\n\n> * check, that the tree line ends with \\n\n\nOk.\n\n> * prevent parse_commit_date from reading beyond the buffer,\n>   - if author line does not end with \\n\n>   - if committer line does not contain >\n\n\"even if\" would be nicer, I think.\n\n> * verify in parse_commit_date, that the commiter line ends with \\n\n\nPerhaps an additional note to leave for people who will later be\ntempted to touch this part of the code is needed; otherwise the\ntime spent on the thread goes wasted.\n\n * keep the existing behaviour to return a commit object even\n   when non-structural fields such as committer and author are\n   malformed, so that tools that need to look at commits to\n   clean up a history with such broken commits can still get at\n   the structural data (i.e. the parents chain and the tree\n   object).\n\nNo need to resend if you allow me to amend the patch along the\nabove lines (please just say so), or you could resend with\nimprovements on your own.\n\nThanks.\n"},{"id":"66039","messageId":"20080120161105.GA10830@auto.tuwien.ac.at","threadId":"11678","inReplyTo":"7vprvx8ueo.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] parse_commit_buffer: don't parse invalid commits","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-01-20T16:11:05Z","receivedAt":"2008-01-20T16:11:05Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Sat, Jan 19, 2008 at 11:52:47AM -0800, Junio C Hamano wrote:\n> No need to resend if you allow me to amend the patch along the\n> above lines (please just say so), or you could resend with\n> improvements on your own.\n\nFeel free to change it.\n\nmfg Martin Kögler\n"}]}