Test to_socket_addr_str_bad is broken on macOS too

[stepancheg]

This is the test from rust source repository.

    // FIXME: figure out why this fails on openbsd and bitrig and fix it
    #[test]
    #[cfg(not(any(windows, target_os = "openbsd", target_os = "bitrig")))]
    fn to_socket_addr_str_bad() {
        assert!(tsa("1200::AB00:1234::2552:7777:1313:34300").is_err());
    }

It says, the test fails on openbsd and bitrig, but it also fails on macOS.

    let a: Vec<_> = "1200::AB00:1234::2552:7777:1313:34300"
        .to_socket_addrs().unwrap().collect();
    println!("{:?}", a); // prints [V4(92.242.132.24:34300)]

I guess the reason is this:

% host random::junk
random::junk has address 92.242.132.24
Host random::junk not found: 3(NXDOMAIN)

% dig random::junk

; <<>> DiG 9.10.6 <<>> random::junk
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 581
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 512
;; QUESTION SECTION:
;random::junk.			IN	A

;; ANSWER SECTION:
random::junk.		0	IN	A	92.242.132.24

;; Query time: 30 msec
;; SERVER: 194.168.4.100#53(194.168.4.100)
;; WHEN: Wed Sep 05 04:07:34 BST 2018
;; MSG SIZE  rcvd: 57

194.168.4.100 is an IP address of DNS of my ISP.

So for some reason, macOS decides to resolve string 1200::AB00:1234::2552:7777:1313 using DNS, and on certain ISP this resolution success.

I think rust should either:

• disable this test on macOS as well
• or explicitly check if hostname contains a colon, immediately fail without passing the string to libc resolver

[stepancheg]

CC @dhuseby @semarie

[pnkfelix]

I switched ISPs recently and I am now observing this problem on my local MacOS development box.

Needless to say, it is annoying.

I want to try to reconstruct the history of how we even got here.

• At some point, this test was added as part of PR std: Fix peeling ports from addresses #23097, which replaced a rsplitn(2, ':') call deep in the guts of some addr parsing logic with rsplitn(1, ':') in order to close issue UdpSocket::bind() accepts invalid IPv6 address #23076, with the claim that this would stop us from breaking the address into more than two parts. Its possible that this test was broken even at this early date on all platforms of interest.
• At a later date, PR std: Changing the meaning of the count to splitn #23951 changed the meaning of the count parameter to fn splitn and fn rsplitn, so that count denoted the maximum number of items returned, rather than the maximum number of splits made. At that time, all of the relevant rsplitn calls got changed back, so that they again pass 2 instead of 1 as the actual argument. The test did not need changing, since the test was written at a higher level than calls to rsplitn. (I mention this event mostly so as a reminder when traversing the history that the calls that pass 2 from this point onward are not bugs.)
• At a much later date, PR Add SGX target to std and dependencies #56066 included a commit that heavily refactored the relevant code here, so that the rsplitn calls in question moved around. But I do not think the identifiable commit (22c4368) changed the semantics of the test itself.

[pnkfelix]

Perhaps most importantly, the conversion in question does not return Err as far back as Rust 1.2.0 for me (rustup for some reason does not let me download versions 1.0.0 nor 1.1.0). I suspect this continues to be the same ISP dependent behavior.

Here is the code I was using to observe the behavior:

use std::net::{SocketAddr, ToSocketAddrs};

fn main() {
    // Convert input to `Debug` fmt'able form.
    fn tsa<A: ToSocketAddrs>(a: A) -> Result<Vec<SocketAddr>, String> {
        match a.to_socket_addrs() {
            Ok(a) => Ok(a.collect()),
            Err(e) => Err(e.to_string()),
        }
    }

    println!("{:?}", tsa("1200::AB00:1234::2552:7777:1313:34300"));
}

[pnkfelix]

The next question I have is: Why isn't this invalid input causing an error prior to the attempt to do a DNS resolve?

Interestingly, the original test case filed in issue #23076 does seem to behave as expected on my MacOS machine: an attempt to evaluate UdpSocket::bind("1200::AB00:1234::2552:7777:1313:34300") returns Err(Os { code: 49, kind: AddrNotAvailable, message: "Can\'t assign requested address" })

So even though the DNS resolve call "succeeds" by returning the IP address of the DNS server, something further along in the system still ensures that the attempt to bind a UdpSocket to the ill-formed address will still fail.

I take this to mean that we may have another choice to make with respect to the failing test here: Perhaps we should replace the existing test that has flaky behavior with a new test that better matches the original problem.

[stepancheg]

DNS resolve call "succeeds" by returning the IP address of the DNS server

Some context: my provider had a "feature": it resolved any DNS name to its own name, so opening a website with misspelt name opens a page with lots of ads.

See the dig output above: it was the DNS server who successfully resolved the name random::junk, not the macOS.

something further along in the system still ensures that the attempt to bind a UdpSocket to the ill-formed address will still fail

A user cannot bind a socket to a non-local address if I got your comment correctly, that's expected behavior.

[pnkfelix]

Yes, thank you @stepancheg for that clarifying note.

I think the choices in front of us are roughly as you stated in the description: either:

• we should disable the test (on MacOS alone, or on all targets),
  • potentially coupled with adding a different regression test,
• or we should change the library function to test that the input contains no illegal characters before passing it up to the DNS server.

What we should not do is continue to let this test flummox developers in a manner dependent on what DNS resolution service they happen to be using.