Re: [PATCH] Update getaddrinfo to RFC 6724 (bug 29496)

[Florian Weimer <fweimer@redhat.com>]

* Andreas Schwab via Libc-alpha:

> diff --git a/posix/gai.conf b/posix/gai.conf
> index 4616ed005b..de76e2c26f 100644
> --- a/posix/gai.conf
> +++ b/posix/gai.conf

> @@ -16,40 +16,34 @@
>  #    used.  There are possible runtime problems.  The default is no.
>  #
>  # label   <mask>   <value>
> +#    Add another rule to the RFC 6724 label table.  See section 2.1 in
> +#    RFC 6724.  The default is:
>  #
>  #label ::1/128       0
>  #label ::/0          1
>  #label 2002::/16     2
>  #label ::/96         3
>  #label ::ffff:0:0/96 4
> +#label 2001::/32     5
> +#label fec0::/10     11
> +#label 3ffe::/16     12
> +#label fc00::/7      13

Change matches new table in section 2.1.

> -#    For sites which prefer IPv4 connections change the last line to
> +#    Add another rule to the RFC 6724 precedence table.  See section 2.1
> +#    and 10.3 in RFC 6724.  The default is:
> +#
> +#precedence ::1/128        50
> +#precedence ::/0           40
> +#precedence ::ffff:0:0/96  35
> +#precedence 2002::/16      30
> +#precedence 2001::/32      5
> +#precedence fc00::/7       3
> +#precedence ::/96          1
> +#precedence fec0::/10      1
> +#precedence 3ffe::/16      1

Likewise.

> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index bcff909b2f..3da296816a 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -1304,7 +1304,7 @@ static const struct prefixentry *labels;
>  /* Default labels.  */
>  static const struct prefixentry default_labels[] =
>    {
> -    /* See RFC 3484 for the details.  */
> +    /* See RFC 6724 for the details.  */
>      { { .__in6_u
>  	= { .__u6_addr8 = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>  			    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 } }

> @@ -1354,23 +1350,39 @@ static const struct prefixentry *precedence;
>  /* The default precedences.  */
>  static const struct prefixentry default_precedence[] =

Table updates match the RFC.

> @@ -1453,7 +1465,9 @@ fls (uint32_t a)
>  {
>    uint32_t mask;
>    int n;
> -  for (n = 0, mask = 1 << 31; n < 32; mask >>= 1, ++n)
> +  if (a == 0)
> +    return 0;
> +  for (n = 0, mask = 1u << 31; n < 32; mask >>= 1, ++n)
>      if ((a & mask) != 0)
>        break;
>    return n;
> @@ -1461,7 +1475,7 @@ fls (uint32_t a)
>  
>  
>  static int
> -rfc3484_sort (const void *p1, const void *p2, void *arg)
> +rfc6724_sort (const void *p1, const void *p2, void *arg)
>  {
>    const size_t idx1 = *(const size_t *) p1;
>    const size_t idx2 = *(const size_t *) p2;
> @@ -1642,7 +1656,7 @@ rfc3484_sort (const void *p1, const void *p2, void *arg)
>  	  in_addr_t netmask1 = 0xffffffffu << (32 - a1->prefixlen);
>  
>  	  if ((in1_src_addr & netmask1) == (in1_dst_addr & netmask1))
> -	    bit1 = fls (in1_dst_addr ^ in1_src_addr);
> +	    bit1 = a1->prefixlen;
>  
>  	  struct sockaddr_in *in2_dst
>  	    = (struct sockaddr_in *) a2->dest_addr->ai_addr;
> @@ -1653,7 +1667,7 @@ rfc3484_sort (const void *p1, const void *p2, void *arg)
>  	  in_addr_t netmask2 = 0xffffffffu << (32 - a2->prefixlen);
>  
>  	  if ((in2_src_addr & netmask2) == (in2_dst_addr & netmask2))
> -	    bit2 = fls (in2_dst_addr ^ in2_src_addr);
> +	    bit2 = a2->prefixlen;
>  	}

This is an undocumented change that I can't find in RFC 6724.  I have a
general distaste for Rule 9, so anything that treats more addresses as
equivalent is fine with me.  However, it is not clear to me why we would
prefer subnets with longer prefixes.  Setting bit1 and bit2 to 1 if the
source and destination address are on the same subnet should achieve the
same thing.

This does not conform to RFC 6724, so it needs a comment, and probably a
NEWS entry as well.

>        else if (a1->dest_addr->ai_family == PF_INET6)
>  	{
> @@ -1672,18 +1686,33 @@ rfc3484_sort (const void *p1, const void *p2, void *arg)
>  
>  	  int i;
>  	  for (i = 0; i < 4; ++i)
>  	    {
> +	      uint32_t mask1, mask2;
> +	      if (i * 32 >= a1->prefixlen)
> +		mask1 = 0;
> +	      else if ((i + 1) * 32 > a1->prefixlen)
> +		mask1 = 0xffffffffu << (32 - (a1->prefixlen & 31));
> +	      else
> +		mask1 = 0xffffffffu;
> +	      if (i * 32 >= a2->prefixlen)
> +		mask2 = 0;
> +	      else if ((i + 1) * 32 > a2->prefixlen)
> +		mask2 = 0xffffffffu << (32 - (a2->prefixlen & 31));
> +	      else
> +		mask2 = 0xffffffffu;
> +	      if (((in1_dst->sin6_addr.s6_addr32[i]
> +		    ^ in1_src->sin6_addr.s6_addr32[i]) & htonl (mask1)) != 0
> +		  || ((in2_dst->sin6_addr.s6_addr32[i]
> +		       ^ in2_src->sin6_addr.s6_addr32[i]) & htonl (mask2)) != 0)
> +		{
> +		  bit1 = fls (ntohl (in1_dst->sin6_addr.s6_addr32[i]
> +				     ^ in1_src->sin6_addr.s6_addr32[i])
> +			      & mask1);
> +		  bit2 = fls (ntohl (in2_dst->sin6_addr.s6_addr32[i]
> +				     ^ in2_src->sin6_addr.s6_addr32[i])
> +			      & mask2);
> +		  break;
> +		}

Why use the common prefix length for IPv6 addresses?  RFC 6724 pretends
that IPv6 assignment is strictly hierarchical, but in reality, it's
not.  Shouldn't we do the same thing as for IPv4 addresses?

It's not clear to me why you changed the implementation.  Does it fix a
bug?  

Thanks,
Florian