Change Ipv6Addr::is_loopback to include IPv4-mapped loopback addresses

[CDirkx]

This PR addresses one of the issues that came up during the larger efforts of trying to stabilize the ip feature, see also the most recent stabilization PR #85585 and issue #85609 where I have tried to summarize all the relevant discussion so far.

Fixes #69772

Edit: for an alternative to this PR, see #86335 which commits to not supporting IPv4-in-IPv6 addresses (so leaving is_loopback unchanged).

Overview

There are two ways of representing an IPv4 address in a IPv6 address:

• As an IPv4-compatible IPv6 address: ::1.2.3.4
• As an IPv4-mapped IPv6 address: ::ffff:1.2.3.4

#69772 raised the issue that other languages take IPv4-mapped addresses into account in their equivalent of is_loopback, for example ::ffff:127.0.0.1. This raised the broader question about which (if any) methods of Ipv6Addr should consider IPv4-mapped or IPv4-compatible addresses.

While we don't yet have a clear answer in general, for is_loopback is seems pretty clear that supporting addresses like ::ffff:127.0.0.1 is a good idea: .NET, Java, Python and Go all follow this behaviour (#69772, #76098 (comment)). This behaviour is also implemented by many real world networking hardware/software (try ping ::ffff::127.0.0.1). In short, it is surprising for users that the Rust implementation is different. Regarding IPv4-compatible addresses, those are deprecated and of the tested other languages none took them into account (try also ping ::127.0.0.1).

Implementation

This PR changes the behaviour of is_loopback from

/// Returns [`true`] if this is a loopback address (::1).

to

/// Returns [`true`] if this is either:
/// - the [loopback address] as defined in [IETF RFC 4291 section 2.5.3] (`::1`).
/// - an [IPv4-mapped] address representing an IPv4 loopback address as defined in [IETF RFC 1122] (`::ffff:127.0.0.0/104`).
///
/// Note that this does not return [`true`] for an [IPv4-compatible] address representing an IPv4 loopback address (`::127.0.0.0/104`).

The documentation of Ipv6Addr is expanded to include a section explaining IPv4-compatible and IPv4-mapped addresses:

• IPv4-compatible addresses are called out as officially deprecated and that only conversion to and from such an address is supported (to_ipv6_compatible, to_ipv4). It is also noted that no special meaning is ascribed to these addresses, so the embedded IPv4 address will never be taken into account.
• IPv4-mapped addresses are explained and it is noted that the embedded IPv4 addresses may be taken into account, is_loopback is given as an example. is_loopback is specifically mentioned as currently the only method that takes IPv4-mapped addresses into account, and a small explanation is included that this is in line with other languages and real-world hardware.

The documentation of the methods to_ipv6_compatible, to_ipv6_mapped, to_ipv4 and to_ipv4_mapped is updated to refer to this section.

