{"thread":{"id":"34308","subject":"[PATCH 1/2] commit: reject invalid UTF-8 codepoints","startedAt":"2013-06-29T17:40:23Z","lastAt":"2013-07-04T00:47:08Z","messageCount":7,"participants":["brian m. carlson","Junio C Hamano","Peter Krefting"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"222231","messageId":"20130629174023.GB226907@vauxhall.crustytoothpaste.net","threadId":"34308","inReplyTo":null,"subject":"[PATCH 1/2] commit: reject invalid UTF-8 codepoints","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-06-29T17:40:23Z","receivedAt":"2013-06-29T17:40:23Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The commit code already contains code for validating UTF-8, but it does not\ncheck for invalid values, such as guaranteed non-characters and surrogates.  Fix\nthis by explicitly checking for and rejecting such characters.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n commit.c                  | 18 +++++++++++++++---\n t/t3900-i18n-commit.sh    |  9 +++++++++\n t/t3900/UTF-8-invalid.txt |  3 +++\n 3 files changed, 27 insertions(+), 3 deletions(-)\n create mode 100644 t/t3900/UTF-8-invalid.txt\n\ndiff --git a/commit.c b/commit.c\nindex 888e02a..2280413 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1244,6 +1244,7 @@ static int find_invalid_utf8(const char *buf, int len)\n \twhile (len) {\n \t\tunsigned char c = *buf++;\n \t\tint bytes, bad_offset;\n+\t\tunsigned int codepoint;\n \n \t\tlen--;\n \t\toffset++;\n@@ -1264,24 +1265,35 @@ static int find_invalid_utf8(const char *buf, int len)\n \t\t\tbytes++;\n \t\t}\n \n-\t\t/* Must be between 1 and 5 more bytes */\n-\t\tif (bytes < 1 || bytes > 5)\n+\t\t/* Must be between 1 and 3 more bytes */\n+\t\tif (bytes < 1 || bytes > 3)\n \t\t\treturn bad_offset;\n \n \t\t/* Do we *have* that many bytes? */\n \t\tif (len < bytes)\n \t\t\treturn bad_offset;\n \n+\t\t/* Place the encoded bits at the bottom of the value. */\n+\t\tcodepoint = (c & 0x7f) >> bytes;\n+\n \t\toffset += bytes;\n \t\tlen -= bytes;\n \n \t\t/* And verify that they are good continuation bytes */\n \t\tdo {\n+\t\t\tcodepoint <<= 6;\n+\t\t\tcodepoint |= *buf & 0x3f;\n \t\t\tif ((*buf++ & 0xc0) != 0x80)\n \t\t\t\treturn bad_offset;\n \t\t} while (--bytes);\n \n-\t\t/* We could/should check the value and length here too */\n+\t\t/* Check the value here */\n+\t\tif (codepoint >= 0xd800 && codepoint <= 0xdfff)\n+\t\t\treturn bad_offset;\n+\t\tif (codepoint > 0x10ffff)\n+\t\t\treturn bad_offset;\n+\t\tif ((codepoint & 0x1ffffe) == 0xfffe)\n+\t\t\treturn bad_offset;\n \t}\n \treturn -1;\n }\ndiff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\nindex 37ddabb..16ed707 100755\n--- a/t/t3900-i18n-commit.sh\n+++ b/t/t3900-i18n-commit.sh\n@@ -39,6 +39,15 @@ test_expect_failure 'UTF-16 refused because of NULs' '\n \tgit commit -a -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt\n '\n \n+test_expect_success 'UTF-8 invalid characters refused' '\n+\trm -f \"$HOME/stderr\" &&\n+\techo \"UTF-8 characters\" >F &&\n+\tgit commit -a -F \"$TEST_DIRECTORY\"/t3900/UTF-8-invalid.txt \\\n+\t\t2>\"$HOME\"/stderr &&\n+\tgrep \"did not conform\" \"$HOME\"/stderr\n+'\n+\n+rm -f \"$HOME/stderr\"\n \n for H in ISO8859-1 eucJP ISO-2022-JP\n do\ndiff --git a/t/t3900/UTF-8-invalid.txt b/t/t3900/UTF-8-invalid.txt\nnew file mode 100644\nindex 0000000..343684d\n--- /dev/null\n+++ b/t/t3900/UTF-8-invalid.txt\n@@ -0,0 +1,3 @@\n+Commit message\n+\n+Invalid surrogate:???\n-- \n1.8.3.1\n"},{"id":"222236","messageId":"7v7ghcl50r.fsf@alter.siamese.dyndns.org","threadId":"34308","inReplyTo":"20130629174023.GB226907@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 1/2] commit: reject invalid UTF-8 codepoints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-30T02:13:40Z","receivedAt":"2013-06-30T02:13:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> The commit code already contains code for validating UTF-8, but it\n> does not check for invalid values, such as guaranteed\n> non-characters and surrogates.  Fix this by explicitly checking\n> for and rejecting such characters.\n>\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> ---\n\nDoes this correspond to the following comment in the same file, and\nif so, shouldn't this part of your patch?\n\ndiff --git a/commit.c b/commit.c\nindex 888e02a..8ce20d5 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1292,8 +1292,8 @@ static int find_invalid_utf8(const char *buf, int len)\n  * If it isn't, it assumes any non-utf8 characters are Latin1,\n  * and does the conversion.\n  *\n- * Fixme: we should probably also disallow overlong forms and\n- * invalid characters. But we don't do that currently.\n+ * Fixme: we should probably also disallow overlong forms.\n+ * But we don't do that fully, yet.\n  */\n static int verify_utf8(struct strbuf *buf)\n {\n\n> @@ -1264,24 +1265,35 @@ static int find_invalid_utf8(const char *buf, int len)\n>  \t\t\tbytes++;\n>  \t\t}\n>  \n> -\t\t/* Must be between 1 and 5 more bytes */\n> -\t\tif (bytes < 1 || bytes > 5)\n> +\t\t/* Must be between 1 and 3 more bytes */\n> +\t\tif (bytes < 1 || bytes > 3)\n>  \t\t\treturn bad_offset;\n\nWe used to allow te original up-to-6 form and this update is about\nrejecting anything above U+10FFFF (in line with e.g. RFC 3629)?\n\n>  \t\t/* Do we *have* that many bytes? */\n>  \t\tif (len < bytes)\n>  \t\t\treturn bad_offset;\n>  \n> +\t\t/* Place the encoded bits at the bottom of the value. */\n> +\t\tcodepoint = (c & 0x7f) >> bytes;\n> +\n>  \t\toffset += bytes;\n>  \t\tlen -= bytes;\n>  \n>  \t\t/* And verify that they are good continuation bytes */\n>  \t\tdo {\n> +\t\t\tcodepoint <<= 6;\n> +\t\t\tcodepoint |= *buf & 0x3f;\n>  \t\t\tif ((*buf++ & 0xc0) != 0x80)\n>  \t\t\t\treturn bad_offset;\n>  \t\t} while (--bytes);\n>  \n> -\t\t/* We could/should check the value and length here too */\n> +\t\t/* Check the value here */\n> +\t\tif (codepoint >= 0xd800 && codepoint <= 0xdfff)\n> +\t\t\treturn bad_offset;\n\nAs you use separate if statements for this check, perhaps you can\ngive each of them a better comment to say what each is rejecting?\nE.g.\n\n\t/* do not allow range for surrogate pair */\n\t\tif (codepoint >= 0xd800 && codepoint <= 0xdfff)\n\t\t\treturn bad_offset;\n\n> +\t\tif (codepoint > 0x10ffff)\n> +\t\t\treturn bad_offset;\n> +\t\tif ((codepoint & 0x1ffffe) == 0xfffe)\n> +\t\t\treturn bad_offset;\n\nAs that comment I quoted at the beginning says, we did not check for\ninvalid encoded values and the primary reason for it is beccause\nthis function did not want to look into the actual values here.  But\nnow you are looking into \"codepoint\", you can now also check for\n\"overlong\" form (e.g. sequence \"C0 80\" turning into U+0000)?\n\n>  \t}\n>  \treturn -1;\n>  }\n> diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh\n> index 37ddabb..16ed707 100755\n> --- a/t/t3900-i18n-commit.sh\n> +++ b/t/t3900-i18n-commit.sh\n> @@ -39,6 +39,15 @@ test_expect_failure 'UTF-16 refused because of NULs' '\n>  \tgit commit -a -F \"$TEST_DIRECTORY\"/t3900/UTF-16.txt\n>  '\n>  \n> +test_expect_success 'UTF-8 invalid characters refused' '\n> +\trm -f \"$HOME/stderr\" &&\n> +\techo \"UTF-8 characters\" >F &&\n> +\tgit commit -a -F \"$TEST_DIRECTORY\"/t3900/UTF-8-invalid.txt \\\n> +\t\t2>\"$HOME\"/stderr &&\n> +\tgrep \"did not conform\" \"$HOME\"/stderr\n> +'\n> +\n> +rm -f \"$HOME/stderr\"\n\nPerhaps\n\n        test_expect_success 'test name' '\n                test_when_finished \"rm -f $HOME/stderr\" &&\n                \"rm -f $HOME/stderr\" &&\n                echo ...\n                grep \"did not conform\" ...\n        '\n\n> diff --git a/t/t3900/UTF-8-invalid.txt b/t/t3900/UTF-8-invalid.txt\n> new file mode 100644\n> index 0000000..343684d\n> --- /dev/null\n> +++ b/t/t3900/UTF-8-invalid.txt\n> @@ -0,0 +1,3 @@\n> +Commit message\n> +\n> +Invalid surrogate:???\n\nI suspect that I did not receive what you intended to send.  Do you\nwant to send this part as a binary patch perhaps?\n"},{"id":"222237","messageId":"20130630022921.GP862789@vauxhall.crustytoothpaste.net","threadId":"34308","inReplyTo":"7v7ghcl50r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] commit: reject invalid UTF-8 codepoints","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-06-30T02:29:21Z","receivedAt":"2013-06-30T02:29:21Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Jun 29, 2013 at 07:13:40PM -0700, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> Does this correspond to the following comment in the same file, and\n> if so, shouldn't this part of your patch?\n\nYes, yes, it should.\n\n> We used to allow te original up-to-6 form and this update is about\n> rejecting anything above U+10FFFF (in line with e.g. RFC 3629)?\n\nCorrect.  Since no codepoints above U+10FFFF will ever be assigned,\nthere's no point in trying to allow those UTF-8 sequences, since they\nwill be caught below anyway.  Some four-byte sequences will produce\ntoo-large codepoints, but all five- and six-byte sequences are\nguaranteed to.\n\n> As you use separate if statements for this check, perhaps you can\n> give each of them a better comment to say what each is rejecting?\n> E.g.\n> \n> \t/* do not allow range for surrogate pair */\n> \t\tif (codepoint >= 0xd800 && codepoint <= 0xdfff)\n> \t\t\treturn bad_offset;\n\nSure.\n\n> As that comment I quoted at the beginning says, we did not check for\n> invalid encoded values and the primary reason for it is beccause\n> this function did not want to look into the actual values here.  But\n> now you are looking into \"codepoint\", you can now also check for\n> \"overlong\" form (e.g. sequence \"C0 80\" turning into U+0000)?\n\nCorrect.  That's what my second patch does.  I thought I would make the\nchanges separately, since they're slightly different and from what I've\nseen git development prefers small, independent patches, but if you'd\nprefer, I can squash them into one patch.\n\n> Perhaps\n> \n>         test_expect_success 'test name' '\n>                 test_when_finished \"rm -f $HOME/stderr\" &&\n\nI wasn't aware that existed, but it makes sense to use it.\n\n> > +Invalid surrogate:???\n> \n> I suspect that I did not receive what you intended to send.  Do you\n> want to send this part as a binary patch perhaps?\n\nIf you ended up with an encoding of U+D800, then you got it.  Otherwise,\nI can resend it as a binary patch during the reroll.\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"222259","messageId":"7vhagfjt22.fsf@alter.siamese.dyndns.org","threadId":"34308","inReplyTo":"20130630022921.GP862789@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 1/2] commit: reject invalid UTF-8 codepoints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-06-30T19:29:41Z","receivedAt":"2013-06-30T19:29:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> On Sat, Jun 29, 2013 at 07:13:40PM -0700, Junio C Hamano wrote:\n>> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n>> Does this correspond to the following comment in the same file, and\n>> if so, shouldn't this part of your patch?\n>\n> Yes, yes, it should.\n> ...\n>> As that comment I quoted at the beginning says, we did not check for\n>> invalid encoded values and the primary reason for it is beccause\n>> this function did not want to look into the actual values here.  But\n>> now you are looking into \"codepoint\", you can now also check for\n>> \"overlong\" form (e.g. sequence \"C0 80\" turning into U+0000)?\n>\n> Correct.  That's what my second patch does.\n\nAh, I started with that quoted comment when I saw 1/2, thinking that\nyou may not even have noticed that these two things are our \"todo\"\nlist, hence I thought it would be natural to do them both in the\nsame commit as the logic to do so is in the single place (the\nfunction you patched).  It is not _wrong_ per-se to do this in two\nsteps, but I do not think it is necessary.  You *could* break this\nto even smaller steps to go to the other extreme, first limiting\nonly upto 4-byte forms, then rejecting a half of the 4-byte form,\nthen rejecting the range for surrogates, and then rejecting overlong\nforms, and the split may be technically logical and \"one step at a\ntime\" but that kind of granularity is probably more noisy than it is\nvaluable.\n\nI think \"1/2 is about rejecting a codepoint outside valid ranges and\n2/2 is about rejecting a valid codepoint inside the valid range but\nexpressed in an invalid way\" is on the borderline between the two\nextremes, but it probably is on the saner side, so let's keep these\ntwo patches separate.\n\n> If you ended up with an encoding of U+D800, then you got it.\n\nI think we are seeing ? U+003F in our mailboxes.\n\nThanks.\n"},{"id":"222269","messageId":"alpine.DEB.2.00.1307010811090.21709@ds9.cixit.se","threadId":"34308","inReplyTo":"20130629174023.GB226907@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 1/2] commit: reject invalid UTF-8 codepoints","fromName":"Peter Krefting","fromEmail":"peter@softwolves.pp.se","sentAt":"2013-07-01T07:12:40Z","receivedAt":"2013-07-01T07:12:40Z","isPatch":true,"sender":{"key":"peter@softwolves.pp.se","avatar":"https://avatars.githubusercontent.com/u/990764?v=4"},"body":"brian m. carlson:\n\n> +\t\t/* Check the value here */\n> +\t\tif (codepoint >= 0xd800 && codepoint <= 0xdfff)\n> +\t\t\treturn bad_offset;\n\nif ((x & 0xFFFFF800) == 0xD800)\n\nis slightly shorter, albeit a bit more difficult to read.\n\nPlease also consider adding some commentary as to what you are \nchecking here, as it is not obvious unless you know Unicode well.\n\n-- \n\\\\// Peter - http://www.softwolves.pp.se/\n"},{"id":"222524","messageId":"20130704001724.GQ862789@vauxhall.crustytoothpaste.net","threadId":"34308","inReplyTo":"7v7ghcl50r.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] commit: reject invalid UTF-8 codepoints","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2013-07-04T00:17:24Z","receivedAt":"2013-07-04T00:17:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Jun 29, 2013 at 07:13:40PM -0700, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> > diff --git a/t/t3900/UTF-8-invalid.txt b/t/t3900/UTF-8-invalid.txt\n> > new file mode 100644\n> > index 0000000..343684d\n> > --- /dev/null\n> > +++ b/t/t3900/UTF-8-invalid.txt\n> > @@ -0,0 +1,3 @@\n> > +Commit message\n> > +\n> > +Invalid surrogate:???\n> \n> I suspect that I did not receive what you intended to send.  Do you\n> want to send this part as a binary patch perhaps?\n\ngit format-patch --binary seems to produce the exact same output as\nwithout the --binary option.  Is there a different way I should be\nselecting the binary option?\n\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187\n"},{"id":"222525","messageId":"7vehbfcfsj.fsf@alter.siamese.dyndns.org","threadId":"34308","inReplyTo":"20130704001724.GQ862789@vauxhall.crustytoothpaste.net","subject":"Re: [PATCH 1/2] commit: reject invalid UTF-8 codepoints","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-04T00:47:08Z","receivedAt":"2013-07-04T00:47:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> git format-patch --binary seems to produce the exact same output as\n> without the --binary option.  Is there a different way I should be\n> selecting the binary option?\n\nThese days --binary is a no-op option for format-patch (we used to\ndefault to showing \"binary files differ\" in diff and required the\noption to force generating binary patch), but the real issue is that\nyou haven't told Git that these \"invalid\" files are binary files and\nnot text X-<.\n\nPerhaps add something like this temporarily to .git/info/attributes\nwhen running the command?\n\n\t$ cat >>.git/info/attributes <<\\EOF\n        t/t3900/UTF-8-invalid.txt\tbinary\n        EOF\n\nAnother idea is _not_ to create these invalid files inside t3900/\nbut create them in the set-up part of the test script to the current\ndirectory, using \"printf\" to make sure the readers who read test\nknow what exact byte sequence is written out in the test and is\nused, which might be a better option in the long run.\n\nThanks.\n"}]}