On Fri, Mar 15, 2024 at 08:12:50PM +0400, Sergey Kandaurov wrote:
>
> > 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];
> >
> > [...]
The patch.
# HG changeset patch
# User Sergey Kandaurov <pluk...@nginx.com>
# Date 1710767670 -14400
# Mon Mar 18 17:14:30 2024 +0400
# Node ID 543ca08e56fbd28f82552564d70edb74d0e628a7
# Parent 89bff782528a91ad123b63b624f798e6fd9c8e68
Core: fixed undefined behaviour with IPv4-mapped IPv6 addresses.
When converting to IPv4 addresses, with implicit integer promotion to hold
the intermediate value, the most significant bit appeared on the sign bit
after left shifting by 24 places, which is undefined behaviour.
In practice, a subsequent store into the left value resulted in the same
behaviour as if there would be a proper cast, at least on known compilers,
such as GCC and Clang, because in_addr_t has the same type width as the
intermediate after integer promotion (POSIX states in_addr_t is equivalent
to uint32_t, which is the same type as "unsigned int" in ILP32 and LP64 data
type models). The reason is that they keep intermediate result in a wide
enough register such that its value is not truncated before storing it in
the left value. This makes to produce object file that don't change after
adding an explicit cast; comparing md5sum over them matches. Still, it is
useful to avoid undefined behaviour and to suppress such reports.
Found with UndefinedBehaviorSanitizer.
Spotted by: Piotr Sikora
diff --git a/src/core/ngx_inet.c b/src/core/ngx_inet.c
--- a/src/core/ngx_inet.c
+++ b/src/core/ngx_inet.c
@@ -507,7 +507,7 @@ ngx_cidr_match(struct sockaddr *sa, ngx_
p = inaddr6->s6_addr;
- inaddr = p[12] << 24;
+ inaddr = (in_addr_t) p[12] << 24;
inaddr += p[13] << 16;
inaddr += p[14] << 8;
inaddr += p[15];
diff --git a/src/http/modules/ngx_http_access_module.c
b/src/http/modules/ngx_http_access_module.c
--- a/src/http/modules/ngx_http_access_module.c
+++ b/src/http/modules/ngx_http_access_module.c
@@ -148,7 +148,7 @@ ngx_http_access_handler(ngx_http_request
p = sin6->sin6_addr.s6_addr;
if (alcf->rules && IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) {
- addr = p[12] << 24;
+ addr = (in_addr_t) p[12] << 24;
addr += p[13] << 16;
addr += p[14] << 8;
addr += p[15];
diff --git a/src/http/modules/ngx_http_geo_module.c
b/src/http/modules/ngx_http_geo_module.c
--- a/src/http/modules/ngx_http_geo_module.c
+++ b/src/http/modules/ngx_http_geo_module.c
@@ -199,7 +199,7 @@ ngx_http_geo_cidr_variable(ngx_http_requ
p = inaddr6->s6_addr;
if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
- inaddr = p[12] << 24;
+ inaddr = (in_addr_t) p[12] << 24;
inaddr += p[13] << 16;
inaddr += p[14] << 8;
inaddr += p[15];
@@ -272,7 +272,7 @@ ngx_http_geo_range_variable(ngx_http_req
if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
p = inaddr6->s6_addr;
- inaddr = p[12] << 24;
+ inaddr = (in_addr_t) p[12] << 24;
inaddr += p[13] << 16;
inaddr += p[14] << 8;
inaddr += p[15];
diff --git a/src/http/modules/ngx_http_geoip_module.c
b/src/http/modules/ngx_http_geoip_module.c
--- a/src/http/modules/ngx_http_geoip_module.c
+++ b/src/http/modules/ngx_http_geoip_module.c
@@ -266,7 +266,7 @@ ngx_http_geoip_addr(ngx_http_request_t *
if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
p = inaddr6->s6_addr;
- inaddr = p[12] << 24;
+ inaddr = (in_addr_t) p[12] << 24;
inaddr += p[13] << 16;
inaddr += p[14] << 8;
inaddr += p[15];
diff --git a/src/stream/ngx_stream_access_module.c
b/src/stream/ngx_stream_access_module.c
--- a/src/stream/ngx_stream_access_module.c
+++ b/src/stream/ngx_stream_access_module.c
@@ -144,7 +144,7 @@ ngx_stream_access_handler(ngx_stream_ses
p = sin6->sin6_addr.s6_addr;
if (ascf->rules && IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) {
- addr = p[12] << 24;
+ addr = (in_addr_t) p[12] << 24;
addr += p[13] << 16;
addr += p[14] << 8;
addr += p[15];
diff --git a/src/stream/ngx_stream_geo_module.c
b/src/stream/ngx_stream_geo_module.c
--- a/src/stream/ngx_stream_geo_module.c
+++ b/src/stream/ngx_stream_geo_module.c
@@ -190,7 +190,7 @@ ngx_stream_geo_cidr_variable(ngx_stream_
p = inaddr6->s6_addr;
if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
- inaddr = p[12] << 24;
+ inaddr = (in_addr_t) p[12] << 24;
inaddr += p[13] << 16;
inaddr += p[14] << 8;
inaddr += p[15];
@@ -263,7 +263,7 @@ ngx_stream_geo_range_variable(ngx_stream
if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
p = inaddr6->s6_addr;
- inaddr = p[12] << 24;
+ inaddr = (in_addr_t) p[12] << 24;
inaddr += p[13] << 16;
inaddr += p[14] << 8;
inaddr += p[15];
diff --git a/src/stream/ngx_stream_geoip_module.c
b/src/stream/ngx_stream_geoip_module.c
--- a/src/stream/ngx_stream_geoip_module.c
+++ b/src/stream/ngx_stream_geoip_module.c
@@ -236,7 +236,7 @@ ngx_stream_geoip_addr(ngx_stream_session
if (IN6_IS_ADDR_V4MAPPED(inaddr6)) {
p = inaddr6->s6_addr;
- inaddr = p[12] << 24;
+ inaddr = (in_addr_t) p[12] << 24;
inaddr += p[13] << 16;
inaddr += p[14] << 8;
inaddr += p[15];
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
