{"thread":{"id":"47484","subject":"[PATCH] Fix urlencode format string on signed char.","startedAt":"2017-12-22T17:23:02Z","lastAt":"2017-12-23T00:29:10Z","messageCount":4,"participants":["Julien Dusser","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"335175","messageId":"20171222172437.19505-1-julien.dusser@free.fr","threadId":"47484","inReplyTo":null,"subject":"[PATCH] Fix urlencode format string on signed char.","fromName":"Julien Dusser","fromEmail":"julien.dusser@free.fr","sentAt":"2017-12-22T17:24:37Z","receivedAt":"2017-12-22T17:23:02Z","isPatch":true,"sender":{"key":"julien.dusser@free.fr","avatar":null},"body":"Git credential fails with special char in password.\nremote: Invalid username or password.\nfatal: Authentication failed for\n\nFile ~/.git-credential contains badly urlencoded characters\n%ffffffXX%ffffffYY instead of %XX%YY.\n\nAdd a cast to an unsigned char to fix urlencode use of %02x\non a char.\n\nSigned-off-by: Julien Dusser \n---\n strbuf.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 323c49ceb..4d5a9ce55 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,\n \t\t (!reserved && is_rfc3986_reserved(ch)))\n \t\t\tstrbuf_addch(sb, ch);\n \t\telse\n-\t\t\tstrbuf_addf(sb, \"%%%02x\", ch);\n+\t\t\tstrbuf_addf(sb, \"%%%02x\", (unsigned char)ch);\n \t}\n }\n \n-- \n2.15.1\n\n"},{"id":"335199","messageId":"xmqqk1xeph6r.fsf@gitster.mtv.corp.google.com","threadId":"47484","inReplyTo":"20171222172437.19505-1-julien.dusser@free.fr","subject":"Re: [PATCH] Fix urlencode format string on signed char.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-22T21:48:44Z","receivedAt":"2017-12-22T21:48:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Julien Dusser writes:\n\n> Git credential fails with special char in password.\n> remote: Invalid username or password.\n> fatal: Authentication failed for\n>\n> File ~/.git-credential contains badly urlencoded characters\n> %ffffffXX%ffffffYY instead of %XX%YY.\n>\n> Add a cast to an unsigned char to fix urlencode use of %02x\n> on a char.\n>\n> Signed-off-by: Julien Dusser \n> ---\n> strbuf.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 323c49ceb..4d5a9ce55 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,\n> \t\t (!reserved && is_rfc3986_reserved(ch)))\n> \t\t\tstrbuf_addch(sb, ch);\n> \t\telse\n> -\t\t\tstrbuf_addf(sb, \"%%%02x\", ch);\n> +\t\t\tstrbuf_addf(sb, \"%%%02x\", (unsigned char)ch);\n> \t}\n> }\n\nThe issue is not limited to credential but anywhere where we need to\nshow a byte with hi-bit set, and it is obvious and straight-forward.\n\nI briefly wondered if the data type for the strings involved in the\ncodepaths that reach this place should all be \"uchar*\" but it feels\nstrange to have \"unsigned char *username\" etc., and the signeness\nmatters only here, so the patch smells like the best one among other\npossibilities.\n\nThanks.\n"},{"id":"335205","messageId":"9073fcc7-c751-5563-81c8-d4992eefd298@free.fr","threadId":"47484","inReplyTo":"xmqqk1xeph6r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] Fix urlencode format string on signed char.","fromName":"Julien Dusser","fromEmail":"julien.dusser@free.fr","sentAt":"2017-12-22T23:49:07Z","receivedAt":"2017-12-22T23:47:18Z","isPatch":true,"sender":{"key":"julien.dusser@free.fr","avatar":null},"body":"Thank you for review. I didn't find any other error.\nCode in http.c:quote_ref_url() is almost the same but ch is a signed \nint, so there's no issue.\n\nLe 22/12/2017 à 22:48, Junio C Hamano a écrit :\n> Julien Dusser writes:\n> \n>> Git credential fails with special char in password.\n>> remote: Invalid username or password.\n>> fatal: Authentication failed for\n>>\n>> File ~/.git-credential contains badly urlencoded characters\n>> %ffffffXX%ffffffYY instead of %XX%YY.\n>>\n>> Add a cast to an unsigned char to fix urlencode use of %02x\n>> on a char.\n>>\n>> Signed-off-by: Julien Dusser \n>> ---\n>> strbuf.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/strbuf.c b/strbuf.c\n>> index 323c49ceb..4d5a9ce55 100644\n>> --- a/strbuf.c\n>> +++ b/strbuf.c\n>> @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len,\n>> \t\t (!reserved && is_rfc3986_reserved(ch)))\n>> \t\t\tstrbuf_addch(sb, ch);\n>> \t\telse\n>> -\t\t\tstrbuf_addf(sb, \"%%%02x\", ch);\n>> +\t\t\tstrbuf_addf(sb, \"%%%02x\", (unsigned char)ch);\n>> \t}\n>> }\n> \n> The issue is not limited to credential but anywhere where we need to\n> show a byte with hi-bit set, and it is obvious and straight-forward.\n> \n> I briefly wondered if the data type for the strings involved in the\n> codepaths that reach this place should all be \"uchar*\" but it feels\n> strange to have \"unsigned char *username\" etc., and the signeness\n> matters only here, so the patch smells like the best one among other\n> possibilities.\n> \n> Thanks.\n> \n\n"},{"id":"335214","messageId":"alpine.DEB.2.21.1.1712230125341.406@MININT-6BKU6QN.europe.corp.microsoft.com","threadId":"47484","inReplyTo":"9073fcc7-c751-5563-81c8-d4992eefd298@free.fr","subject":"Re: [PATCH] Fix urlencode format string on signed char.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-23T00:29:00Z","receivedAt":"2017-12-23T00:29:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Julien,\n\nOn Sat, 23 Dec 2017, Julien Dusser wrote:\n\n> Thank you for review. I didn't find any other error.\n> Code in http.c:quote_ref_url() is almost the same but ch is a signed int, so\n> there's no issue.\n\nBut that ch comes from a signed char *, so it actually *is* an issue: if\nyou cast a signed char of value -1 to an int, it will still be -1. So\nwe'll also need:\n\n-- snipsnap --\ndiff --git a/http.c b/http.c\nindex 117ddae..ed8221f 100644\n--- a/http.c\n+++ b/http.c\n@@ -1347,7 +1347,7 @@ void finish_all_active_slots(void)\n }\n \n /* Helpers for modifying and creating URLs */\n-static inline int needs_quote(int ch)\n+static inline int needs_quote(unsigned char ch)\n {\n \tif (((ch >= 'A') && (ch <= 'Z'))\n \t\t\t|| ((ch >= 'a') && (ch <= 'z'))\n@@ -1363,11 +1363,11 @@ static char *quote_ref_url(const char *base, const char *ref)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *cp;\n-\tint ch;\n+\tunsigned char ch;\n \n \tend_url_with_slash(&buf, base);\n \n-\tfor (cp = ref; (ch = *cp) != 0; cp++)\n+\tfor (cp = ref; (ch = (unsigned char)*cp); cp++)\n \t\tif (needs_quote(ch))\n \t\t\tstrbuf_addf(&buf, \"%%%02x\", ch);\n \t\telse\n"}]}