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

[Florian Weimer <fweimer@redhat.com>]

* Andreas Schwab:

> On Sep 04 2023, Florian Weimer wrote:
>
>>> @@ -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.
>
> See Appendix B, changes to the rules, "Changed the definition of
> CommonPrefixLen()".

So it's the “up to the length of S's prefix” part here:

| 2.2.  Common Prefix Length
| 
|    We define the common prefix length CommonPrefixLen(S, D) of a source
|    address S and a destination address D as the length of the longest
|    prefix (looking at the most significant, or leftmost, bits) that the
|    two addresses have in common, up to the length of S's prefix (i.e.,
|    the portion of the address not including the interface ID).  For
|    example, CommonPrefixLen(fe80::1, fe80::2) is 64.

There's an erratum for this because not all addresses have interface
IDs, so this is just confusing.

In any case, new code still does not implement the rule that prefers
longer common prefixes.  Our rule goes like this, if I understand the
code correctly:

* If one source/destination pair is on the same subnet and the other is
  not, prefer the first.

* If both source/destination pairs are on the same (but potentially
  distinct) subnets, prefer the smaller subnet, the one with the longer
  prefix.

I still think the second, length-based heuristic is dubious.  In any
case, it doesn't match what's in the RFC (and there's an old comment to
that effect).  To reiterate, I think that's fine, and we should go
further.

>>> @@ -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?
>
> That's the same consequence as above due to the changes in
> CommonPrefixLen.

But it doesn't implement the same algorithm, as far as I can tell.  I
think we should use this in both cases:

* If one source/destination pair is on the same subnet and the other is
  not, prefer the first.

Thanks,
Florian