[PATCH] introduce hex2chr() for converting two hexadecimal digits to a character

[PATCH] introduce hex2chr() for converting two hexadecimal digits to a character

[René Scharfe]

Add and use a helper function that decodes the char value of two
hexadecimal digits.  It returns a negative number on error, avoids
running over the end of the given string and doesn't shift negative
values.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 cache.h      | 10 ++++++++++
 hex.c        | 12 ++----------
 pkt-line.c   | 23 ++---------------------
 pretty.c     | 13 +++++--------
 ref-filter.c | 20 +-------------------
 url.c        | 21 +--------------------
 6 files changed, 21 insertions(+), 78 deletions(-)

diff --git a/cache.h b/cache.h
index b780a91..b0dae4b 100644
--- a/cache.h
+++ b/cache.h
@@ -1139,6 +1139,16 @@ static inline unsigned int hexval(unsigned char c)
 	return hexval_table[c];
 }
 
+/*
+ * Convert two consecutive hexadecimal digits into a char.  Return a
+ * negative value on error.  Don't run over the end of short strings.
+ */
+static inline int hex2chr(const char *s)
+{
+	int val = hexval(s[0]);
+	return (val < 0) ? val : (val << 4) | hexval(s[1]);
+}
+
 /* Convert to/from hex/sha1 representation */
 #define MINIMUM_ABBREV minimum_abbrev
 #define DEFAULT_ABBREV default_abbrev