(Also included are some small improvements to the style consistency of the documentation (pulled out into #85676) and readability of some examples)

Unresolved Questions

Stability

This PR changes the behaviour of the stable is_loopback from a simple check for equality to Ipv6Addr::LOCALHOST to something more complex, although arguable the change makes the behaviour more in line in what users would expect. To what extent is such a change in behaviour covered by stability guarantees; is just a change in documentation enough or should it be called out in compatibility notes?

Making is_loopback special

It is expected that in the future some more methods of Ipv6Addr will take IPv4-mapped addresses into account, but this is currently blocked on nailing down the exact semantics (#85609). This PR is written to be largely agnostic to such further changes, is_loopback is called out as currently the only method that has this behaviour. However, if eventually the decision is made to not apply this behaviour to other methods, is it worth it to have only is_loopback have this special behaviour? Do the arguments in favour of supporting IPv4-mapped addresses pull their weight if only is_loopback will ever support it.

[CDirkx]

Regarding making is_loopback special, I believe it is worth it to make this change, even if none of the other methods will ever take IPv4-mapped addresses into account. Most other languages don't offer as many utility methods other than is_localhost anyway, so for those Rust is pretty much free to decide what semantics it wants to offer. However for is_localhost every other major language we checked has the proposed behaviour, it seems inconsistent with real-world usage to not also implement this behaviour.

[CDirkx]

Regarding stability, would this PR require an FCP?

[CDirkx]

Ipv6Addr::to_ipv4_mapped is unstable and unstable const.

[CDirkx]

Not changed in this PR, but historical background for anyone wondering why the comparison is written like this:

• self == Ipv6Addr::LOCALHOST relies on Eq/PartialEq, which is not possible in a const function.
• self.octets() == Ipv6Addr::LOCALHOST.octets() leads to worse code generation, a problem with comparing arrays.

[joshtriplett]

@CDirkx

self.octets() == Ipv6Addr::LOCALHOST.octets() leads to worse code generation, a problem with comparing arrays.

#85828 might fix that.

But also, does u128::from(self) generate reasonable code?

[the8472]

Does this actually work with socket APIs?

E.g. is this guaranteed to connect even if one is the ipv4-mapped and the other is ipv6?

assert!(src.is_loopback() && dst.is_loopback());
socket.bind(src);
socket.connect(dst);

[CDirkx]

Mmm good question, that is a point that I have not seen before in this discussion. The answer is no, at least on my system (Windows) binding and connecting to mixed addresses give various errors. I can not even bind to ::ffff:127.0.0.1: "The requested address is not valid in its context."

The original issue mentions:

Once I bind my server to [::] to work on IPv4 AND IPv6 at the same time and then connect to it via v4 to 127.0.0.1 the is_loopback call returns false.

Only when handling both IPv4 and IPv6 addresses (and thus IPv4 addresses getting mapped to IPv6 addresses) are IPv4-mapped addresses expected to be taken into account. Maybe this is related to the fact that all of the other tested languages do not have separate types representing IPv4 or IPv6 addresses, like Rust has Ipv4Addr and Ipv6Addr. So the fact that those other languages all have a certain behaviour might have less relevance for Ipv6Addr, and makes me less certain that this PR is the right approach.

Not sure what to do about this, we could leave Ipv6Addr::is_loopback as is and explicitly document that neither IPv4-mapped and IPv4-compatible addresses are taken into account, along with an example of how to do this yourself with to_ipv4_mapped (which is currently unstable). Another option would be to instead implement this behaviour in IpAddr::is_loopback, as IpAddr kind-of represents handling "both IPv4 and IPv6" addresses. However then it could be confusing that IpAddr doesn't strictly delegate to either Ipv4Addr or Ipv6Addr.

[the8472]

Maybe this is related to the fact that all of the other tested languages do not have separate types representing IPv4 or IPv6 addresses

Java does. And Inet6Address::isLoopbackAddress only checks for IPv6 loopback addresses, not ipv4-mapped ones.

https://github.com/openjdk/jdk/blob/0b7735938407fad5c2dbfb509d2d47bf172305e9/src/java.base/share/classes/java/net/Inet6Address.java#L318-L324

But all the public parser methods try to automatically parse v4-mapped ones to Inet4Address. If I recall correctly you have to do some gymnastics to create an Inet6Address holding a v4-mapped address.

[CDirkx]

Oh you're right, I forgot about Inet6Address.

It appears that in that case OpenJDK and Oracle Java have different behaviour: in Oracle Java both InetAddress and Inet6Address consider ::ffff:127.0.0.1 a loopback address (https://ideone.com/yAWa8b).

[the8472]

You're using getByName there which is implemented by the superclass InetAddress and has a polymorphic return type, calling it on Inet6Address still invokes the superclass method. So you're actually getting an Inet4Address there on each call.

Afaik the only way to actually construct an ipv4-mapped one is to use Inet6Address::getByAddress and pass it as byte array.

[joshtriplett]

This seems entirely reasonable to me, and has plenty of precedent from the standard libraries of other languages.

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @sfackler
• @yaahc

Concerns:

• expected-behavior (Change Ipv6Addr::is_loopback to include IPv4-mapped loopback addresses #85655 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[the8472]

No concerns currently listed.

I think there still are some unsettled questions in #85655 (comment) and #85655 (comment)

Imo it would be cleaner to have a method like to_canonical(self) -> IpAddr which turns an ipv4-mapped ipv6 address into IpAddr::V4 and all other ip6 addresses into IpAddr::V6 and then add IpAddr::is_localhost() which delegates to the inner.

This way we would have version-specific methods that don't have to know about the other version and the enum that delegates to the version-specific logic.

[joshtriplett]

@the8472 I agree that that needs consideration. It's not obvious if all addresses that return true from .is_loopback() need to be routable, or if they should just semantically be loopback addresses. I can see an argument for either.

On the one hand, we could keep IPv6 and IPv4 addresses completely separate and people can always check for v4mapped addresses and convert/check those in their own code. On the other hand, we could make methods on IPv6 addresses check for the corresponding property on v4mapped addresses they're holding, and if someone wants to know that they don't have a v4mapped address they can check that. Either is justifiable; the question is which is more useful.

[the8472]

can always check for v4mapped addresses and convert/check those in their own code

Yeah, but currently that's slightly inconvenient or noisy since you have to go through to_ipv4_mapped and do some extra matching, which is why I suggested adding to_canonical(). With that it would be as simple as

• version-agnostic check: addr6.to_canonical().is_loopback()
• version-specific check: addr6.is_loopback()

[joshtriplett]

@rfcbot concern expected-behavior

Registering a concern to evaluate whether this is the expected behavior and semantics, or whether a different behavior would work better in practice.

[CDirkx]

The comment about Java's behaviour prompted me to look a bit deeper into Java and the other languages.

About IPv4-mapped addresses the Java docs state "Java will never return an IPv4-mapped address. These classes can take an IPv4-mapped address as input, both in byte array and text representation. However, it will be converted into an IPv4 address." So an Inet6Address representing ::ffff:127.0.0.1 is actually not constructable, it will always be converted to an Inet4Address. So in Java isLoopbackAddress on an Inet6Address is only true for ::1.

In Python there is also the different types IPv4Address and IPv6Address, and is_loopback on an IPv6Address is also only true for ::1: https://github.com/python/cpython/blob/46b16d0bdbb1722daed10389e27226a2370f1635/Lib/ipaddress.py#L2033-L2041

So my previous statement that all other tested languages implement this behaviour was wrong, I'm sorry for not being more careful there. What appears to be the case instead is that languages that don't have separate types for IPv4 and IPv6 addresses consider ::ffff:127.0.0.1 a loopback address (Go, .NET) and languages that have a separate type for IPv6 addresses do not (Java, Python).

In that case it would seem more consistent for Rust (which has separate Ipv4Addr and Ipv6Addr) to not accept this PR and keep is_loopback a simple check against Ipv6Addr::LOCALHOST (::1). That would mean we can also stick closer to just the specification, which mentions ::1 as the only IPv6 loopback address and doesn't describe any special behaviour for IPv4-mapped addresses.

[bors]

☔ The latest upstream changes (presumably #86160) made this pull request unmergeable. Please resolve the merge conflicts.

[yaahc]

In that case it would seem more consistent for Rust (which has separate Ipv4Addr and Ipv6Addr) to not accept this PR and keep is_loopback a simple check against Ipv6Addr::LOCALHOST (::1). That would mean we can also stick closer to just the specification, which mentions ::1 as the only IPv6 loopback address and doesn't describe any special behaviour for IPv4-mapped addresses.

This seems compelling to me, though sad given how much care you've clearly put into this PR. Would you like me to go ahead and FCP close this or did you want to close it yourself?

[CDirkx]

I can open up another PR that also documents IPv4-mapped and IPv4-compatible addresses and explicitly states that Rust will not assign any special meaning to those addresses, so IPv4-in-IPv6 are not directly supported. Even though that would only be a change in documentation, a FCP on that PR could finally settle this question, so it also doesn't need to be discussed again for other methods like is_documentation and is_global.

[CDirkx]

I have opened #86335 as an alternative to this PR, which commits to not supporting IPv4-in-IPv6 addresses (so leaving is_loopback unchanged).

[CDirkx]

One additional reason I thought of against this and in favour of #86335: If we were to unconditionally support IPv4-in-IPv6 addresses, I don't see any way to opt-out and get back the current behaviour. However with #86335 it will still be possible to explicitly opt-in to supporting IPv4-in-IPv6 addresses using to_ipv4_mapped or other convenience methods, and thus able to accommodate strictly more use cases.

[joshtriplett]

@rfcbot cancel

Closing this in favor of #86335.

[rfcbot]

@joshtriplett proposal cancelled.

[yaahc]

Checking off sfackler's checkbox since he's left the libs team.