{"thread":{"id":"15004","subject":"[PATCH] Allow emails with boundaries to work again [v2]","startedAt":"2008-08-14T15:35:42Z","lastAt":"2008-08-18T05:42:02Z","messageCount":2,"participants":["Don Zickus","Kevin Ballard"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"87198","messageId":"1218728142-23780-1-git-send-email-dzickus@redhat.com","threadId":"15004","inReplyTo":null,"subject":"[PATCH] Allow emails with boundaries to work again [v2]","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2008-08-14T15:35:42Z","receivedAt":"2008-08-14T15:35:42Z","isPatch":true,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"Recent changes to is_multipart_boundary() caused git-mailinfo to segfault.\nThe reason was after handling the end of the boundary the code tried to look\nfor another boundary.  Because the boundary list was empty, dereferencing\nthe pointer to the top of the boundary caused the program to go boom.\n\nThe fix is to check to see if the list is empty and if so go on its merry\nway instead of looking for another boundary.\n\nI also fixed a couple of increments and decrements that didn't look correct\nrelating to content_top.\n\nThe boundary test case was updated to catch future problems like this again.\n\nSigned-Off-by: Don Zickus <dzickus@redhat.com>\n\n---\nJunio,\n\nI modified the patch more to your liking, I think.  You inquired about\nreturning after failing from find_boundary() and I gave you a not so correct\nanswer.  Once I re-read the code this morning (minus a screaming kid in the\nbackground), I realized it was easy to do and more correct.\n\nI also figured out why the boundary test case didn't catch this.  Apparently\nfind_boundary() went to read another line and found none, so it returned 0.\nAdding an empty line to sample.mbox, caused git-mailinfo to segfault like\nthe way I saw it.  My fix allows everything to work again.\n\nCheers,\nDon\n---\n builtin-mailinfo.c  |    6 +++---\n t/t5100/sample.mbox |    1 +\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\nindex 6ae2bf3..207d0ef 100644\n--- a/builtin-mailinfo.c\n+++ b/builtin-mailinfo.c\n@@ -175,7 +175,7 @@ static void handle_content_type(struct strbuf *line)\n \t\t message_type = TYPE_OTHER;\n \tif (slurp_attr(line->buf, \"boundary=\", boundary)) {\n \t\tstrbuf_insert(boundary, 0, \"--\", 2);\n-\t\tif (content_top++ >= &content[MAX_BOUNDARIES]) {\n+\t\tif (++content_top > &content[MAX_BOUNDARIES]) {\n \t\t\tfprintf(stderr, \"Too many boundaries to handle\\n\");\n \t\t\texit(1);\n \t\t}\n@@ -603,7 +603,7 @@ static void handle_filter(struct strbuf *line);\n static int find_boundary(void)\n {\n \twhile (!strbuf_getline(&line, fin, '\\n')) {\n-\t\tif (is_multipart_boundary(&line))\n+\t\tif (*content_top && is_multipart_boundary(&line))\n \t\t\treturn 1;\n \t}\n \treturn 0;\n@@ -626,7 +626,7 @@ again:\n \t\t/* technically won't happen as is_multipart_boundary()\n \t\t   will fail first.  But just in case..\n \t\t */\n-\t\tif (content_top-- < content) {\n+\t\tif (--content_top < content) {\n \t\t\tfprintf(stderr, \"Detected mismatched boundaries, \"\n \t\t\t\t\t\"can't recover\\n\");\n \t\t\texit(1);\ndiff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\nindex d7ca79b..4bf7947 100644\n--- a/t/t5100/sample.mbox\n+++ b/t/t5100/sample.mbox\n@@ -500,3 +500,4 @@ index 3e5fe51..aabfe5c 100644\n 1.6.0.rc2\n \n --=-=-=--\n+\n-- \n1.5.5.1\n"},{"id":"87513","messageId":"87332D4C-A77E-4D36-91B5-045D94C6B668@sb.org","threadId":"15004","inReplyTo":"1218728142-23780-1-git-send-email-dzickus@redhat.com","subject":"Re: [PATCH] Allow emails with boundaries to work again [v2]","fromName":"Kevin Ballard","fromEmail":"kevin@sb.org","sentAt":"2008-08-18T05:42:02Z","receivedAt":"2008-08-18T05:42:02Z","isPatch":true,"sender":{"key":"kevin@sb.org","avatar":"https://avatars.githubusercontent.com/u/714?v=4"},"body":"Is there a reason this isn't in next yet? It definitely solves a crash  \nI ran into when trying to pipe a mail containing an attached patch to  \ngit-am.\n\n-Kevin Ballard\n\nOn Aug 14, 2008, at 8:35 AM, Don Zickus wrote:\n\n> Recent changes to is_multipart_boundary() caused git-mailinfo to  \n> segfault.\n> The reason was after handling the end of the boundary the code tried  \n> to look\n> for another boundary.  Because the boundary list was empty,  \n> dereferencing\n> the pointer to the top of the boundary caused the program to go boom.\n>\n> The fix is to check to see if the list is empty and if so go on its  \n> merry\n> way instead of looking for another boundary.\n>\n> I also fixed a couple of increments and decrements that didn't look  \n> correct\n> relating to content_top.\n>\n> The boundary test case was updated to catch future problems like  \n> this again.\n>\n> Signed-Off-by: Don Zickus <dzickus@redhat.com>\n>\n> ---\n> Junio,\n>\n> I modified the patch more to your liking, I think.  You inquired about\n> returning after failing from find_boundary() and I gave you a not so  \n> correct\n> answer.  Once I re-read the code this morning (minus a screaming kid  \n> in the\n> background), I realized it was easy to do and more correct.\n>\n> I also figured out why the boundary test case didn't catch this.   \n> Apparently\n> find_boundary() went to read another line and found none, so it  \n> returned 0.\n> Adding an empty line to sample.mbox, caused git-mailinfo to segfault  \n> like\n> the way I saw it.  My fix allows everything to work again.\n>\n> Cheers,\n> Don\n> ---\n> builtin-mailinfo.c  |    6 +++---\n> t/t5100/sample.mbox |    1 +\n> 2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c\n> index 6ae2bf3..207d0ef 100644\n> --- a/builtin-mailinfo.c\n> +++ b/builtin-mailinfo.c\n> @@ -175,7 +175,7 @@ static void handle_content_type(struct strbuf  \n> *line)\n> \t\t message_type = TYPE_OTHER;\n> \tif (slurp_attr(line->buf, \"boundary=\", boundary)) {\n> \t\tstrbuf_insert(boundary, 0, \"--\", 2);\n> -\t\tif (content_top++ >= &content[MAX_BOUNDARIES]) {\n> +\t\tif (++content_top > &content[MAX_BOUNDARIES]) {\n> \t\t\tfprintf(stderr, \"Too many boundaries to handle\\n\");\n> \t\t\texit(1);\n> \t\t}\n> @@ -603,7 +603,7 @@ static void handle_filter(struct strbuf *line);\n> static int find_boundary(void)\n> {\n> \twhile (!strbuf_getline(&line, fin, '\\n')) {\n> -\t\tif (is_multipart_boundary(&line))\n> +\t\tif (*content_top && is_multipart_boundary(&line))\n> \t\t\treturn 1;\n> \t}\n> \treturn 0;\n> @@ -626,7 +626,7 @@ again:\n> \t\t/* technically won't happen as is_multipart_boundary()\n> \t\t   will fail first.  But just in case..\n> \t\t */\n> -\t\tif (content_top-- < content) {\n> +\t\tif (--content_top < content) {\n> \t\t\tfprintf(stderr, \"Detected mismatched boundaries, \"\n> \t\t\t\t\t\"can't recover\\n\");\n> \t\t\texit(1);\n> diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox\n> index d7ca79b..4bf7947 100644\n> --- a/t/t5100/sample.mbox\n> +++ b/t/t5100/sample.mbox\n> @@ -500,3 +500,4 @@ index 3e5fe51..aabfe5c 100644\n> 1.6.0.rc2\n>\n> --=-=-=--\n> +\n> -- \n> 1.5.5.1\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\n-- \nKevin Ballard\nhttp://kevin.sb.org\nkevin@sb.org\nhttp://www.tildesoft.com\n"}]}