Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] Core: fix conversion of IPv4-mapped IPv6 addresses

Sergey Kandaurov
March 15, 2024 12:14PM
> On 28 Feb 2024, at 05:21, Piotr Sikora via nginx-devel <nginx-devel@nginx.org> wrote:
>
> # HG changeset patch
> # User Piotr Sikora <piotr@aviatrix.com>
> # Date 1708977626 0
> # Mon Feb 26 20:00:26 2024 +0000
> # Branch patch007
> # Node ID 5584232259d28489efba149f2f5ae730691ff0d4
> # Parent 03e5549976765912818120e11f6b08410a2af6a9
> Core: fix conversion of IPv4-mapped IPv6 addresses.
>
> Found with UndefinedBehaviorSanitizer (shift).

Hello, thanks for the patch.

The "shift" remark doesn't describe a problem in details.

What actually happens is:
- loading from "u_char", with integer promotion
- left shift by 24 to most significant bit 31

The latter cannot be represented in (signed) integer in popular
data type models: bit 31 gets to the integer type sign bit,
and is caught by UndefinedBehaviorSanitizer, e.g.:
| runtime error: left shift of 255 by 24 places cannot be represented in type 'int'

The lvalue typically has an unsigned integer type of the same
size (POSIX states in_addr_t is equivalent to uint32_t, which
is of the same type as "unsigned int" in ILP32 and LP64).
Known compilers, such as GCC and Clang, represent intermediate
result used as a source for left shift in a wide register such that
it doesn't get truncated when storing it in the left value.
This makes objects don't change after adding an explicit cast,
comparing md5sum over them matches.

Notably, this differs from left-shifting signed integer
into the sign bit fixed in ad736705a744, where, as described
in the change, the left value is of the larger type width.

Still, I won't object to apply something similar to ad736705a744
in order to suppress such UndefinedBehaviorSanitizer reports.
They look valid, although seemingly innocent.