diff --git a/hex.c b/hex.c
index 9619b67..ab2610e 100644
--- a/hex.c
+++ b/hex.c
@@ -39,16 +39,8 @@ int get_sha1_hex(const char *hex, unsigned char *sha1)
 {
 	int i;
 	for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
-		unsigned int val;
-		/*
-		 * hex[1]=='\0' is caught when val is checked below,
-		 * but if hex[0] is NUL we have to avoid reading
-		 * past the end of the string:
-		 */
-		if (!hex[0])
-			return -1;
-		val = (hexval(hex[0]) << 4) | hexval(hex[1]);
-		if (val & ~0xff)
+		int val = hex2chr(hex);
+		if (val < 0)
 			return -1;
 		*sha1++ = val;
 		hex += 2;
diff --git a/pkt-line.c b/pkt-line.c
index 62fdb37..30489c6 100644
--- a/pkt-line.c
+++ b/pkt-line.c
@@ -172,27 +172,8 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,
 
 static int packet_length(const char *linelen)
 {
-	int n;
-	int len = 0;
-
-	for (n = 0; n < 4; n++) {
-		unsigned char c = linelen[n];
-		len <<= 4;
-		if (c >= '0' && c <= '9') {
-			len += c - '0';
-			continue;
-		}
-		if (c >= 'a' && c <= 'f') {
-			len += c - 'a' + 10;
-			continue;
-		}
-		if (c >= 'A' && c <= 'F') {
-			len += c - 'A' + 10;
-			continue;
-		}
-		return -1;
-	}
-	return len;
+	int val = hex2chr(linelen);
+	return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
 }
 
 int packet_read(int fd, char **src_buf, size_t *src_len,
diff --git a/pretty.c b/pretty.c
index 9609afb..9788bd8 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1065,7 +1065,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	const struct commit *commit = c->commit;
 	const char *msg = c->message;
 	struct commit_list *p;
-	int h1, h2;
+	int ch;
 
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
@@ -1089,14 +1089,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		return 1;
 	case 'x':
 		/* %x00 == NUL, %x0a == LF, etc. */
-		if (0 <= (h1 = hexval_table[0xff & placeholder[1]]) &&
-		    h1 <= 16 &&
-		    0 <= (h2 = hexval_table[0xff & placeholder[2]]) &&
-		    h2 <= 16) {
-			strbuf_addch(sb, (h1<<4)|h2);
-			return 3;
-		} else
+		ch = hex2chr(placeholder + 1);
+		if (ch < 0)
 			return 0;
+		strbuf_addch(sb, ch);
+		return 3;
 	case 'w':
 		if (placeholder[1] == '(') {
 			unsigned long width = 0, indent1 = 0, indent2 = 0;
diff --git a/ref-filter.c b/ref-filter.c
index bc551a7..9adbb8a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1576,24 +1576,6 @@ void ref_array_sort(struct ref_sorting *sorting, struct ref_array *array)
 	qsort(array->items, array->nr, sizeof(struct ref_array_item *), compare_refs);
 }
 
-static int hex1(char ch)
-{
-	if ('0' <= ch && ch <= '9')
-		return ch - '0';
-	else if ('a' <= ch && ch <= 'f')
-		return ch - 'a' + 10;
-	else if ('A' <= ch && ch <= 'F')
-		return ch - 'A' + 10;
-	return -1;
-}
-static int hex2(const char *cp)
-{
-	if (cp[0] && cp[1])
-		return (hex1(cp[0]) << 4) | hex1(cp[1]);
-	else
-		return -1;
-}
-
 static void append_literal(const char *cp, const char *ep, struct ref_formatting_state *state)
 {
 	struct strbuf *s = &state->stack->output;
@@ -1603,7 +1585,7 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting
 			if (cp[1] == '%')
 				cp++;
 			else {
-				int ch = hex2(cp + 1);
+				int ch = hex2chr(cp + 1);
 				if (0 <= ch) {
 					strbuf_addch(s, ch);
 					cp += 3;
diff --git a/url.c b/url.c
index 2d89ad1..eaf4f07 100644
--- a/url.c
+++ b/url.c
@@ -29,25 +29,6 @@ int is_url(const char *url)
 	return (url[0] == ':' && url[1] == '/' && url[2] == '/');
 }
 
-static int url_decode_char(const char *q)
-{
-	int i;
-	unsigned char val = 0;
-	for (i = 0; i < 2; i++) {
-		unsigned char c = *q++;
-		val <<= 4;
-		if (c >= '0' && c <= '9')
-			val += c - '0';
-		else if (c >= 'a' && c <= 'f')
-			val += c - 'a' + 10;
-		else if (c >= 'A' && c <= 'F')
-			val += c - 'A' + 10;
-		else
-			return -1;
-	}
-	return val;
-}
-
 static char *url_decode_internal(const char **query, int len,
 				 const char *stop_at, struct strbuf *out,
 				 int decode_plus)
@@ -66,7 +47,7 @@ static char *url_decode_internal(const char **query, int len,
 		}
 
 		if (c == '%') {
-			int val = url_decode_char(q + 1);
+			int val = hex2chr(q + 1);
 			if (0 <= val) {
 				strbuf_addch(out, val);
 				q += 3;
-- 
2.10.0

Re: [PATCH] introduce hex2chr() for converting two hexadecimal digits to a character

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

Hi René,

On Sat, 3 Sep 2016, René Scharfe wrote:

> Add and use a helper function that decodes the char value of two
> hexadecimal digits.  It returns a negative number on error, avoids
> running over the end of the given string and doesn't shift negative
> values.

I like it! Maybe stress a little bit why this is a good change? Like, DRY
up code, makes the code safer (bt avoiding shifting negative values)?

>  6 files changed, 21 insertions(+), 78 deletions(-)

Very, very nice!

Thanks,
Dscho

Re: [PATCH] introduce hex2chr() for converting two hexadecimal digits to a character

[René Scharfe]

Am 04.09.2016 um 09:49 schrieb Johannes Schindelin:
> Hi René,
>
> On Sat, 3 Sep 2016, René Scharfe wrote:
>
>> Add and use a helper function that decodes the char value of two
>> hexadecimal digits.  It returns a negative number on error, avoids
>> running over the end of the given string and doesn't shift negative
>> values.
>
> I like it! Maybe stress a little bit why this is a good change? Like, DRY
> up code, makes the code safer (bt avoiding shifting negative values)?
>
>>  6 files changed, 21 insertions(+), 78 deletions(-)
>
> Very, very nice!

That's the main reason: Consistency.  It's intended to be a safe, easy 
to use and reasonably fast replacement for those other (lengthy) 
variations.

René