Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: net: change Dial to accept host+":"+port #44955

Closed
rsc opened this issue Mar 12, 2021 · 7 comments
Closed

proposal: net: change Dial to accept host+":"+port #44955

rsc opened this issue Mar 12, 2021 · 7 comments
Labels
Projects
Milestone

Comments

@rsc
Copy link
Contributor

@rsc rsc commented Mar 12, 2021

#28308 suggests having vet report mistakes of the forms:

net.Dial("tcp", host+":"+port)
net.Dial("tcp", fmt.Sprintf("%s:%d", host, port))

The mistake being that if host is an IPv6 address then the address will be malformed.
The argument is supposed to be "[" + ipv6 + "]:" + port in that case,
and you should use

net.Dial("tcp", net.JoinHostPort(host, port))

instead.

SplitHostPort and JoinHostPort are important to use for URLs:
http://::1:80/ and http://::1/ are invalid, while http://[::1]:80/ and http://[::1]/ are fine.
(SplitHostPort and JoinHostPort only deal with the first of each pair.)

If you don't know whether a port is present, then the brackets resolve the ambiguity:
::1:80 is an IPv6 address, and [::1]:80 is an IPv6 address followed by a port.

But Dial with network "tcp" and "udp" does not have that "is the port present?" ambiguity: the port is required.
The port also never contains a colon.
So the final colon in the string is unambiguously the separator between host and port.

I wonder if we should make Dial with network "tcp" and "udp" accept host+":"+port.
Then all the things we're talking about flagging in #28308 would become correct instead.

The only downside is that if you mistakenly did

net.Dial("tcp", host)

(oops you forgot the port), then under certain rare circumstances that might not turn into a "invalid address error".
The vast majority of cases would behave as before:

  • 👍 domain name like google.com → error as before
  • 👍 ipv4 address like 192.168.0.1 → error as before
  • 👍 ipv6 address like ::1 → error as before
  • 👍 ipv6 address like 2001:db8::1 → error as before
  • 👍 ipv6 address like 2001:db8::5a95 → error as before
  • 👍 ipv6 address like 2001:db8::7226 → error as before
  • 👍 ipv6 address like fe80::69aa:dcc:67ff:5a95 → error as before
  • ✘ ipv6 address like fe80::e5e7:65b1:ff8a:7226 → dials [fe80::e5e7:65b1:ff8a]:7226

The only case that changes is when the IPv6 address ends in two or more non-wildcard chunks and the final chunk is entirely decimal digits. For the typical 16-bit chunks, that's only 15% of such addresses.

And of course if you do write this code, you're very likely to hit one of the error cases, and as soon as you do, you notice and fix the code.

On the other hand, consider

net.Dial("tcp", host+":"+port)

This works for domain names and IPv4 addresses and then breaks as soon as someone tries an IPv6 host.
We can make this work 100% of the time instead.

Overall, the cost of redefining that fairly unlikely mistake to be a different failure seems a small price to pay for fixing all the code that is using strings to put together Dial arguments and will fall over at the first IPv6 address literal.

Should we just fix all this code?

@josharian
Copy link
Contributor

@josharian josharian commented Mar 12, 2021

This sounds great.

One downside is that the failure mode is worse. Before you get an error, which (I hope) the code handles, and which vet can flag. After you dial an unexpected server, which could (in theory) be a security problem.

@stapelberg
Copy link
Contributor

@stapelberg stapelberg commented Mar 12, 2021

Thanks for filing this.

Because I can’t see it 100% clearly spelled out, let me ask directly: the proposal is to make not only net.Dial("tcp", "[fe80::e5e7:65b1:ff8a:7226]:80") work, but to also make net.Dial("tcp", "fe80::e5e7:65b1:ff8a:7226:80") work, correct?

The upside of fixing existing code is great, but I’m wondering if making Go programs accept address formats that are not accepted in other programs is a good idea? Addresses are frequently accepted via flags, so end users (not even programmers) come into contact with them.

@rsc
Copy link
Contributor Author

@rsc rsc commented Mar 12, 2021

@stapelberg, yes Dial would take both forms.

I'm not sure I understand the downside of fixing shell scripts that use $host:$port at the same time we fix Go programs that use host+":"+port.

@stapelberg
Copy link
Contributor

@stapelberg stapelberg commented Mar 15, 2021

I'm not sure I understand the downside of fixing shell scripts that use $host:$port at the same time we fix Go programs that use host+":"+port.

Fixing shell scripts as well is fine.

What I was trying to get at is that introducing a new accepted form of addresses means that people might start using them and later on end up confused when the address form they learnt with <some-go-program> doesn’t work in <any-non-go-program>. Sure, Go was just trying to be helpful, but in the end, users might need to invest troubleshooting time because of Go being different.

Perhaps other languages/libraries will follow suit, but likely many won’t.

I’m not saying this is a reason for not doing it, but we should be aware that we’re introducing a Go-ism here. Probably not a big deal given the rarity of IPv6 literal addresses.

@rsc rsc moved this from Incoming to Active in Proposals Mar 24, 2021
@ainar-g
Copy link
Contributor

@ainar-g ainar-g commented Mar 24, 2021

I am very much in favour of the original solution, issue 28308.

Not only will the proposal in the OP increase the complexity and error-prone-ness of net.Dial itself, but it could also trigger a domino effect across the ecosystem. For example, should a net.Resolver.Dial callback also accept the new “barely-correct” form? Should net/http.DialContext? That just seems like a very bad idea, especially when considering that the alternative, the original proposal, actually encourages people to write more correct code.

@rsc
Copy link
Contributor Author

@rsc rsc commented Mar 31, 2021

The overall reaction here is not positive enough to seem like we should move forward. Retracting.

@rsc rsc closed this Mar 31, 2021
@rsc rsc moved this from Active to Declined in Proposals Apr 1, 2021
@rsc
Copy link
Contributor Author

@rsc rsc commented Apr 1, 2021

No change in consensus, so declined.
— rsc for the proposal review group

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Declined
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants