# [PATCH] Fix urlencode format string on signed char. 4 messages from 2017-12-22 to 2017-12-23. Participants: Julien Dusser, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/47484 ## Julien Dusser, 2017-12-22 17:24 Subject: [PATCH] Fix urlencode format string on signed char. Message-ID: <20171222172437.19505-1-julien.dusser@free.fr> URL: https://gitlist.dev/e/20171222172437.19505-1-julien.dusser%40free.fr ``` Git credential fails with special char in password. remote: Invalid username or password. fatal: Authentication failed for File ~/.git-credential contains badly urlencoded characters %ffffffXX%ffffffYY instead of %XX%YY. Add a cast to an unsigned char to fix urlencode use of %02x on a char. Signed-off-by: Julien Dusser --- strbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strbuf.c b/strbuf.c index 323c49ceb..4d5a9ce55 100644 --- a/strbuf.c +++ b/strbuf.c @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len, (!reserved && is_rfc3986_reserved(ch))) strbuf_addch(sb, ch); else - strbuf_addf(sb, "%%%02x", ch); + strbuf_addf(sb, "%%%02x", (unsigned char)ch); } } -- 2.15.1 ``` ## Junio C Hamano, 2017-12-22 21:48 Subject: Re: [PATCH] Fix urlencode format string on signed char. Message-ID: URL: https://gitlist.dev/e/xmqqk1xeph6r.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20171222172437.19505-1-julien.dusser@free.fr> ``` Julien Dusser writes: > Git credential fails with special char in password. > remote: Invalid username or password. > fatal: Authentication failed for > > File ~/.git-credential contains badly urlencoded characters > %ffffffXX%ffffffYY instead of %XX%YY. > > Add a cast to an unsigned char to fix urlencode use of %02x > on a char. > > Signed-off-by: Julien Dusser > --- > strbuf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/strbuf.c b/strbuf.c > index 323c49ceb..4d5a9ce55 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len, > (!reserved && is_rfc3986_reserved(ch))) > strbuf_addch(sb, ch); > else > - strbuf_addf(sb, "%%%02x", ch); > + strbuf_addf(sb, "%%%02x", (unsigned char)ch); > } > } The issue is not limited to credential but anywhere where we need to show a byte with hi-bit set, and it is obvious and straight-forward. I briefly wondered if the data type for the strings involved in the codepaths that reach this place should all be "uchar*" but it feels strange to have "unsigned char *username" etc., and the signeness matters only here, so the patch smells like the best one among other possibilities. Thanks. ``` ## Julien Dusser, 2017-12-22 23:49 Subject: Re: [PATCH] Fix urlencode format string on signed char. Message-ID: <9073fcc7-c751-5563-81c8-d4992eefd298@free.fr> URL: https://gitlist.dev/e/9073fcc7-c751-5563-81c8-d4992eefd298%40free.fr In-Reply-To: ``` Thank you for review. I didn't find any other error. Code in http.c:quote_ref_url() is almost the same but ch is a signed int, so there's no issue. Le 22/12/2017 à 22:48, Junio C Hamano a écrit : > Julien Dusser writes: > >> Git credential fails with special char in password. >> remote: Invalid username or password. >> fatal: Authentication failed for >> >> File ~/.git-credential contains badly urlencoded characters >> %ffffffXX%ffffffYY instead of %XX%YY. >> >> Add a cast to an unsigned char to fix urlencode use of %02x >> on a char. >> >> Signed-off-by: Julien Dusser >> --- >> strbuf.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/strbuf.c b/strbuf.c >> index 323c49ceb..4d5a9ce55 100644 >> --- a/strbuf.c >> +++ b/strbuf.c >> @@ -658,7 +658,7 @@ static void strbuf_add_urlencode(struct strbuf *sb, const char *s, size_t len, >> (!reserved && is_rfc3986_reserved(ch))) >> strbuf_addch(sb, ch); >> else >> - strbuf_addf(sb, "%%%02x", ch); >> + strbuf_addf(sb, "%%%02x", (unsigned char)ch); >> } >> } > > The issue is not limited to credential but anywhere where we need to > show a byte with hi-bit set, and it is obvious and straight-forward. > > I briefly wondered if the data type for the strings involved in the > codepaths that reach this place should all be "uchar*" but it feels > strange to have "unsigned char *username" etc., and the signeness > matters only here, so the patch smells like the best one among other > possibilities. > > Thanks. > ``` ## Johannes Schindelin, 2017-12-23 00:29 Subject: Re: [PATCH] Fix urlencode format string on signed char. Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.21.1.1712230125341.406%40MININT-6BKU6QN.europe.corp.microsoft.com In-Reply-To: <9073fcc7-c751-5563-81c8-d4992eefd298@free.fr> ``` Hi Julien, On Sat, 23 Dec 2017, Julien Dusser wrote: > Thank you for review. I didn't find any other error. > Code in http.c:quote_ref_url() is almost the same but ch is a signed int, so > there's no issue. But that ch comes from a signed char *, so it actually *is* an issue: if you cast a signed char of value -1 to an int, it will still be -1. So we'll also need: -- snipsnap -- diff --git a/http.c b/http.c index 117ddae..ed8221f 100644 --- a/http.c +++ b/http.c @@ -1347,7 +1347,7 @@ void finish_all_active_slots(void) } /* Helpers for modifying and creating URLs */ -static inline int needs_quote(int ch) +static inline int needs_quote(unsigned char ch) { if (((ch >= 'A') && (ch <= 'Z')) || ((ch >= 'a') && (ch <= 'z')) @@ -1363,11 +1363,11 @@ static char *quote_ref_url(const char *base, const char *ref) { struct strbuf buf = STRBUF_INIT; const char *cp; - int ch; + unsigned char ch; end_url_with_slash(&buf, base); - for (cp = ref; (ch = *cp) != 0; cp++) + for (cp = ref; (ch = (unsigned char)*cp); cp++) if (needs_quote(ch)) strbuf_addf(&buf, "%%%02x", ch); else ```