[PATCH 1 of 2] Core: add function to decode hexadecimal strings

[Maxim Dounin]

Hello!

On Fri, Jul 28, 2017 at 01:45:36PM -0500, Nate Karstens wrote:

> # HG changeset patch
> # User Nate Karstens <nate.karstens at garmin.com>
> # Date 1501265787 18000
> #      Fri Jul 28 13:16:27 2017 -0500
> # Node ID 7e10e1dc1fae0f538a0a74fd6783f9b33a905933
> # Parent  72d3aefc2993a639cc8cddc94aefc7c4390ee252
> Core: add function to decode hexadecimal strings.
> 
> Adds functionality to convert a hexadecimal string into binary data.
> This will be used to decode PSKs stored in hexadecimal representation.
> 
> Signed-off-by: Nate Karstens <nate.karstens at garmin.com>
> 
> diff -r 72d3aefc2993 -r 7e10e1dc1fae src/core/ngx_string.c
> --- a/src/core/ngx_string.c     Wed Jul 26 13:13:51 2017 +0300
> +++ b/src/core/ngx_string.c     Fri Jul 28 13:16:27 2017 -0500
> @@ -1104,6 +1104,54 @@ ngx_hextoi(u_char *line, size_t n)
>  }
> 
> 
> +ngx_int_t
> +ngx_hex_decode(u_char *dst, size_t dlen, u_char *src, size_t slen)
> +{
> +    u_char     c, ch;
> +    ngx_int_t  len;
> +
> +    if ((slen & 1) || (dlen < (slen / 2))) {
> +        return NGX_ERROR;

It doesn't look like dlen is useful in the interface, at least in 
the for it is implemented.  It could make sense if the function 
would written in a way which allows decoding only some bytes of 
the source string, but a simple "buffer must be at least as large 
as half of the source string" prerequisite certainly doesn't need 
an additional parameter.  For example, ngx_hex_dump() doesn't have 
it.

> +    }
> +
> +    len = slen / 2;

Returning a length known in advice might not be a good idea 
either.  It is already known to the caller.  Rather, I would 
recommend returning NGX_OK as, for example, ngx_decode_base64() 
does.

> +
> +    while (slen > 0) {
> +        ch = *src;
> +        c = (u_char) (ch | 0x20);
> +
> +        if (ch >= '0' && ch <= '9') {
> +            *dst = ch - '0';
> +        } else if (c >= 'a' && c <= 'f') {
> +            *dst = c - 'a' + 10;

It might be a good idea to introduce a u_char variable on stack, 
similar to how it is done in ngx_http_parse_complex_uri(). 

It also might be a good idea to restructure the code to avoid 
meaningless lowercase operations for 0..9 characters.  And I see 
no reason to use two variables, "ch" and "c".

It might also need some additional empty lines to better match 
style, though the code is anyway needs to be rewritten.

> +        } else {
> +            return NGX_ERROR;
> +        }
> +
> +        *dst <<= 4;
> +        src++;

It might be more logical to combine src++ with reading the 
character,

   ch = *src++;

Such approach can be seen in many other functions in ngx_string.c.

> +
> +        ch = *src;
> +        c = (u_char) (ch | 0x20);
> +
> +        if (ch >= '0' && ch <= '9') {
> +            *dst |= ch - '0';
> +        } else if (c >= 'a' && c <= 'f') {
> +            *dst |= c - 'a' + 10;
> +        } else {
> +            return NGX_ERROR;
> +        }
> +
> +        dst++;
> +        src++;
> +
> +        slen -= 2;
> +    }
> +
> +    return len;
> +}
> +
> +
>  u_char *
>  ngx_hex_dump(u_char *dst, u_char *src, size_t len)
>  {
> diff -r 72d3aefc2993 -r 7e10e1dc1fae src/core/ngx_string.h
> --- a/src/core/ngx_string.h     Wed Jul 26 13:13:51 2017 +0300
> +++ b/src/core/ngx_string.h     Fri Jul 28 13:16:27 2017 -0500
> @@ -176,6 +176,7 @@ off_t ngx_atoof(u_char *line, size_t n);
>  time_t ngx_atotm(u_char *line, size_t n);
>  ngx_int_t ngx_hextoi(u_char *line, size_t n);
> 
> +ngx_int_t ngx_hex_decode(u_char *dst, size_t dlen, u_char *src, size_t slen);
>  u_char *ngx_hex_dump(u_char *dst, u_char *src, size_t len);

It might be a good idea to change the order, so the decode 
function is placed after an encode one, as in other similar 
functions in the same files.

-- 
Maxim Dounin
http://nginx.org/