>
> Signed-off-by: Piotr Sikora <piotr@aviatrix.com>
>
> diff -r 03e554997676 -r 5584232259d2 src/core/ngx_inet.c
> --- a/src/core/ngx_inet.c Mon Feb 26 20:00:23 2024 +0000
> +++ b/src/core/ngx_inet.c Mon Feb 26 20:00:26 2024 +0000
> @@ -507,10 +507,10 @@
>
> p = inaddr6->s6_addr;
>
> - inaddr = p[12] << 24;
> - inaddr += p[13] << 16;
> - inaddr += p[14] << 8;
> - inaddr += p[15];
> + inaddr = (in_addr_t) p[12] << 24;
> + inaddr += (in_addr_t) p[13] << 16;
> + inaddr += (in_addr_t) p[14] << 8;
> + inaddr += (in_addr_t) p[15];
>
> inaddr = htonl(inaddr);
> }
> diff -r 03e554997676 -r 5584232259d2 src/http/modules/ngx_http_access_module.c
> --- a/src/http/modules/ngx_http_access_module.c Mon Feb 26 20:00:23 2024 +0000
> +++ b/src/http/modules/ngx_http_access_module.c Mon Feb 26 20:00:26 2024 +0000
> @@ -148,10 +148,10 @@
> p = sin6->sin6_addr.s6_addr;
>
> if (alcf->rules && IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) {
> - addr = p[12] << 24;
> - addr += p[13] << 16;
> - addr += p[14] << 8;
> - addr += p[15];
> + addr = (in_addr_t) p[12] << 24;
> + addr += (in_addr_t) p[13] << 16;
> + addr += (in_addr_t) p[14] << 8;
> + addr += (in_addr_t) p[15];
> return ngx_http_access_inet(r, alcf, htonl(addr));
> }
>
> diff -r 03e554997676 -r 5584232259d2 src/http/modules/ngx_http_geo_module.c
> --- a/src/http/modules/ngx_http_geo_module.c Mon Feb 26 20:00:23 2024 +0000
> +++ b/src/http/modules/ngx_http_geo_module.c Mon Feb 26 20:00:26 2024 +0000
> @@ -199,10 +199,10 @@
> p = inaddr6->s6_addr;
>
> if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
> - inaddr = p[12] << 24;
> - inaddr += p[13] << 16;
> - inaddr += p[14] << 8;
> - inaddr += p[15];
> + inaddr = (in_addr_t) p[12] << 24;
> + inaddr += (in_addr_t) p[13] << 16;
> + inaddr += (in_addr_t) p[14] << 8;
> + inaddr += (in_addr_t) p[15];
>
> vv = (ngx_http_variable_value_t *)
> ngx_radix32tree_find(ctx->u.trees.tree, inaddr);
> @@ -272,10 +272,10 @@
> if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
> p = inaddr6->s6_addr;
>
> - inaddr = p[12] << 24;
> - inaddr += p[13] << 16;
> - inaddr += p[14] << 8;
> - inaddr += p[15];
> + inaddr = (in_addr_t) p[12] << 24;
> + inaddr += (in_addr_t) p[13] << 16;
> + inaddr += (in_addr_t) p[14] << 8;
> + inaddr += (in_addr_t) p[15];
>
> } else {
> inaddr = INADDR_NONE;
> diff -r 03e554997676 -r 5584232259d2 src/http/modules/ngx_http_geoip_module.c
> --- a/src/http/modules/ngx_http_geoip_module.c Mon Feb 26 20:00:23 2024 +0000
> +++ b/src/http/modules/ngx_http_geoip_module.c Mon Feb 26 20:00:26 2024 +0000
> @@ -266,10 +266,10 @@
> if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
> p = inaddr6->s6_addr;
>
> - inaddr = p[12] << 24;
> - inaddr += p[13] << 16;
> - inaddr += p[14] << 8;
> - inaddr += p[15];
> + inaddr = (in_addr_t) p[12] << 24;
> + inaddr += (in_addr_t) p[13] << 16;
> + inaddr += (in_addr_t) p[14] << 8;
> + inaddr += (in_addr_t) p[15];
>
> return inaddr;
> }
> diff -r 03e554997676 -r 5584232259d2 src/stream/ngx_stream_access_module.c
> --- a/src/stream/ngx_stream_access_module.c Mon Feb 26 20:00:23 2024 +0000
> +++ b/src/stream/ngx_stream_access_module.c Mon Feb 26 20:00:26 2024 +0000
> @@ -144,10 +144,10 @@
> p = sin6->sin6_addr.s6_addr;
>
> if (ascf->rules && IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) {
> - addr = p[12] << 24;
> - addr += p[13] << 16;
> - addr += p[14] << 8;
> - addr += p[15];
> + addr = (in_addr_t) p[12] << 24;
> + addr += (in_addr_t) p[13] << 16;
> + addr += (in_addr_t) p[14] << 8;
> + addr += (in_addr_t) p[15];
> return ngx_stream_access_inet(s, ascf, htonl(addr));
> }
>
> diff -r 03e554997676 -r 5584232259d2 src/stream/ngx_stream_geo_module.c
> --- a/src/stream/ngx_stream_geo_module.c Mon Feb 26 20:00:23 2024 +0000
> +++ b/src/stream/ngx_stream_geo_module.c Mon Feb 26 20:00:26 2024 +0000
> @@ -190,10 +190,10 @@
> p = inaddr6->s6_addr;
>
> if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
> - inaddr = p[12] << 24;
> - inaddr += p[13] << 16;
> - inaddr += p[14] << 8;
> - inaddr += p[15];
> + inaddr = (in_addr_t) p[12] << 24;
> + inaddr += (in_addr_t) p[13] << 16;
> + inaddr += (in_addr_t) p[14] << 8;
> + inaddr += (in_addr_t) p[15];
>
> vv = (ngx_stream_variable_value_t *)
> ngx_radix32tree_find(ctx->u.trees.tree, inaddr);
> @@ -263,10 +263,10 @@
> if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
> p = inaddr6->s6_addr;
>
> - inaddr = p[12] << 24;
> - inaddr += p[13] << 16;
> - inaddr += p[14] << 8;
> - inaddr += p[15];
> + inaddr = (in_addr_t) p[12] << 24;
> + inaddr += (in_addr_t) p[13] << 16;
> + inaddr += (in_addr_t) p[14] << 8;
> + inaddr += (in_addr_t) p[15];
>
> } else {
> inaddr = INADDR_NONE;
> diff -r 03e554997676 -r 5584232259d2 src/stream/ngx_stream_geoip_module.c
> --- a/src/stream/ngx_stream_geoip_module.c Mon Feb 26 20:00:23 2024 +0000
> +++ b/src/stream/ngx_stream_geoip_module.c Mon Feb 26 20:00:26 2024 +0000
> @@ -236,10 +236,10 @@
> if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
> p = inaddr6->s6_addr;
>
> - inaddr = p[12] << 24;
> - inaddr += p[13] << 16;
> - inaddr += p[14] << 8;
> - inaddr += p[15];
> + inaddr = (in_addr_t) p[12] << 24;
> + inaddr += (in_addr_t) p[13] << 16;
> + inaddr += (in_addr_t) p[14] << 8;
> + inaddr += (in_addr_t) p[15];
>
> return inaddr;
> }
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH] Core: fix conversion of IPv4-mapped IPv6 addresses

Piotr Sikora via nginx-devel 289 February 27, 2024 08:24PM

Re: [PATCH] Core: fix conversion of IPv4-mapped IPv6 addresses

Sergey Kandaurov 28 March 15, 2024 12:14PM

Re: [PATCH] Core: fix conversion of IPv4-mapped IPv6 addresses

Sergey Kandaurov 27 March 18, 2024 09:18AM

Re: [PATCH] Core: fix conversion of IPv4-mapped IPv6 addresses

Piotr Sikora via nginx-devel 24 March 21, 2024 02:56AM

Re: [PATCH] Core: fix conversion of IPv4-mapped IPv6 addresses

Sergey Kandaurov 44 March 27, 2024 01:52PM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 133
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready