Hello!

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

> # HG changeset patch
> # User Nate Karstens <nate.karstens@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@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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel