{"thread":{"id":"49293","subject":"Re: CONTENT_LENGTH can no longer be empty","startedAt":"2018-09-06T06:10:42Z","lastAt":"2018-09-12T16:10:59Z","messageCount":39,"participants":["Jonathan Nieder","Max Kirillov","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"357514","messageId":"20180906061038.GA94045@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180905202613.GA20473@blodeuwedd","subject":"Re: CONTENT_LENGTH can no longer be empty","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-06T06:10:38Z","receivedAt":"2018-09-06T06:10:42Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJelmer Vernooij wrote[1]:\n\n> Git's http-backend has become slightly stricter about the content\n> of the CONTENT_LENGTH variable. Previously, Dulwich would leave this\n> variable empty but git now expects it to be set to 0 for GET requests\n> without a body.\n>\n> I'm uploading a fixed version of dulwich.\n\nThanks for tracking it down! This is likely due to v2.19.0-rc0~45^2~2\n(http-backend: respect CONTENT_LENGTH as specified by rfc3875,\n2018-06-10).\n\nMax, RFC 3875 appears to allow a CONTENT_LENGTH of \"\" when no data is\nattached to the request. Should we check for this case (e.g. inserting\na *str check in\n\n\tif (str && !git_parse_ssize_t(str, &val))\n\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n\n?\n\nThanks,\nJonathan\n\n[1] https://bugs.debian.org/907587\n"},{"id":"357570","messageId":"20180906193516.28909-1-max@max630.net","threadId":"49293","inReplyTo":"20180906061038.GA94045@aiede.svl.corp.google.com","subject":"[PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-06T19:35:16Z","receivedAt":"2018-09-06T19:42:48Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"According to RFC3875, empty environment variable is equivalent to unset,\nand for CONTENT_LENGTH it should mean zero body to read.\n\nHowever, as discussed in [1], unset CONTENT_LENGTH is also used for\nchunked encoding to indicate reading until EOF, so keep this behavior also\nfor empty CONTENT_LENGTH.\n\nAdd a test for the case.\n\n[1] https://public-inbox.org/git/20160329201349.GB9527@sigill.intra.peff.net/\n\nSigned-off-by: Max Kirillov \n---\nHi.\n\nThis should fix it. I'm not sure should it treat it as 0 or \"-1\"\nAt least the tests mentioned by Jeff fails if I try to treat missing CONTENT_LENGTH as \"-1\"\nSo keep the existing behavior as much as possible\n http-backend.c | 2 +-\n t/t5562-http-backend-content-length.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e88d29f62b..a1230d7ead 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -353,7 +353,7 @@ static ssize_t get_content_length(void)\n \tssize_t val = -1;\n \tconst char *str = getenv(\"CONTENT_LENGTH\");\n \n-\tif (str && !git_parse_ssize_t(str, &val))\n+\tif (str && *str && !git_parse_ssize_t(str, &val))\n \t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n \treturn val;\n }\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 057dcb85d6..ca34c2f054 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n \tgrep \"fatal:.*CONTENT_LENGTH\" err\n '\n \n+test_expect_success 'empty CONTENT_LENGTH' '\n+\tenv \\\n+\t\tQUERY_STRING=/repo.git/HEAD \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/HEAD \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=GET \\\n+\t\tCONTENT_LENGTH=\"\" \\\n+\t\tgit http-backend act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n test_done\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"357587","messageId":"xmqq1sa6z3zp.fsf@gitster-ct.c.googlers.com","threadId":"49293","inReplyTo":"20180906193516.28909-1-max@max630.net","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-06T21:54:18Z","receivedAt":"2018-09-06T21:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> According to RFC3875, empty environment variable is equivalent to unset,\n> and for CONTENT_LENGTH it should mean zero body to read.\n>\n> However, as discussed in [1], unset CONTENT_LENGTH is also used for\n> chunked encoding to indicate reading until EOF, so keep this behavior also\n> for empty CONTENT_LENGTH.\n\nMakes sense.\n\n>\n> Add a test for the case.\n>\n> [1] https://public-inbox.org/git/20160329201349.GB9527@sigill.intra.peff.net/\n>\n> Signed-off-by: Max Kirillov \n> ---\n> Hi.\n>\n> This should fix it. I'm not sure should it treat it as 0 or \"-1\"\n> At least the tests mentioned by Jeff fails if I try to treat missing CONTENT_LENGTH as \"-1\"\n> So keep the existing behavior as much as possible\n\nI am not sure what you mean by the above, between 0 and -1. The\ncode signals the caller of get_content_length() that req_len is -1\nwhich is used as a sign to read through to the EOF, so it appears to\nme that the code treats missing content-length (i.e. str == NULL\ncase) as \"-1\".\n\n\n> http-backend.c | 2 +-\n> t/t5562-http-backend-content-length.sh | 11 +++++++++++\n> 2 files changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/http-backend.c b/http-backend.c\n> index e88d29f62b..a1230d7ead 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -353,7 +353,7 @@ static ssize_t get_content_length(void)\n> \tssize_t val = -1;\n> \tconst char *str = getenv(\"CONTENT_LENGTH\");\n> \n> -\tif (str && !git_parse_ssize_t(str, &val))\n> +\tif (str && *str && !git_parse_ssize_t(str, &val))\n> \t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n> \treturn val;\n> }\n> diff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\n> index 057dcb85d6..ca34c2f054 100755\n> --- a/t/t5562-http-backend-content-length.sh\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n> \tgrep \"fatal:.*CONTENT_LENGTH\" err\n> '\n> \n> +test_expect_success 'empty CONTENT_LENGTH' '\n> +\tenv \\\n> +\t\tQUERY_STRING=/repo.git/HEAD \\\n> +\t\tPATH_TRANSLATED=\"$PWD\"/.git/HEAD \\\n> +\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n> +\t\tREQUEST_METHOD=GET \\\n> +\t\tCONTENT_LENGTH=\"\" \\\n> +\t\tgit http-backend act.out 2>act.err &&\n> +\tverify_http_result \"200 OK\"\n> +'\n> +\n> test_done\n"},{"id":"357590","messageId":"20180906224505.GA81412@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180906193516.28909-1-max@max630.net","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-06T22:45:05Z","receivedAt":"2018-09-06T22:45:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMax Kirillov wrote:\n\n> According to RFC3875, empty environment variable is equivalent to unset,\n> and for CONTENT_LENGTH it should mean zero body to read.\n>\n> However, as discussed in [1], unset CONTENT_LENGTH is also used for\n> chunked encoding to indicate reading until EOF, so keep this behavior also\n> for empty CONTENT_LENGTH.\n>\n> Add a test for the case.\n>\n> [1] https://public-inbox.org/git/20160329201349.GB9527@sigill.intra.peff.net/\n>\n> Signed-off-by: Max Kirillov \n\nReported-by: Jelmer Vernooij \n\nThanks for fixing it.\n\nCan you include a summary of [1] instead of relying on the mailing\nlist archive? Perhaps just omiting \"as discussed in [1]\" would do the\ntrick. Alternatively, if there's a point from that discussion that's\nrelevant to the change, please include it here. That way, people\nfinding this change later can save some time by avoiding having to dig\nthrough that mailing list thread.\n\nFor example, it's probably worth mentioning that this was discovered\nusing dulwich's test suite.\n\n[...]\n> This should fix it. I'm not sure should it treat it as 0 or \"-1\"\n> At least the tests mentioned by Jeff fails if I try to treat missing CONTENT_LENGTH as \"-1\"\n> So keep the existing behavior as much as possible\n\nThat sounds worth figuring out so we can understand and possibly\ndocument it better. What are the ramifications of this choice ---\nwhat would work / not work with each choice?\n\n[...]\n> --- a/t/t5562-http-backend-content-length.sh\n> +++ b/t/t5562-http-backend-content-length.sh\n> @@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n\nYay, thanks for this as well.\n\nSincerely,\nJonathan\n"},{"id":"357600","messageId":"20180907032740.GA20545@jessie.local","threadId":"49293","inReplyTo":"xmqq1sa6z3zp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-07T03:27:40Z","receivedAt":"2018-09-07T03:27:50Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Thu, Sep 06, 2018 at 02:54:18PM -0700, Junio C Hamano wrote:\n> Max Kirillov writes:\n>> This should fix it. I'm not sure should it treat it as 0 or \"-1\"\n>> At least the tests mentioned by Jeff fails if I try to treat missing CONTENT_LENGTH as \"-1\"\n>> So keep the existing behavior as much as possible\n> \n> I am not sure what you mean by the above, between 0 and -1. The\n> code signals the caller of get_content_length() that req_len is -1\n> which is used as a sign to read through to the EOF, so it appears to\n> me that the code treats missing content-length (i.e. str == NULL\n> case) as \"-1\".\n\nI made a mistake in this, it should be \"if I try to treat missing\nCONTENT_LENGTH as 0\". This, as far as I understand, what the\nRFC specifies.\n\nThat is, after the following change, the test \"large fetch-pack\nrequests can be split across POSTs\" from t5551 starts faliing:\n\n-- >8 --\n@@ -353,8 +353,12 @@ static ssize_t get_content_length(void)\n ssize_t val = -1;\n const char *str = getenv(\"CONTENT_LENGTH\");\n \n- if (str && *str && !git_parse_ssize_t(str, &val))\n- die(\"failed to parse CONTENT_LENGTH: %s\", str);\n+ if (str && *str) {\n+ if (!git_parse_ssize_t(str, &val))\n+ die(\"failed to parse CONTENT_LENGTH: %s\", str);\n+ } else\n+ val = 0;\n+\n return val;\n }\n"},{"id":"357602","messageId":"20180907033607.24604-1-max@max630.net","threadId":"49293","inReplyTo":"20180906193516.28909-1-max@max630.net","subject":"[PATCH v2] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-07T03:36:07Z","receivedAt":"2018-09-07T03:36:18Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"According to RFC3875, empty environment variable is equivalent to unset,\nand for CONTENT_LENGTH it should mean zero body to read.\n\nHowever, unset CONTENT_LENGTH is also used for chunked encoding to indicate\nreading until EOF. At least, the test \"large fetch-pack requests can be split\nacross POSTs\" from t5551 starts faliing, if unset or empty CONTENT_LENGTH is\ntreated as zero length body. So keep the existing behavior as much as possible.\n\nAdd a test for the case.\n\nReported-By: Jelmer Vernooij \nSigned-off-by: Max Kirillov \n---\nAdded the \"reported-by\" and explained inline the reason to keep existing behavior\n http-backend.c | 2 +-\n t/t5562-http-backend-content-length.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e88d29f62b..a1230d7ead 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -353,7 +353,7 @@ static ssize_t get_content_length(void)\n \tssize_t val = -1;\n \tconst char *str = getenv(\"CONTENT_LENGTH\");\n \n-\tif (str && !git_parse_ssize_t(str, &val))\n+\tif (str && *str && !git_parse_ssize_t(str, &val))\n \t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n \treturn val;\n }\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 057dcb85d6..ca34c2f054 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n \tgrep \"fatal:.*CONTENT_LENGTH\" err\n '\n \n+test_expect_success 'empty CONTENT_LENGTH' '\n+\tenv \\\n+\t\tQUERY_STRING=/repo.git/HEAD \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/HEAD \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=GET \\\n+\t\tCONTENT_LENGTH=\"\" \\\n+\t\tgit http-backend act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n test_done\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"357603","messageId":"20180907033831.GB1383@sigill.intra.peff.net","threadId":"49293","inReplyTo":"20180907032740.GA20545@jessie.local","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-07T03:38:31Z","receivedAt":"2018-09-07T03:38:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 07, 2018 at 06:27:40AM +0300, Max Kirillov wrote:\n\n> On Thu, Sep 06, 2018 at 02:54:18PM -0700, Junio C Hamano wrote:\n> > Max Kirillov writes:\n> >> This should fix it. I'm not sure should it treat it as 0 or \"-1\"\n> >> At least the tests mentioned by Jeff fails if I try to treat missing CONTENT_LENGTH as \"-1\"\n> >> So keep the existing behavior as much as possible\n> > \n> > I am not sure what you mean by the above, between 0 and -1. The\n> > code signals the caller of get_content_length() that req_len is -1\n> > which is used as a sign to read through to the EOF, so it appears to\n> > me that the code treats missing content-length (i.e. str == NULL\n> > case) as \"-1\".\n> \n> I made a mistake in this, it should be \"if I try to treat missing\n> CONTENT_LENGTH as 0\". This, as far as I understand, what the\n> RFC specifies.\n> \n> That is, after the following change, the test \"large fetch-pack\n> requests can be split across POSTs\" from t5551 starts faliing:\n> \n> -- >8 --\n> @@ -353,8 +353,12 @@ static ssize_t get_content_length(void)\n> ssize_t val = -1;\n> const char *str = getenv(\"CONTENT_LENGTH\");\n> \n> - if (str && *str && !git_parse_ssize_t(str, &val))\n> - die(\"failed to parse CONTENT_LENGTH: %s\", str);\n> + if (str && *str) {\n> + if (!git_parse_ssize_t(str, &val))\n> + die(\"failed to parse CONTENT_LENGTH: %s\", str);\n> + } else\n> + val = 0;\n> +\n\nRight, I'm pretty sure it is a problem if you treat a missing\nCONTENT_LENGTH as \"present, but zero\". Because chunked encodings from\napache really do want us to read until EOF.\n\nMy understanding from Jelmer's report is that a present-but-empty\nvariable should be counted as \"0\" to mean \"do not read any body bytes\".\nThat matches my reading of RFC 3875, which says:\n\n If no data is attached, then NULL (or unset).\n\n(and earlier they explicitly define NULL as the empty string). That\nsaid, we do not do what they say for the \"unset\" case. And cannot\nwithout breaking chunked encoding from apache. So I don't know how much\nwe want to follow that rfc to the letter, but at least it makes sense to\nme to revert this case back to what Git used to do, and what the rfc\nsays.\n\nIn other words, I think the logic we want is:\n\n if (!str) {\n\t/*\n\t * RFC3875 says this must mean \"no body\", but in practice we\n\t * receive chunked encodings with no CONTENT_LENGTH. Tell the\n\t * caller to read until EOF.\n\t */\n\tval = -1;\n } else if (!*str) {\n\t/*\n\t * An empty length should be treated as \"no body\" according to\n\t * RFC3875, and this seems to hold in practice.\n\t */\n\tval = 0;\n } else {\n\t/*\n\t * We have a CONTENT_LENGTH; trust what's in it as long as it\n\t * can be parsed.\n\t */\n\tif (!git_parse_ssize_t(str, &val))\n\t die(...);\n }\n\n-Peff\n"},{"id":"357606","messageId":"20180907042039.GB20545@jessie.local","threadId":"49293","inReplyTo":"20180907033831.GB1383@sigill.intra.peff.net","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-07T04:20:39Z","receivedAt":"2018-09-07T04:20:44Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Thu, Sep 06, 2018 at 11:38:31PM -0400, Jeff King wrote:\n> My understanding from Jelmer's report is that a present-but-empty\n> variable should be counted as \"0\" to mean \"do not read any body bytes\".\n> That matches my reading of RFC 3875, which says:\n> \n> If no data is attached, then NULL (or unset).\n> \n> (and earlier they explicitly define NULL as the empty string). That\n> said, we do not do what they say for the \"unset\" case. And cannot\n> without breaking chunked encoding from apache. So I don't know how much\n> we want to follow that rfc to the letter, but at least it makes sense to\n> me to revert this case back to what Git used to do, and what the rfc\n> says.\n\nI could find this discussion about it:\nhttps://lists.gt.net/apache/users/373042\n\nBasically, it says the CGI RFC was written before chunked\nencoding appeared, so implementations should choose between\ncaching all boody before calling script, or breaking the\nspec some way. So apache does it so.\n\n(I wonder how IIS would handle it)\n\n> In other words, I think the logic we want is:\n> \n> if (!str) {\n> \t/*\n> \t * RFC3875 says this must mean \"no body\", but in practice we\n> \t * receive chunked encodings with no CONTENT_LENGTH. Tell the\n> \t * caller to read until EOF.\n> \t */\n> \tval = -1;\n> } else if (!*str) {\n> \t/*\n> \t * An empty length should be treated as \"no body\" according to\n> \t * RFC3875, and this seems to hold in practice.\n> \t */\n> \tval = 0;\n> } else {\n> \t/*\n> \t * We have a CONTENT_LENGTH; trust what's in it as long as it\n> \t * can be parsed.\n> \t */\n> \tif (!git_parse_ssize_t(str, &val))\n> \t die(...);\n> }\n\nI feel reluctant to treat empty and unset differently, but\nprobably this is the only thing which could be done.\n\nI'll resumbmit some time later.\n"},{"id":"357607","messageId":"CAF7_NFRg8wOQ0JbjkJ2gpxKs+oh3s8qXVSPfsWSth2tiUK39hw@mail.gmail.com","threadId":"49293","inReplyTo":"20180907033831.GB1383@sigill.intra.peff.net","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-07T04:59:26Z","receivedAt":"2018-09-07T04:59:30Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Actually, another reason for the latest issue was that CONTENT_LENGTH\nis parsed for GET requests at all. It should be parsed only for POST\nrequests, or, rather, only for upoad-pack and receive-pack requests.\n"},{"id":"357615","messageId":"xmqqsh2ly6vw.fsf@gitster-ct.c.googlers.com","threadId":"49293","inReplyTo":"CAF7_NFRg8wOQ0JbjkJ2gpxKs+oh3s8qXVSPfsWSth2tiUK39hw@mail.gmail.com","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-07T09:49:23Z","receivedAt":"2018-09-07T09:49:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n> Actually, another reason for the latest issue was that CONTENT_LENGTH\n> is parsed for GET requests at all. It should be parsed only for POST\n> requests, or, rather, only for upoad-pack and receive-pack requests.\n\nNot really. The layered design of the HTTP protocol means that any\nrequest type can have non-empty body, but request types for which\nno semantics of the body is defined must ignore what is in the body,\nwhich in turn means we need to parse and pay attention to the\ncontent length etc. to find the end of the body, if only to ignore\nit.\n\nIn any case, hopefully we can fix this before the final, as this is\na regression introduced during this cycle?\n"},{"id":"357672","messageId":"20180908001940.GB225427@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180907033607.24604-1-max@max630.net","subject":"Re: [PATCH v2] http-backend: allow empty CONTENT_LENGTH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-08T00:19:40Z","receivedAt":"2018-09-08T00:19:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMax Kirillov wrote:\n\n> According to RFC3875, empty environment variable is equivalent to unset,\n> and for CONTENT_LENGTH it should mean zero body to read.\n>\n> However, unset CONTENT_LENGTH is also used for chunked encoding to indicate\n> reading until EOF. At least, the test \"large fetch-pack requests can be split\n> across POSTs\" from t5551 starts faliing, if unset or empty CONTENT_LENGTH is\n> treated as zero length body. So keep the existing behavior as much as possible.\n>\n> Add a test for the case.\n>\n> Reported-By: Jelmer Vernooij \n> Signed-off-by: Max Kirillov \n> ---\n> Added the \"reported-by\" and explained inline the reason to keep existing behavior\n\nLovely, thanks.\n\nTo me, \"keep the existing behavior as much as possible\" isn't comforting\nbecause it doesn't tell me *which* existing behavior. Fortunately the patch\nitself is comforting: it makes us treat \"\" the same way as unset, which is\nexactly what the RFC requires.\n\nSo I'm happy with this version. Thanks for your patient work.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"357676","messageId":"20180908054105.GC20545@jessie.local","threadId":"49293","inReplyTo":"xmqqsh2ly6vw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-08T05:41:05Z","receivedAt":"2018-09-08T05:41:14Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Fri, Sep 07, 2018 at 02:49:23AM -0700, Junio C Hamano wrote:\n> Max Kirillov writes:\n> \n>> Actually, another reason for the latest issue was that CONTENT_LENGTH\n>> is parsed for GET requests at all. It should be parsed only for POST\n>> requests, or, rather, only for upoad-pack and receive-pack requests.\n> \n> Not really. The layered design of the HTTP protocol means that any\n> request type can have non-empty body, but request types for which\n> no semantics of the body is defined must ignore what is in the body,\n> which in turn means we need to parse and pay attention to the\n> content length etc. to find the end of the body, if only to ignore\n> it.\n\nI don't think it is git's job to police web server implementations,\nespecially considering that there is a gap between letter of RFC and\nactual behavior. Anyway, it only runs the check for \"*/info/refs\" GET\nrequest, which ends up in get_info_refs(). Other GET requests do not\ncheck CONTENT_LENGTH. Also, the version of service which is started from\nget_info_refs() do not consume input (I think, actually, the\n\"--stateless-rpc\" argument is not needed there).\n\n> In any case, hopefully we can fix this before the final, as this is\n> a regression introduced during this cycle?\n\nYes, I'm working on it.\n"},{"id":"357677","messageId":"20180908054224.21856-1-max@max630.net","threadId":"49293","inReplyTo":"20180908001940.GB225427@aiede.svl.corp.google.com","subject":"[PATCH v3] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-08T05:42:24Z","receivedAt":"2018-09-08T05:42:33Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Before 817f7dc223, CONTENT_LENGTH variable was never considered,\nhttp-backend was just reading request body from standard input until EOF\nwhen it, or a command started by it, needed it.\n\nThen it was discovered that some HTTP do not close standard input, instead\nexpecting CGI scripts to obey CONTENT_LENGTH. In 817f7dc223, behavior\nwas changed to consider the CONTENT_LENGTH variable when it is set. Case\nof unset CONTENT_LENGTH was kept to mean \"read until EOF\" which is not\ncompliant to the RFC3875 (which treats it as empty body), but\npractically is used when client uses chunked encoding to submit big\nrequest.\n\nCase of empty CONTENT_LENGTH has slept through this conditions.\nApparently, it is used for GET requests, and RFC3875 does specify that\nit also means empty body. Current implementation, however, fails to\nparse it and aborts the request.\n\nFix the case of empty CONTENT_LENGTH to also be treated as \"read until EOF\".\nIt does not actually matter what does it mean because body is never read\nanyway, it just should not cause parse error. Add a test for the case.\n\nReported-By: Jelmer Vernooij \nSigned-off-by: Max Kirillov \n---\nProvided more thorough message, also fix test (it did not test actually the error before)\n\nThere will be more versions later, at least the one which suggested by Jeff\n\nPS: did I write v2, it should be v3 of course!\n http-backend.c | 2 +-\n t/t5562-http-backend-content-length.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e88d29f62b..a1230d7ead 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -353,7 +353,7 @@ static ssize_t get_content_length(void)\n \tssize_t val = -1;\n \tconst char *str = getenv(\"CONTENT_LENGTH\");\n \n-\tif (str && !git_parse_ssize_t(str, &val))\n+\tif (str && *str && !git_parse_ssize_t(str, &val))\n \t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n \treturn val;\n }\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 057dcb85d6..b28c3c4765 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n \tgrep \"fatal:.*CONTENT_LENGTH\" err\n '\n \n+test_expect_success 'empty CONTENT_LENGTH' '\n+\tenv \\\n+\t\tQUERY_STRING=\"/repo.git/info/refs?service=git-receive-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=GET \\\n+\t\tCONTENT_LENGTH=\"\" \\\n+\t\tgit http-backend act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n test_done\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"357678","messageId":"20180908053521.21218-1-max@max630.net","threadId":"49293","inReplyTo":"20180908001940.GB225427@aiede.svl.corp.google.com","subject":"[PATCH v2] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-08T05:35:21Z","receivedAt":"2018-09-08T05:43:22Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Before 817f7dc223, CONTENT_LENGTH variable was never considered,\nhttp-backend was just reading request body from standard input until EOF\nwhen it, or a command started by it, needed it.\n\nThen it was discovered that some HTTP do not close standard input, instead\nexpecting CGI scripts to obey CONTENT_LENGTH. In 817f7dc223, behavior\nwas changed to consider the CONTENT_LENGTH variable when it is set. Case\nof unset CONTENT_LENGTH was kept to mean \"read until EOF\" which is not\ncompliant to the RFC3875 (which treats it as empty body), but\npractically is used when client uses chunked encoding to submit big\nrequest.\n\nCase of empty CONTENT_LENGTH has slept through this conditions.\nApparently, it is used for GET requests, and RFC3875 does specify that\nit also means empty body. Current implementation, however, fails to\nparse it and aborts the request.\n\nFix the case of empty CONTENT_LENGTH to also be treated as \"read until EOF\".\nIt does not actually matter what does it mean because body is never read\nanyway, it just should not cause parse error. Add a test for the case.\n\nReported-By: Jelmer Vernooij \nSigned-off-by: Max Kirillov \n---\nProvided more thorough message, also fix test (it did not test actually the error before)\n\nThere will be more versions later, at least the one which suggested by Jeff\n http-backend.c | 2 +-\n t/t5562-http-backend-content-length.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e88d29f62b..a1230d7ead 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -353,7 +353,7 @@ static ssize_t get_content_length(void)\n \tssize_t val = -1;\n \tconst char *str = getenv(\"CONTENT_LENGTH\");\n \n-\tif (str && !git_parse_ssize_t(str, &val))\n+\tif (str && *str && !git_parse_ssize_t(str, &val))\n \t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n \treturn val;\n }\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 057dcb85d6..b28c3c4765 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n \tgrep \"fatal:.*CONTENT_LENGTH\" err\n '\n \n+test_expect_success 'empty CONTENT_LENGTH' '\n+\tenv \\\n+\t\tQUERY_STRING=\"/repo.git/info/refs?service=git-receive-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=GET \\\n+\t\tCONTENT_LENGTH=\"\" \\\n+\t\tgit http-backend act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n test_done\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"357729","messageId":"20180909041016.23980-1-max@max630.net","threadId":"49293","inReplyTo":"20180907033607.24604-1-max@max630.net","subject":"[PATCH v4] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-09T04:10:16Z","receivedAt":"2018-09-09T04:12:21Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"Before 817f7dc223, CONTENT_LENGTH variable was never considered,\nhttp-backend was just reading request body from standard input until EOF\nwhen it, or a command started by it, needed it.\n\nThen it was discovered that some HTTP do not close standard input, instead\nexpecting CGI scripts to obey CONTENT_LENGTH. In 817f7dc223, behavior\nwas changed to consider the CONTENT_LENGTH variable when it is set. Case\nof unset CONTENT_LENGTH was kept to mean \"read until EOF\" which is not\ncompliant to the RFC3875 (which treats it as empty body), but\npractically is used when client uses chunked encoding to submit big\nrequest.\n\nCase of empty CONTENT_LENGTH has slept through this conditions.\nApparently, it is used for GET requests, and RFC3875 does specify that\nit also means empty body. Current implementation, however, fails to\nparse it and aborts the request.\n\nFix the case of empty CONTENT_LENGTH to be treated as zero-length body\nis expected, as specified by RFC3875. It does not actually matter what\ndoes it mean because body is never read anyway, it just should not cause\nparse error. Add a test for the case.\n\nReported-By: Jelmer Vernooij \nAuthored-by: Jeff King \nSigned-off-by: Max Kirillov \n---\nThe fix suggested by Jeff. I supposed there should be \"signed-off\"\nThe tests pass as well\n http-backend.c | 24 ++++++++++++++++++++++--\n t/t5562-http-backend-content-length.sh | 11 +++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex e88d29f62b..949821b46f 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -353,8 +353,28 @@ static ssize_t get_content_length(void)\n \tssize_t val = -1;\n \tconst char *str = getenv(\"CONTENT_LENGTH\");\n \n-\tif (str && !git_parse_ssize_t(str, &val))\n-\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n+\tif (!str) {\n+\t\t/*\n+\t\t * RFC3875 says this must mean \"no body\", but in practice we\n+\t\t * receive chunked encodings with no CONTENT_LENGTH. Tell the\n+\t\t * caller to read until EOF.\n+\t\t */\n+\t\tval = -1;\n+\t} else if (!*str) {\n+\t\t/*\n+\t\t * An empty length should be treated as \"no body\" according to\n+\t\t * RFC3875, and this seems to hold in practice.\n+\t\t */\n+\t\tval = 0;\n+\t} else {\n+\t\t/*\n+\t\t * We have a non-empty CONTENT_LENGTH; trust what's in it as long\n+\t\t * as it can be parsed.\n+\t\t */\n+\t\tif (!git_parse_ssize_t(str, &val))\n+\t\t\tdie(\"failed to parse CONTENT_LENGTH: '%s'\", str);\n+\t}\n+\n \treturn val;\n }\n \ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex 057dcb85d6..b28c3c4765 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -152,4 +152,15 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n \tgrep \"fatal:.*CONTENT_LENGTH\" err\n '\n \n+test_expect_success 'empty CONTENT_LENGTH' '\n+\tenv \\\n+\t\tQUERY_STRING=\"/repo.git/info/refs?service=git-receive-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n+\t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n+\t\tREQUEST_METHOD=GET \\\n+\t\tCONTENT_LENGTH=\"\" \\\n+\t\tgit http-backend act.out 2>act.err &&\n+\tverify_http_result \"200 OK\"\n+'\n+\n test_done\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"357730","messageId":"20180909044050.GD20545@jessie.local","threadId":"49293","inReplyTo":"xmqqsh2ly6vw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-09T04:40:50Z","receivedAt":"2018-09-09T04:40:56Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Fri, Sep 07, 2018 at 02:49:23AM -0700, Junio C Hamano wrote:\n> In any case, hopefully we can fix this before the final, as this is\n> a regression introduced during this cycle?\n\nI think I am going to stop at the v4. Unless there are some\ncorrections requested.\n"},{"id":"357775","messageId":"20180910051748.GA55941@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180908054224.21856-1-max@max630.net","subject":"Re: [PATCH v3] http-backend: allow empty CONTENT_LENGTH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-10T05:17:48Z","receivedAt":"2018-09-10T05:17:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Max Kirillov \nSubject: http-backend test: make empty CONTENT_LENGTH test more realistic\n\nThis is a test of smart HTTP, so it should use the smart HTTP endpoints\n(e.g. /info/refs?service=git-receive-pack), not dumb HTTP (HEAD).\n\nSigned-off-by: Max Kirillov \nSigned-off-by: Jonathan Nieder \n---\nMax Kirillov wrote:\n\n> Provided more thorough message, also fix test (it did not test\n> actually the error before)\n>\n> There will be more versions later, at least the one which suggested\n> by Jeff\n\nv2 is in \"next\", and I believe that version should already be\nsufficient for Git 2.19. Please correct me if I'm wrong.\n\nSince v2 is in \"next\", I think any further refinements are supposed to\nbe incremental patches on top. Here's an example (representing the\nv2->v3 diff). It's more of an RFC than a serious patch, because:\n\nThis version of the test doesn't seem to reproduce the bug. When I\nrun the test against the unfixed version of http-backend, it passes.\nIdeas?\n\nNot about this patch: could this test share some infrustructure with\nt5560-http-backend-noserver.sh? If there were some common shell\nlibrary that they shared, the tests might be easier to read and write.\n\nThanks,\nJonathan\n\n t/t5562-http-backend-content-length.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex f94d01f69e..fceb3d39c1 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -155,8 +155,8 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n \n test_expect_success 'empty CONTENT_LENGTH' '\n \tenv \\\n-\t\tQUERY_STRING=/repo.git/HEAD \\\n-\t\tPATH_TRANSLATED=\"$PWD\"/.git/HEAD \\\n+\t\tQUERY_STRING=\"/repo.git/info/refs?service=git-receive-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n \t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n \t\tREQUEST_METHOD=GET \\\n \t\tCONTENT_LENGTH=\"\" \\\n-- \n2.19.0.rc2.392.g5ba43deb5a\n\n"},{"id":"357777","messageId":"20180910052558.GB55941@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180909041016.23980-1-max@max630.net","subject":"Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-10T05:25:58Z","receivedAt":"2018-09-10T05:26:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Max Kirillov wrote:\n\n> Reported-By: Jelmer Vernooij \n> Authored-by: Jeff King \n> Signed-off-by: Max Kirillov \n\nNit: for this kind of case of forwarding someone else's patch, we put\na From field at the beginning of the body of the message. \"git\nformat-patch\" can produce a message with that format if you commit\nwith 'git commit --author=\"Someone Else \"' and run\nformat-patch with --from=\"My Name \". More details are\nin the DISCUSSION section of git-format-patch(1).\n\nAs with v3, since v2 is already in \"next\" this should go incremental.\n\n[...]\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -353,8 +353,28 @@ static ssize_t get_content_length(void)\n> \tssize_t val = -1;\n> \tconst char *str = getenv(\"CONTENT_LENGTH\");\n> \n> -\tif (str && !git_parse_ssize_t(str, &val))\n> -\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n> +\tif (!str) {\n> +\t\t/*\n> +\t\t * RFC3875 says this must mean \"no body\", but in practice we\n> +\t\t * receive chunked encodings with no CONTENT_LENGTH. Tell the\n> +\t\t * caller to read until EOF.\n> +\t\t */\n> +\t\tval = -1;\n> +\t} else if (!*str) {\n> +\t\t/*\n> +\t\t * An empty length should be treated as \"no body\" according to\n> +\t\t * RFC3875, and this seems to hold in practice.\n> +\t\t */\n> +\t\tval = 0;\n\nAre there example callers that this version fixes? Where can I read\nmore, or what can I run to experience it?\n\nFor example, v2.19.0-rc0~45^2~2 (http-backend: respect CONTENT_LENGTH\nas specified by rfc3875, 2018-06-10) mentions IIS/Windows; does IIS\nmake use of this distinction?\n\nThanks,\nJonathan\n"},{"id":"357780","messageId":"20180910131724.GA5233@sigill.intra.peff.net","threadId":"49293","inReplyTo":"20180910052558.GB55941@aiede.svl.corp.google.com","subject":"Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-10T13:17:25Z","receivedAt":"2018-09-10T13:17:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 09, 2018 at 10:25:58PM -0700, Jonathan Nieder wrote:\n\n> > --- a/http-backend.c\n> > +++ b/http-backend.c\n> > @@ -353,8 +353,28 @@ static ssize_t get_content_length(void)\n> > \tssize_t val = -1;\n> > \tconst char *str = getenv(\"CONTENT_LENGTH\");\n> > \n> > -\tif (str && !git_parse_ssize_t(str, &val))\n> > -\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n> > +\tif (!str) {\n> > +\t\t/*\n> > +\t\t * RFC3875 says this must mean \"no body\", but in practice we\n> > +\t\t * receive chunked encodings with no CONTENT_LENGTH. Tell the\n> > +\t\t * caller to read until EOF.\n> > +\t\t */\n> > +\t\tval = -1;\n> > +\t} else if (!*str) {\n> > +\t\t/*\n> > +\t\t * An empty length should be treated as \"no body\" according to\n> > +\t\t * RFC3875, and this seems to hold in practice.\n> > +\t\t */\n> > +\t\tval = 0;\n> \n> Are there example callers that this version fixes? Where can I read\n> more, or what can I run to experience it?\n> \n> For example, v2.19.0-rc0~45^2~2 (http-backend: respect CONTENT_LENGTH\n> as specified by rfc3875, 2018-06-10) mentions IIS/Windows; does IIS\n> make use of this distinction?\n\nSo this code is what I recommended based on my reading of the RFC, and\nbased on my understanding of the Debian bug. But I admit I'm confused.\n\nI thought the complaint was that this:\n\n CONTENT_LENGTH= git http-backend\n\nwas reading a body, when it shouldn't be. And so setting it to 0 here\nmade sense.\n\nBut that couldn't have been what older versions were doing, since they\nnever looked at CONTENT_LENGTH at all, and instead always read to EOF.\nSo presumably the original problem wasn't that we tried to read a body,\nbut that the empty string caused git_parse_ssize_t to report failure,\nand we called die(). Which probably should be explained by 574c513e8d\n(http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too\nlate for that.\n\nSo after that patch, we really do have the original behavior, and that's\nenough for v2.19.\n\nBut the remaining question then is: what should clients expect on an\nempty variable? We know what the RFC says, and we know what dulwich\nexpected, but I'm not sure we have real world cases beyond that. So it\nmight actually make sense to punt until we see one, though I don't mind\ndoing what the rfc says in the meantime. And then the explanation in the\ncommit message would be \"do what the rfc says\", and any test probably\nought to be feeding a non-empty empty and confirming that we don't read\nit.\n\n-Peff\n"},{"id":"357793","messageId":"xmqqa7opux4v.fsf@gitster-ct.c.googlers.com","threadId":"49293","inReplyTo":"20180910131724.GA5233@sigill.intra.peff.net","subject":"Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-10T16:37:20Z","receivedAt":"2018-09-10T16:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But that couldn't have been what older versions were doing, since they\n> never looked at CONTENT_LENGTH at all, and instead always read to EOF.\n> So presumably the original problem wasn't that we tried to read a body,\n> but that the empty string caused git_parse_ssize_t to report failure,\n> and we called die(). Which probably should be explained by 574c513e8d\n> (http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too\n> late for that.\n>\n> So after that patch, we really do have the original behavior, and that's\n> enough for v2.19.\n\nTo recap to make sure I am following it correctly:\n\n - pay attention to content-length when it is clearly given with a\n byte count, which is an improvement over v2.18\n\n - mimick what we have been doing until now when content-length is\n missing or set to an empty string, so we are regression free and\n bug-to-bug compatible relative to v2.18 in these two cases.\n\n> But the remaining question then is: what should clients expect on an\n> empty variable? We know what the RFC says, and we know what dulwich\n> expected, but I'm not sure we have real world cases beyond that. So it\n> might actually make sense to punt until we see one, though I don't mind\n> doing what the rfc says in the meantime. And then the explanation in the\n> commit message would be \"do what the rfc says\", and any test probably\n> ought to be feeding a non-empty empty and confirming that we don't read\n> it.\n\nThe RFC is pretty clear that no data is signaled by \"NULL (or\nunset)\", meaning an empty string value and missing variable both\nmean the same \"no message body\", but it further says that the\nservers MUST set CONTENT_LENGTH if and only if there is a\nmessage-body, which contradicts with itself (if you adhered to 'if\nand only if', in no case you would set it to NULL).\n\nGoogling \"cgi chunked encoding\" seems to give us tons of hits to\nshow that people are puzzled, just like us, that the scripts would\nnot get to see Chunked (as the server is supposed to deChunk to\ncount content-length before calling the backend). So I agree \"do\nwhat the rfc says\" is a good thing to try early in the next cycle.\n"},{"id":"357813","messageId":"20180910184619.GA20678@sigill.intra.peff.net","threadId":"49293","inReplyTo":"xmqqa7opux4v.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-10T18:46:19Z","receivedAt":"2018-09-10T18:46:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 09:37:20AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But that couldn't have been what older versions were doing, since they\n> > never looked at CONTENT_LENGTH at all, and instead always read to EOF.\n> > So presumably the original problem wasn't that we tried to read a body,\n> > but that the empty string caused git_parse_ssize_t to report failure,\n> > and we called die(). Which probably should be explained by 574c513e8d\n> > (http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too\n> > late for that.\n> >\n> > So after that patch, we really do have the original behavior, and that's\n> > enough for v2.19.\n> \n> To recap to make sure I am following it correctly:\n> \n> - pay attention to content-length when it is clearly given with a\n> byte count, which is an improvement over v2.18\n> \n> - mimick what we have been doing until now when content-length is\n> missing or set to an empty string, so we are regression free and\n> bug-to-bug compatible relative to v2.18 in these two cases.\n\nThat (and what you wrote below) matches my current understanding, too.\nThough I did already admit to being confused. ;)\n\n-Peff\n"},{"id":"357829","messageId":"20180910203628.GF20545@jessie.local","threadId":"49293","inReplyTo":"20180910051748.GA55941@aiede.svl.corp.google.com","subject":"Re: [PATCH v3] http-backend: allow empty CONTENT_LENGTH","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-10T20:36:28Z","receivedAt":"2018-09-10T20:36:34Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Sun, Sep 09, 2018 at 10:17:48PM -0700, Jonathan Nieder wrote:\n> From: Max Kirillov \n> Subject: http-backend test: make empty CONTENT_LENGTH test more realistic\n\nThank you, yes, this is what should have left\n"},{"id":"357830","messageId":"20180910205359.32332-1-max@max630.net","threadId":"49293","inReplyTo":"20180910052558.GB55941@aiede.svl.corp.google.com","subject":"[PATCH] http-backend: Treat empty CONTENT_LENGTH as zero","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-10T20:53:59Z","receivedAt":"2018-09-10T20:54:18Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"From: Jeff King \nSubject: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero\n\nThere is no known case where empty body it used by a server as\ninstruction to read until EOF, so there is no need to violate the RFC.\nMake get_content_length() return 0 in this case.\n\nCurrently there is no practical difference, as the GET request\nwhere it can be empty is handled without actual reading the body\n(in get_info_refs() function), but it is better to stick to the correct\nbehavior.\n\nSigned-off-by: Max Kirillov \n---\nThe incremental. Hopefully I described the reason right. Needs \"signed-off-by\"\n http-backend.c | 24 ++++++++++++++++++++++--\n 1 file changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 458642ef72..ea36a52118 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -353,8 +353,28 @@ static ssize_t get_content_length(void)\n \tssize_t val = -1;\n \tconst char *str = getenv(\"CONTENT_LENGTH\");\n \n-\tif (str && *str && !git_parse_ssize_t(str, &val))\n-\t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n+\tif (!str) {\n+\t\t/*\n+\t\t * RFC3875 says this must mean \"no body\", but in practice we\n+\t\t * receive chunked encodings with no CONTENT_LENGTH. Tell the\n+\t\t * caller to read until EOF.\n+\t\t */\n+\t\tval = -1;\n+\t} else if (!*str) {\n+\t\t/*\n+\t\t * An empty length should be treated as \"no body\" according to\n+\t\t * RFC3875, and this seems to hold in practice.\n+\t\t */\n+\t\tval = 0;\n+\t} else {\n+\t\t/*\n+\t\t * We have a non-empty CONTENT_LENGTH; trust what's in it as long\n+\t\t * as it can be parsed.\n+\t\t */\n+\t\tif (!git_parse_ssize_t(str, &val))\n+\t\t\tdie(\"failed to parse CONTENT_LENGTH: '%s'\", str);\n+\t}\n+\n \treturn val;\n }\n \n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"357832","messageId":"20180910212221.GG26356@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180910205359.32332-1-max@max630.net","subject":"Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-10T21:22:21Z","receivedAt":"2018-09-10T21:22:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMax Kirillov wrote:\n\n> From: Jeff King \n> Subject: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero\n\nmicronit: s/Treat/treat/\n\n> There is no known case where empty body it used by a server as\n> instruction to read until EOF, so there is no need to violate the RFC.\n> Make get_content_length() return 0 in this case.\n>\n> Currently there is no practical difference, as the GET request\n> where it can be empty is handled without actual reading the body\n> (in get_info_refs() function), but it is better to stick to the correct\n> behavior.\n>\n> Signed-off-by: Max Kirillov \n> ---\n> The incremental. Hopefully I described the reason right. Needs \"signed-off-by\"\n\nThanks. I am wondering if we should go all the way and do\n\n\tssize_t val;\n\tconst char *str = getenv(\"CONTENT_LENGTH\");\n\n\tif (!str || !*str)\n\t\treturn 0;\n\tif (!git_parse_ssize_t(str, &val))\n\t\tdie(...);\n\treturn val;\n\nThat would match the RFC, but it seems to make t5510-fetch.sh hang,\nright after\n\n ok 165 - --negotiation-tip understands abbreviated SHA-1\n\nWhen I run with -v -i -x, it stalls at\n\n ++ git -C '/usr/local/google/home/jrn/src/git/t/trash directory.t5510-fetch/httpd/www/server' tag -d alpha_1 alpha_2 beta_1 beta_2\n Deleted tag 'alpha_1' (was a84e4a9)\n Deleted tag 'alpha_2' (was 7dd5cf4)\n Deleted tag 'beta_1' (was bcb5c65)\n Deleted tag 'beta_2' (was d3b6dcd)\n +++ pwd\n ++ GIT_TRACE_PACKET='/usr/local/google/home/jrn/src/git/t/trash directory.t5510-fetch/trace'\n ++ git -C client fetch --negotiation-tip=alpha_1 --negotiation-tip=beta_1 origin alpha_s beta_s\n\nDo you know why?\n\nThanks,\nJonathan\n"},{"id":"357843","messageId":"20180911015553.GA5838@sigill.intra.peff.net","threadId":"49293","inReplyTo":"20180910212221.GG26356@aiede.svl.corp.google.com","subject":"Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-11T01:55:53Z","receivedAt":"2018-09-11T01:55:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 02:22:21PM -0700, Jonathan Nieder wrote:\n\n> Thanks. I am wondering if we should go all the way and do\n> \n> \tssize_t val;\n> \tconst char *str = getenv(\"CONTENT_LENGTH\");\n> \n> \tif (!str || !*str)\n> \t\treturn 0;\n> \tif (!git_parse_ssize_t(str, &val))\n> \t\tdie(...);\n> \treturn val;\n> \n> That would match the RFC, but it seems to make t5510-fetch.sh hang,\n> right after\n> \n> ok 165 - --negotiation-tip understands abbreviated SHA-1\n> \n> When I run with -v -i -x, it stalls at\n> \n> ++ git -C '/usr/local/google/home/jrn/src/git/t/trash directory.t5510-fetch/httpd/www/server' tag -d alpha_1 alpha_2 beta_1 beta_2\n> Deleted tag 'alpha_1' (was a84e4a9)\n> Deleted tag 'alpha_2' (was 7dd5cf4)\n> Deleted tag 'beta_1' (was bcb5c65)\n> Deleted tag 'beta_2' (was d3b6dcd)\n> +++ pwd\n> ++ GIT_TRACE_PACKET='/usr/local/google/home/jrn/src/git/t/trash directory.t5510-fetch/trace'\n> ++ git -C client fetch --negotiation-tip=alpha_1 --negotiation-tip=beta_1 origin alpha_s beta_s\n> \n> Do you know why?\n\nYes. :)\n\nIt's due to this comment in the patch you are replying to:\n\n+ if (!str) {\n+ /*\n+ * RFC3875 says this must mean \"no body\", but in practice we\n+ * receive chunked encodings with no CONTENT_LENGTH. Tell the\n+ * caller to read until EOF.\n+ */\n+ val = -1;\n\n-Peff\n"},{"id":"357844","messageId":"20180911015800.GB5838@sigill.intra.peff.net","threadId":"49293","inReplyTo":"20180910205359.32332-1-max@max630.net","subject":"Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-11T01:58:00Z","receivedAt":"2018-09-11T01:58:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 11:53:59PM +0300, Max Kirillov wrote:\n\n> From: Jeff King \n> Subject: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero\n> \n> There is no known case where empty body it used by a server as\n> instruction to read until EOF, so there is no need to violate the RFC.\n> Make get_content_length() return 0 in this case.\n> \n> Currently there is no practical difference, as the GET request\n> where it can be empty is handled without actual reading the body\n> (in get_info_refs() function), but it is better to stick to the correct\n> behavior.\n\nThere could be a difference if there is a server which actually sets\nCONTENT_LENGTH to the empty string for a chunked body. But we don't know\nof any such server at this point.\n\n> Signed-off-by: Max Kirillov \n> ---\n> The incremental. Hopefully I described the reason right. Needs \"signed-off-by\"\n\nCertainly this is:\n\n Signed-off-by: Jeff King \n\n-Peff\n"},{"id":"357845","messageId":"20180911022028.GA20518@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180911015553.GA5838@sigill.intra.peff.net","subject":"Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-11T02:20:28Z","receivedAt":"2018-09-11T02:20:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Mon, Sep 10, 2018 at 02:22:21PM -0700, Jonathan Nieder wrote:\n\n>> Thanks. I am wondering if we should go all the way and do\n>>\n>> \tssize_t val;\n>> \tconst char *str = getenv(\"CONTENT_LENGTH\");\n>>\n>> \tif (!str || !*str)\n>> \t\treturn 0;\n>> \tif (!git_parse_ssize_t(str, &val))\n>> \t\tdie(...);\n>> \treturn val;\n>>\n>> That would match the RFC, but it seems to make t5510-fetch.sh hang,\n[...]\n>> Do you know why?\n>\n> Yes. :)\n>\n> It's due to this comment in the patch you are replying to:\n>\n> + if (!str) {\n> + /*\n> + * RFC3875 says this must mean \"no body\", but in practice we\n> + * receive chunked encodings with no CONTENT_LENGTH. Tell the\n> + * caller to read until EOF.\n> + */\n> + val = -1;\n\nAh! So \"in practice\" includes \"in Apache\". An old discussion[1] on\nApache's httpd-users list agrees.\n\nThe question then becomes: what does IIS do for zero-length requests?\nDoes any other web server fail to support \"read until EOF\" in general?\n\nThe CGI standard does not cover chunked encoding so we can't lean on\nthe standard for advice. It's not clear to me yet whether this patch\nimproves on what's in \"master\".\n\nThanks,\nJonathan\n\n[1] http://mail-archives.apache.org/mod_mbox/httpd-users/200909.mbox/%3C4AAACC38.3070200@rowe-clan.net%3E\n"},{"id":"357846","messageId":"20180911023025.GA7739@sigill.intra.peff.net","threadId":"49293","inReplyTo":"20180911022028.GA20518@aiede.svl.corp.google.com","subject":"Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-11T02:30:25Z","receivedAt":"2018-09-11T02:30:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 10, 2018 at 07:20:28PM -0700, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> > On Mon, Sep 10, 2018 at 02:22:21PM -0700, Jonathan Nieder wrote:\n> \n> >> Thanks. I am wondering if we should go all the way and do\n> >>\n> >> \tssize_t val;\n> >> \tconst char *str = getenv(\"CONTENT_LENGTH\");\n> >>\n> >> \tif (!str || !*str)\n> >> \t\treturn 0;\n> >> \tif (!git_parse_ssize_t(str, &val))\n> >> \t\tdie(...);\n> >> \treturn val;\n> >>\n> >> That would match the RFC, but it seems to make t5510-fetch.sh hang,\n> [...]\n> >> Do you know why?\n> >\n> > Yes. :)\n> >\n> > It's due to this comment in the patch you are replying to:\n> >\n> > + if (!str) {\n> > + /*\n> > + * RFC3875 says this must mean \"no body\", but in practice we\n> > + * receive chunked encodings with no CONTENT_LENGTH. Tell the\n> > + * caller to read until EOF.\n> > + */\n> > + val = -1;\n> \n> Ah! So \"in practice\" includes \"in Apache\". An old discussion[1] on\n> Apache's httpd-users list agrees.\n> \n> The question then becomes: what does IIS do for zero-length requests?\n> Does any other web server fail to support \"read until EOF\" in general?\n> \n> The CGI standard does not cover chunked encoding so we can't lean on\n> the standard for advice. It's not clear to me yet whether this patch\n> improves on what's in \"master\".\n\nI'd note that the case in question (no CONTENT_LENGTH at all) is not\nchanged between this patch and master. It's only the case of\nCONTENT_LENGTH set to an empty string. But I agree that it is not clear\nto me whether it is actually improving anything in practice.\n\n-Peff\n"},{"id":"357848","messageId":"20180911034227.GB20518@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180910205359.32332-1-max@max630.net","subject":"[PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-11T03:42:27Z","receivedAt":"2018-09-11T03:42:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"As discussed in v2.19.0-rc0~45^2~2 (http-backend: respect\nCONTENT_LENGTH as specified by rfc3875, 2018-06-10), HTTP servers such\nas IIS do not close a CGI script's standard input at the end of a\nrequest, instead expecting CGI scripts to stop reading after\nCONTENT_LENGTH bytes. That commit taught http-backend to respect this\nconvention except when CONTENT_LENGTH is unset, in which case it\npreserved the previous behavior of reading until EOF.\n\nRFC 3875 (the CGI specification) explains:\n\n The CONTENT_LENGTH variable contains the size of the message-body\n attached to the request, if any, in decimal number of octets. If no\n data is attached, then NULL (or unset).\n\n CONTENT_LENGTH = \"\" | 1*digit\n\nAnd:\n\n This specification does not distinguish between zero-length (NULL)\n values and missing values.\n\nBut that specification was written before HTTP/1.1 and chunked\nencoding. With chunked encoding, the length of a request is not known\nearly and it is useful to start a CGI script to process it anyway, so\nApache and many other servers violate the spec: they leave\nCONTENT_LENGTH unset and rely on EOF to indicate the end of request.\nThis is reproducible using t5510-fetch.sh, which hangs if http-backend\nis patched to treat a missing CONTENT_LENGTH as zero.\n\nSo we are in a bind: to support HTTP servers that don't produce EOF,\nhttp-backend should respect an unset or empty CONTENT_LENGTH that\nrepresents zero, and to support chunked encoding, http-backend should\nrespect an unset CONTENT_LENGTH that represents \"read until EOF\".\n\nFortunately, there's a way out. Use the HTTP_TRANSFER_ENCODING\nenvironment variable to distinguish the two cases.\n\nReported-by: Jeff King \nHelped-by: Max Kirillov \nSigned-off-by: Jonathan Nieder \n---\nHow about this?\n\n http-backend.c | 19 +++++++++++++++++--\n 1 file changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 458642ef72..7902eeb0b3 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o\n \n static ssize_t get_content_length(void)\n {\n-\tssize_t val = -1;\n+\tssize_t val;\n \tconst char *str = getenv(\"CONTENT_LENGTH\");\n \n-\tif (str && *str && !git_parse_ssize_t(str, &val))\n+\tif (!str || !*str) {\n+\t\t/*\n+\t\t * According to RFC 3875, an empty or missing\n+\t\t * CONTENT_LENGTH means \"no body\", but RFC 3875\n+\t\t * precedes HTTP/1.1 and chunked encoding. Apache and\n+\t\t * its imitators leave CONTENT_LENGTH unset for\n+\t\t * chunked requests, for which we should use EOF to\n+\t\t * detect the end of the request.\n+\t\t */\n+\t\tstr = getenv(\"HTTP_TRANSFER_ENCODING\");\n+\t\tif (str && !strcmp(str, \"chunked\"))\n+\t\t\treturn -1;\n+\n+\t\treturn 0;\n+\t}\n+\tif (!git_parse_ssize_t(str, &val))\n \t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n \treturn val;\n }\n-- \n2.19.0.397.gdd90340f6a\n\n"},{"id":"357849","messageId":"20180911040343.GC20518@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180911034227.GB20518@aiede.svl.corp.google.com","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-11T04:03:43Z","receivedAt":"2018-09-11T04:04:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Kicking off the reviews: ;-)\n\nJonathan Nieder wrote:\n\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o\n> \n> static ssize_t get_content_length(void)\n[...]\n> +\t\t/*\n> +\t\t * According to RFC 3875, an empty or missing\n> +\t\t * CONTENT_LENGTH means \"no body\", but RFC 3875\n> +\t\t * precedes HTTP/1.1 and chunked encoding. Apache and\n> +\t\t * its imitators leave CONTENT_LENGTH unset for\n\nWhich imitators? Maybe this should just say \"Apache leaves [...]\".\n\n> +\t\t * chunked requests, for which we should use EOF to\n> +\t\t * detect the end of the request.\n> +\t\t */\n> +\t\tstr = getenv(\"HTTP_TRANSFER_ENCODING\");\n> +\t\tif (str && !strcmp(str, \"chunked\"))\n\nRFC 2616 says Transfer-Encoding is a list of transfer-codings applied,\nin the order that they were applied, and that \"chunked\" is always\napplied last. That means a transfer-encoding like\n\n\tTransfer-Encoding: identity chunked\n\nwould be permitted, or e.g.\n\n\tTransfer-Encoding: gzip chunked\n\nDoes that means we should be using a check like\n\n\tstr && (!strcmp(str, \"chunked\") || ends_with(str, \" chunked\"))\n\n?\n\nThat said, a quick search of codesearch.debian.net mostly finds\nexamples using straight comparison, so maybe the patch is fine as-is.\n\nThanks,\nJonathan\n"},{"id":"357850","messageId":"20180911040621.GD20518@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180910203628.GF20545@jessie.local","subject":"Re: [PATCH v3] http-backend: allow empty CONTENT_LENGTH","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-11T04:06:21Z","receivedAt":"2018-09-11T04:06:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Max Kirillov wrote:\n> On Sun, Sep 09, 2018 at 10:17:48PM -0700, Jonathan Nieder wrote:\n\n>> From: Max Kirillov \n>> Subject: http-backend test: make empty CONTENT_LENGTH test more realistic\n>\n> Thank you, yes, this is what should have left\n\nOh, tying up this loose end: do you know why the test passes without\n574c513e8d (http-backend: allow empty CONTENT_LENGTH, 2018-09-10)?\n"},{"id":"357851","messageId":"xmqqo9d4r7j2.fsf@gitster-ct.c.googlers.com","threadId":"49293","inReplyTo":"20180911034227.GB20518@aiede.svl.corp.google.com","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T04:18:41Z","receivedAt":"2018-09-11T04:18:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> RFC 3875 (the CGI specification) explains:\n>\n> The CONTENT_LENGTH variable contains the size of the message-body\n> attached to the request, if any, in decimal number of octets. If no\n> data is attached, then NULL (or unset).\n>\n> CONTENT_LENGTH = \"\" | 1*digit\n>\n> And:\n>\n> This specification does not distinguish between zero-length (NULL)\n> values and missing values.\n>\n> But that specification was written before HTTP/1.1 and chunked\n> encoding.\n\nRFC 3875 is from October 2004, while RFC 2616 (HTTP/1.1) is from\nJune 1999; I presume that 3875 only writes down what has already\nbeen an established practice and that is where the date discrepancy\ncomes from.\n\nThis is a bit old but some of those who participated in the\ndiscussion archived at https://lists.gt.net/apache/users/373042\nseemed to know what they were talking about. In short, lack of\ncontent-length for CGI scripts driven by Apache seems to mean that\nthe content length is unknown and we are expected to read through to\nthe eof, and we are expected to ignore the CGI spec, which says\nmissing CONTENT_LENGTH and CONTENT_LENGTH set to an empty string\nboth must mean there is no message body. Instead, we need to take\nthe former as a sign to read through to the end.\n\n> Fortunately, there's a way out. Use the HTTP_TRANSFER_ENCODING\n> environment variable to distinguish the two cases.\n\nCute.\n\nI'm anxious to learn how well this works in practice. Or is this a\ntrick you know somebody else's system already uses (in which case,\nthat's wonderful)?\n\n> +\tif (!str || !*str) {\n> +\t\t/*\n> +\t\t * According to RFC 3875, an empty or missing\n> +\t\t * CONTENT_LENGTH means \"no body\", but RFC 3875\n> +\t\t * precedes HTTP/1.1 and chunked encoding. Apache and\n> +\t\t * its imitators leave CONTENT_LENGTH unset for\n> +\t\t * chunked requests, for which we should use EOF to\n> +\t\t * detect the end of the request.\n> +\t\t */\n> +\t\tstr = getenv(\"HTTP_TRANSFER_ENCODING\");\n> +\t\tif (str && !strcmp(str, \"chunked\"))\n> +\t\t\treturn -1;\n> +\n> +\t\treturn 0;\n> +\t}\n> +\tif (!git_parse_ssize_t(str, &val))\n> \t\tdie(\"failed to parse CONTENT_LENGTH: %s\", str);\n> \treturn val;\n> }\n"},{"id":"357852","messageId":"20180911042919.GE20518@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"xmqqo9d4r7j2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-11T04:29:19Z","receivedAt":"2018-09-11T04:29:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> RFC 3875 (the CGI specification) explains:\n>>\n>> The CONTENT_LENGTH variable contains the size of the message-body\n>> attached to the request, if any, in decimal number of octets. If no\n>> data is attached, then NULL (or unset).\n[...]\n>> But that specification was written before HTTP/1.1 and chunked\n>> encoding.\n>\n> RFC 3875 is from October 2004, while RFC 2616 (HTTP/1.1) is from\n> June 1999; I presume that 3875 only writes down what has already\n> been an established practice and that is where the date discrepancy\n> comes from.\n\nYes, CGI 1.1 is from 1995. More details are at\nhttps://www.w3.org/CGI/.\n\n[...]\n>> Fortunately, there's a way out. Use the HTTP_TRANSFER_ENCODING\n>> environment variable to distinguish the two cases.\n>\n> Cute.\n>\n> I'm anxious to learn how well this works in practice. Or is this a\n> trick you know somebody else's system already uses (in which case,\n> that's wonderful)?\n\nAlas, I came up with it today so I don't know yet how well it will\nwork in practice.\n\nI can poke around a little tomorrow in Apache to sanity-check the\napproach. Results from anyone able to test using various HTTP servers\n(lighttpd, etc) would also be very welcome.\n\nThanks,\nJonathan\n"},{"id":"357885","messageId":"xmqqk1nrq4su.fsf@gitster-ct.c.googlers.com","threadId":"49293","inReplyTo":"20180911040343.GC20518@aiede.svl.corp.google.com","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T18:15:13Z","receivedAt":"2018-09-11T18:15:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Kicking off the reviews: ;-)\n>\n> Jonathan Nieder wrote:\n>\n>> --- a/http-backend.c\n>> +++ b/http-backend.c\n>> @@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t req_len, unsigned char **o\n>> \n>> static ssize_t get_content_length(void)\n> [...]\n>> +\t\t/*\n>> +\t\t * According to RFC 3875, an empty or missing\n>> +\t\t * CONTENT_LENGTH means \"no body\", but RFC 3875\n>> +\t\t * precedes HTTP/1.1 and chunked encoding. Apache and\n>> +\t\t * its imitators leave CONTENT_LENGTH unset for\n>\n> Which imitators? Maybe this should just say \"Apache leaves [...]\".\n\nI tend to agree; I do not mind amending the text while queuing.\n\n>> +\t\t * chunked requests, for which we should use EOF to\n>> +\t\t * detect the end of the request.\n>> +\t\t */\n>> +\t\tstr = getenv(\"HTTP_TRANSFER_ENCODING\");\n>> +\t\tif (str && !strcmp(str, \"chunked\"))\n>\n> RFC 2616 says Transfer-Encoding is a list of transfer-codings applied,\n> in the order that they were applied, and that \"chunked\" is always\n> applied last. That means a transfer-encoding like\n>\n> \tTransfer-Encoding: identity chunked\n>\n> would be permitted, or e.g.\n>\n> \tTransfer-Encoding: gzip chunked\n>\n> Does that means we should be using a check like\n>\n> \tstr && (!strcmp(str, \"chunked\") || ends_with(str, \" chunked\"))\n>\n> ?\n\nHmph, that's \n\n\t\"Transfer-Encoding\" \":\" 1#transfer-coding\n\nwhere #rule is\n\n #rule\n A construct \"#\" is defined, similar to \"*\", for defining lists of\n elements. The full form is \"#element\" indicating at least\n and at most elements, each separated by one or more commas\n (\",\") and OPTIONAL linear white space (LWS). This makes the usual\n form of lists very easy; a rule such as\n ( *LWS element *( *LWS \",\" *LWS element ))\n can be shown as\n 1#element\n\nSo\n\n - you need to account for comma\n - your LWS may not be a SP\n\nif you want to handle gzipped stream coming in a chunked form, I\nthink.\n\nUnless I am missing the rule in CGI spec that is used to transform\nthe value on the Transfer-Encoding header to HTTP_TRANSFER_ENCODING\nenvironment variable, that is.\n\n> That said, a quick search of codesearch.debian.net mostly finds\n> examples using straight comparison, so maybe the patch is fine as-is.\n\nI do not think we would mind terribly if we do not support\ncombinations like gzipped-and-then-chunked from day one. An in-code\nNEEDSWORK comment that refers to the production in RFC 2616 Page 143\nmay not hurt, though.\n\nThanks.\n"},{"id":"357887","messageId":"xmqqa7onq490.fsf@gitster-ct.c.googlers.com","threadId":"49293","inReplyTo":"xmqqk1nrq4su.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-11T18:27:07Z","receivedAt":"2018-09-11T18:27:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>> +\t\t/*\n>>> +\t\t * According to RFC 3875, an empty or missing\n>>> +\t\t * CONTENT_LENGTH means \"no body\", but RFC 3875\n>>> +\t\t * precedes HTTP/1.1 and chunked encoding. Apache and\n>>> +\t\t * its imitators leave CONTENT_LENGTH unset for\n>>\n>> Which imitators? Maybe this should just say \"Apache leaves [...]\".\n>\n> I tend to agree; I do not mind amending the text while queuing.\n> ...\n> I do not think we would mind terribly if we do not support\n> combinations like gzipped-and-then-chunked from day one. An in-code\n> NEEDSWORK comment that refers to the production in RFC 2616 Page 143\n> may not hurt, though.\n\nI refrained from reflowing the first paragraph of the comment in\nthis message, but will probably reflow it before committing, if the\nupdated text is acceptable.\n\n\n http-backend.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/http-backend.c b/http-backend.c\nindex 8f515a6def..b997eafb00 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -357,10 +357,17 @@ static ssize_t get_content_length(void)\n \t\t/*\n \t\t * According to RFC 3875, an empty or missing\n \t\t * CONTENT_LENGTH means \"no body\", but RFC 3875\n-\t\t * precedes HTTP/1.1 and chunked encoding. Apache and\n-\t\t * its imitators leave CONTENT_LENGTH unset for\n+\t\t * precedes HTTP/1.1 and chunked encoding. Apache\n+\t\t * leaves CONTENT_LENGTH unset for\n \t\t * chunked requests, for which we should use EOF to\n \t\t * detect the end of the request.\n+\t\t *\n+\t\t * NEEDSWORK: Transfer-Encoding header is defined to\n+\t\t * be a list of elements where \"chunked\", if exists,\n+\t\t * must be at the end. The current code only deals\n+\t\t * with the case where \"chunked\" is the only element.\n+\t\t * See RFC 2616 (14.41 Transfer-Encoding) when\n+\t\t * extending this code.\n \t\t */\n \t\tstr = getenv(\"HTTP_TRANSFER_ENCODING\");\n \t\tif (str && !strcmp(str, \"chunked\"))\n"},{"id":"357910","messageId":"20180911203336.4601-1-max@max630.net","threadId":"49293","inReplyTo":"20180911040621.GD20518@aiede.svl.corp.google.com","subject":"[PATCH v2] http-backend test: make empty CONTENT_LENGTH test more realistic","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2018-09-11T20:33:36Z","receivedAt":"2018-09-11T20:33:49Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"This is a test of smart HTTP, so it should use the smart HTTP endpoints\n(e.g. /info/refs?service=git-receive-pack), not dumb HTTP (HEAD).\n\nSigned-off-by: Jonathan Nieder \nSigned-off-by: Max Kirillov \n---\n> do you know why the test passes without 574c513e8d (http-backend: allow empty CONTENT_LENGTH, 2018-09-10)?\n\nBecause I did not know what is QUERY_STRING (it is the part after \"?\"). Fixed now\n(Somehow I did see the failure during development. Maybe there was another parameter before?)\n\nI suspect it is not OK in other places of the test, but I hope it should not affect the result\n t/t5562-http-backend-content-length.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5562-http-backend-content-length.sh b/t/t5562-http-backend-content-length.sh\nindex f94d01f69e..b24d8b05a4 100755\n--- a/t/t5562-http-backend-content-length.sh\n+++ b/t/t5562-http-backend-content-length.sh\n@@ -155,8 +155,8 @@ test_expect_success 'CONTENT_LENGTH overflow ssite_t' '\n \n test_expect_success 'empty CONTENT_LENGTH' '\n \tenv \\\n-\t\tQUERY_STRING=/repo.git/HEAD \\\n-\t\tPATH_TRANSLATED=\"$PWD\"/.git/HEAD \\\n+\t\tQUERY_STRING=\"service=git-receive-pack\" \\\n+\t\tPATH_TRANSLATED=\"$PWD\"/.git/info/refs \\\n \t\tGIT_HTTP_EXPORT_ALL=TRUE \\\n \t\tREQUEST_METHOD=GET \\\n \t\tCONTENT_LENGTH=\"\" \\\n-- \n2.17.0.1185.g782057d875\n\n"},{"id":"357950","messageId":"20180912055626.GA13642@sigill.intra.peff.net","threadId":"49293","inReplyTo":"xmqqk1nrq4su.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-12T05:56:26Z","receivedAt":"2018-09-12T05:56:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 11, 2018 at 11:15:13AM -0700, Junio C Hamano wrote:\n\n> > That said, a quick search of codesearch.debian.net mostly finds\n> > examples using straight comparison, so maybe the patch is fine as-is.\n> \n> I do not think we would mind terribly if we do not support\n> combinations like gzipped-and-then-chunked from day one. An in-code\n> NEEDSWORK comment that refers to the production in RFC 2616 Page 143\n> may not hurt, though.\n\nIt's pretty common for Git to send gzip'd contents, so this might\nactually be necessary on day one. However, it looks like we do so by\nsetting the content-encoding header.\n\nI really wonder if this topic is worth pursuing further without finding\na real-world case that actually fails with the v2.19 code. I.e., is\nthere actually a server that doesn't set CONTENT_LENGTH and really can't\nhandle read-to-eof? It's plausible to me, but it's also equally\nplausible that we'd be breaking some other case.\n\n-Peff\n"},{"id":"357952","messageId":"20180912062648.GA197819@aiede.svl.corp.google.com","threadId":"49293","inReplyTo":"20180912055626.GA13642@sigill.intra.peff.net","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-09-12T06:26:48Z","receivedAt":"2018-09-12T06:26:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Sep 11, 2018 at 11:15:13AM -0700, Junio C Hamano wrote:\n\n>> I do not think we would mind terribly if we do not support\n>> combinations like gzipped-and-then-chunked from day one. An in-code\n>> NEEDSWORK comment that refers to the production in RFC 2616 Page 143\n>> may not hurt, though.\n>\n> It's pretty common for Git to send gzip'd contents, so this might\n> actually be necessary on day one. However, it looks like we do so by\n> setting the content-encoding header.\n\nCorrect, we haven't been using Transfer-Encoding for that.\n\n> I really wonder if this topic is worth pursuing further without finding\n> a real-world case that actually fails with the v2.19 code. I.e., is\n> there actually a server that doesn't set CONTENT_LENGTH and really can't\n> handle read-to-eof? It's plausible to me, but it's also equally\n> plausible that we'd be breaking some other case.\n\nI wonder about the motivating IIS case. The CGI spec says that\nCONTENT_LENGTH is set if and only if the message has a message-body.\nWhen discussing message-body, it says\n\n Request-Data = [ request-body ] [ extension-data ]\n[...]\n A request-body is supplied with the request if the CONTENT_LENGTH is\n not NULL. The server MUST make at least that many bytes available\n for the script to read. The server MAY signal an end-of-file\n condition after CONTENT_LENGTH bytes have been read or it MAY supply\n extension data. Therefore, the script MUST NOT attempt to read more\n than CONTENT_LENGTH bytes, even if more data is available.\n\nDoes that mean that if CONTENT_LENGTH is not set, then we are\nguaranteed to see EOF, because extension-data cannot be present? If\nso, then what we have in v2.19 (plus Max's test improvement that is in\n\"next\") is already enough.\n\nSo I agree.\n\n 1. Junio, please eject this patch from \"pu\", since we don't have any\n need for it.\n\n 2. IIS users, please test v2.19 and let us know how it goes.\n\nDo we have any scenarios that would use an empty POST (or other\nnon-GET) request?\n\nThanks,\nJonathan\n"},{"id":"357975","messageId":"xmqqk1nqn1bm.fsf@gitster-ct.c.googlers.com","threadId":"49293","inReplyTo":"20180912055626.GA13642@sigill.intra.peff.net","subject":"Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-12T16:10:53Z","receivedAt":"2018-09-12T16:10:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I really wonder if this topic is worth pursuing further without finding\n> a real-world case that actually fails with the v2.19 code. I.e., is\n> there actually a server that doesn't set CONTENT_LENGTH and really can't\n> handle read-to-eof? It's plausible to me, but it's also equally\n> plausible that we'd be breaking some other case.\n\nOK.\n"}]}