{"thread":{"id":"36916","subject":"[PATCH] http: fix charset detection of extract_content_type()","startedAt":"2014-06-14T18:49:34Z","lastAt":"2014-06-17T09:31:07Z","messageCount":5,"participants":["nori","Yi EungJun","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"244197","messageId":"539caff7.e7bc420a.76b9.fffff853@mx.google.com","threadId":"36916","inReplyTo":null,"subject":"[PATCH] http: fix charset detection of extract_content_type()","fromName":"nori","fromEmail":"semtlenori@gmail.com","sentAt":"2014-06-14T18:49:34Z","receivedAt":"2014-06-14T18:49:34Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"extract_content_type() could not extract a charset parameter if the\nparameter is not the first one and there is a whitespace and a following\nsemicolon just before the parameter. For example:\n\n text/plain; format=fixed ;charset=utf-8\n\nSigned-off-by: Yi EungJun \n---\n http.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 2b4f6a3..05e8b91 100644\n--- a/http.c\n+++ b/http.c\n@@ -971,7 +971,7 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n \n \tstrbuf_reset(charset);\n \twhile (*p) {\n-\t\twhile (isspace(*p))\n+\t\twhile (isspace(*p) || *p == ';')\n \t\t\tp++;\n \t\tif (!extract_param(p, \"charset\", charset))\n \t\t\treturn;\n-- \n2.0.0.422.gb6302de\n"},{"id":"244208","messageId":"539d5770.23de440a.5332.ffffe521@mx.google.com","threadId":"36916","inReplyTo":"539caff7.e7bc420a.76b9.fffff853@mx.google.com","subject":"Re: [PATCH] http: fix charset detection of extract_content_type()","fromName":"Yi EungJun","fromEmail":"semtlenori@gmail.com","sentAt":"2014-06-15T08:21:00Z","receivedAt":"2014-06-15T08:21:00Z","isPatch":true,"sender":{"key":"semtlenori@gmail.com","avatar":"https://gravatar.com/avatar/8363435d2badb3450df0dd7c4ec2113f6e1d62c44dd3cf7dfe83c5a389b8a9bf?d=mp&s=160"},"body":"Could you change the author to \"Yi EungJun \"\nif you apply this patch?\n"},{"id":"244328","messageId":"xmqqha3kr9m4.fsf@gitster.dls.corp.google.com","threadId":"36916","inReplyTo":"539caff7.e7bc420a.76b9.fffff853@mx.google.com","subject":"Re: [PATCH] http: fix charset detection of extract_content_type()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-16T18:29:39Z","receivedAt":"2014-06-16T18:29:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"nori writes:\n\n> extract_content_type() could not extract a charset parameter if the\n> parameter is not the first one and there is a whitespace and a following\n> semicolon just before the parameter. For example:\n>\n> text/plain; format=fixed ;charset=utf-8\n>\n> Signed-off-by: Yi EungJun \n> ---\n\nPeff, doesn't this look somehow familiar? Perhaps with e3131626\n(http: optionally extract charset parameter from content-type,\n2014-05-22), this patch is unnecessary?\n\n> http.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/http.c b/http.c\n> index 2b4f6a3..05e8b91 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -971,7 +971,7 @@ static void extract_content_type(struct strbuf *raw, struct strbuf *type,\n> \n> \tstrbuf_reset(charset);\n> \twhile (*p) {\n> -\t\twhile (isspace(*p))\n> +\t\twhile (isspace(*p) || *p == ';')\n> \t\t\tp++;\n> \t\tif (!extract_param(p, \"charset\", charset))\n> \t\t\treturn;\n"},{"id":"244337","messageId":"20140616192005.GA19322@sigill.intra.peff.net","threadId":"36916","inReplyTo":"xmqqha3kr9m4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] http: fix charset detection of extract_content_type()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-16T19:20:05Z","receivedAt":"2014-06-16T19:20:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 16, 2014 at 11:29:39AM -0700, Junio C Hamano wrote:\n\n> nori writes:\n> \n> > extract_content_type() could not extract a charset parameter if the\n> > parameter is not the first one and there is a whitespace and a following\n> > semicolon just before the parameter. For example:\n> >\n> > text/plain; format=fixed ;charset=utf-8\n> >\n> > Signed-off-by: Yi EungJun \n> > ---\n> \n> Peff, doesn't this look somehow familiar? Perhaps with e3131626\n> (http: optionally extract charset parameter from content-type,\n> 2014-05-22), this patch is unnecessary?\n\nNo, I think this is built on top of e3131626 to catcha case I mised. It\nlooks OK at first glance, but I'd really like to add better test\ncoverage for this parsing.\n\n-Peff\n"},{"id":"244389","messageId":"20140617093106.GA5922@sigill.intra.peff.net","threadId":"36916","inReplyTo":"539caff7.e7bc420a.76b9.fffff853@mx.google.com","subject":"Re: [PATCH] http: fix charset detection of extract_content_type()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-17T09:31:07Z","receivedAt":"2014-06-17T09:31:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 15, 2014 at 03:49:34AM +0900, nori wrote:\n\n> extract_content_type() could not extract a charset parameter if the\n> parameter is not the first one and there is a whitespace and a following\n> semicolon just before the parameter. For example:\n> \n> text/plain; format=fixed ;charset=utf-8\n\nThanks, I think your patch does the right thing. We also have a similar\nsituation going the other way. If we have:\n\n text/plain; charset=utf-8; format=fixed\n\nwe will parse the charset as \"utf-8;\". My version of iconv actually\nseems to accept that, but we should probably be more careful.\n\nI think parameter values can actually be fully quoted. So you could\nhave something as nasty as:\n\n text/plain; some-param=\"a long value with ;semicolons;\"; charset=utf-8\n\nI'd rather not get into parsing that level, as I don't expect it to\nhappen in the real world. And besides, we actually _would_ find the\ncharset here with the current code; the only thing we might do wrong is\nto parse:\n\n text/plain; tricky=\"param; charset=foo\"; charset=bar\n\nwith charset \"foo\" rather than \"bar\". But that's highly unlikely, and\nthe stakes are fairly low (we just show the error message in the wrong\ncharset).\n\nAnyway, when you re-roll with the correct \"From:\" header, do you mind\nsquashing in the extra change and the tests below?\n\ndiff --git a/http.c b/http.c\nindex 05e8b91..3a28b21 100644\n--- a/http.c\n+++ b/http.c\n@@ -927,7 +927,7 @@ static int extract_param(const char *raw, const char *name,\n \t\treturn -1;\n \traw++;\n \n-\twhile (*raw && !isspace(*raw))\n+\twhile (*raw && !isspace(*raw) && *raw != ';')\n \t\tstrbuf_addch(out, *raw++);\n \treturn 0;\n }\ndiff --git a/t/lib-httpd/error.sh b/t/lib-httpd/error.sh\nindex eafc9d2..a77b8e5 100755\n--- a/t/lib-httpd/error.sh\n+++ b/t/lib-httpd/error.sh\n@@ -19,6 +19,10 @@ case \"$PATH_INFO\" in\n \tprintf \"text/plain; charset=utf-16\"\n \tcharset=utf-16\n \t;;\n+*odd-spacing*)\n+\tprintf \"text/plain; foo=bar ;charset=utf-16; other=nonsense\"\n+\tcharset=utf-16\n+\t;;\n esac\n printf \"\\n\"\n \ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 01b8aae..ac71418 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -191,5 +191,10 @@ test_expect_success 'http error messages are reencoded' '\n \tgrep \"this is the error message\" stderr\n '\n \n+test_expect_success 'reencoding is robust to whitespace oddities' '\n+\ttest_must_fail git clone \"$HTTPD_URL/error/odd-spacing\" 2>stderr &&\n+\tgrep \"this is the error message\" stderr\n+'\n+\n stop_httpd\n test_done\n"}]}