> On 28 Feb 2024, at 05:21, Piotr Sikora via nginx-devel
> <nginx-devel@nginx.org> wrote:
>
> # HG changeset patch
> # User Piotr Sikora <pi...@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 <pi...@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
