Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] QUIC: better sockaddr initialization

Alejandro Colomar
May 21, 2023 05:32AM
Hi Maxim,

On 5/21/23 03:42, Maxim Dounin wrote:
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1684633125 -10800
> # Sun May 21 04:38:45 2023 +0300
> # Node ID 68fa4b86ed46138dd1a8fcf2cfd80206de068bec
> # Parent 235d482ef6bc8c40a956b2413865d42c94e0fc05
> QUIC: better sockaddr initialization.
>
> The qsock->sockaddr field is a ngx_sockaddr_t union, and therefore can hold
> any sockaddr (and union members, such qsock->sockaddr.sockaddr, can be used
> to access appropriate variant of the sockaddr). It is better to set it via
> qsock->sockaddr itself though, and not qsock->sockaddr.sockaddr, so static
> analyzers won't complain about out-of-bounds access.

Correct. The previous code was UB, due to memcpy(3) writing to the
'struct sockaddr' member. By writing to sockaddr, you were only
allowed to alias via other members the sa_family_t field, but no
others.

Only if you had written to a 'struct sockaddr_storage' (which nginx
doesn't use in the union), you'd have guarantees that it's storage is
accessible via any other sockaddr_* structures. This broke with
aliasing rules, but the wording in POSIX has been improved for the
upcoming Issue 8 to allow this.

See:
https://austingroupbugs.net/view.php?id=1641
https://lore.kernel.org/linux-man/20230330171310.12330-1-alx@kernel.org/T
https://lore.kernel.org/linux-man/20230421202718.21831-1-alx@kernel.org/T

So, for doing things right, when multiple sockaddr_* structures are
involved, you need to write to a sockaddr_storage, or write to a union
containing all of the structures you're interested in. Otherwise,
the only field that you can safely alias from any other sockaddr_*
is the sa_family_t one, and aliasing anything else is UB.


>
> Prodded by Coverity (CID 1530403).
>
> diff --git a/src/event/quic/ngx_event_quic_udp.c b/src/event/quic/ngx_event_quic_udp.c
> --- a/src/event/quic/ngx_event_quic_udp.c
> +++ b/src/event/quic/ngx_event_quic_udp.c
> @@ -183,7 +183,7 @@ ngx_quic_recvmsg(ngx_event_t *ev)
>
> qsock = ngx_quic_get_socket(c);
>
> - ngx_memcpy(&qsock->sockaddr.sockaddr, sockaddr, socklen);
> + ngx_memcpy(&qsock->sockaddr, sockaddr, socklen);

Yes, since you write to the union, you can later use any of the
members.

Reviewed-by: Alejandro Colomar <alx@nginx.com>

Cheers,
Alex

> qsock->socklen = socklen;
>
> c->udp->buffer = &buf;
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
http://www.alejandro-colomar.es/
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH] QUIC: better sockaddr initialization

Maxim Dounin 293 May 20, 2023 09:44PM

Re: [PATCH] QUIC: better sockaddr initialization

Roman Arutyunyan 87 May 21, 2023 05:08AM

Re: [PATCH] QUIC: better sockaddr initialization

Maxim Dounin 91 May 21, 2023 08:58AM

Re: [PATCH] QUIC: better sockaddr initialization

Alejandro Colomar 108 May 21, 2023 05:32AM

Re: [PATCH] QUIC: better sockaddr initialization

Maxim Dounin 97 May 21, 2023 09:12AM

Re: [PATCH] QUIC: better sockaddr initialization

Alejandro Colomar 86 May 21, 2023 10:36AM

Re: [PATCH] QUIC: better sockaddr initialization

Maxim Dounin 100 May 21, 2023 05:24PM

Re: [PATCH] QUIC: better sockaddr initialization

Alejandro Colomar 126 May 21, 2023 07:08PM

Re: [PATCH] QUIC: better sockaddr initialization

Maxim Dounin 97 May 21, 2023 10:36PM

memcpy(3), strict aliasing, pointer provenance rules (was: [PATCH] QUIC: better sockaddr initialization)

Alejandro Colomar 135 May 22, 2023 10:26AM

Re: memcpy(3), strict aliasing, pointer provenance rules (was: [PATCH] QUIC: better sockaddr initialization)

Maxim Dounin 143 May 22, 2023 04:06PM



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

Online Users

Guests: 339
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