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

cmd/vet: flag using %s:%d to construct network addresses #28308

Open
stapelberg opened this issue Oct 22, 2018 · 13 comments
Open

cmd/vet: flag using %s:%d to construct network addresses #28308

stapelberg opened this issue Oct 22, 2018 · 13 comments

Comments

@stapelberg
Copy link
Contributor

@stapelberg stapelberg commented Oct 22, 2018

I recently diagnosed a bug in someone’s Go program where a user reported that they couldn’t get the program to connect, and it turned out the issue was that the programmer used fmt.Sprintf("%s:%d", host, port) instead of net.JoinHostPort to construct a network address to pass to net.Dial: https://twitter.com/zekjur/status/1049642773278650368. The issue is subtle: net.JoinHostPort correctly handles IPv6 address literals (e.g. 2001:4860:4860::8888), whereas fmt.Sprintf doesn’t.

A very similar issue is to use strings.Split instead of net.SplitHostPort.

I noticed that both of these issues are fairly prevalent, likely because programmers aren’t that accustomed to using IPv6 literals yet, but I expect them to become more common as IPv6 adoption continues to grow.

I propose adding a vet check to flag using %s:%d format strings with arguments whose names contain port, addr, host, listen or bind. This heuristic will flag 12356 occurrences¹ I found on GitHub using BigQuery, and hopefully make programmers aware not only of net.JoinHostPort but also net.SplitHostPort, for which writing a check is significantly harder.

I can send a CL to implement this. Let me know what you think.

① The BigQuery query I used was:

WITH lines AS
  (SELECT
    id,
    SPLIT(content, "\n") AS line
  FROM `gocontents.gocontents`)
SELECT path, flattened_line
FROM lines
CROSS JOIN UNNEST(lines.line) AS flattened_line
JOIN `gocontents.gofiles` AS files ON files.id = lines.id
WHERE
  REGEXP_CONTAINS(flattened_line, r"fmt.Sprintf\(\"%s:%d\", (port|addr|host|listen|bind)")
@mvdan
Copy link
Member

@mvdan mvdan commented Oct 23, 2018

This definitely checks the correctness and frequency requirements for a vet check - the issue leads to bugs, and it happens often.

However, I'm not sure if it ticks off the precision requirement. If the check simply inspects the syntax tree, it could be prone to false positives or negatives. Dominik proposes a slightly different approach for his tooling in dominikh/go-tools#358, for example.

@bcmills bcmills added this to the Unplanned milestone Oct 23, 2018
lisa added a commit to openshift/managed-cluster-validating-webhooks that referenced this issue Aug 21, 2020
tklauser added a commit to cilium/cilium that referenced this issue Aug 26, 2020
Using fmt.Sprintf("%s:%d", host, port) or similar to construct network
address strings can to problems when dealing with IPv6 addresses.
However, net.JoinHostPort formats IPv6 in the correct [host]:port
notation, for example [fe80::a46c:bcff:feed:a481]:8080

For consistency, also use the same function in places where it's obvious
we're dealing with IPv4 addresses.

Also see golang/go#28308

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
tklauser added a commit to cilium/cilium that referenced this issue Aug 26, 2020
Using fmt.Sprintf("%s:%d", host, port) or similar to construct network
address strings can to problems when dealing with IPv6 addresses.
However, net.JoinHostPort formats IPv6 in the correct [host]:port
notation, for example [fe80::a46c:bcff:feed:a481]:8080

For consistency, also use the same function in places where it's obvious
we're dealing with IPv4 addresses.

Also see golang/go#28308

Found using semgrep using the hostport.yml rule from
https://github.com/dgryski/semgrep-go

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
kkourt added a commit to cilium/cilium that referenced this issue Sep 4, 2020
Using fmt.Sprintf("%s:%d", host, port) or similar to construct network
address strings can to problems when dealing with IPv6 addresses.
However, net.JoinHostPort formats IPv6 in the correct [host]:port
notation, for example [fe80::a46c:bcff:feed:a481]:8080

For consistency, also use the same function in places where it's obvious
we're dealing with IPv4 addresses.

Also see golang/go#28308

Found using semgrep using the hostport.yml rule from
https://github.com/dgryski/semgrep-go

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
pchaigno added a commit to cilium/cilium that referenced this issue Oct 22, 2020
[ upstream commit 070214f ]

Using fmt.Sprintf("%s:%d", host, port) or similar to construct network
address strings can to problems when dealing with IPv6 addresses.
However, net.JoinHostPort formats IPv6 in the correct [host]:port
notation, for example [fe80::a46c:bcff:feed:a481]:8080

For consistency, also use the same function in places where it's obvious
we're dealing with IPv4 addresses.

Also see golang/go#28308

Found using semgrep using the hostport.yml rule from
https://github.com/dgryski/semgrep-go

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno added a commit to cilium/cilium that referenced this issue Oct 22, 2020
[ upstream commit 070214f ]

Using fmt.Sprintf("%s:%d", host, port) or similar to construct network
address strings can to problems when dealing with IPv6 addresses.
However, net.JoinHostPort formats IPv6 in the correct [host]:port
notation, for example [fe80::a46c:bcff:feed:a481]:8080

For consistency, also use the same function in places where it's obvious
we're dealing with IPv4 addresses.

Also see golang/go#28308

Found using semgrep using the hostport.yml rule from
https://github.com/dgryski/semgrep-go

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Paul Chaignon <paul@cilium.io>
pchaigno added a commit to cilium/cilium that referenced this issue Oct 27, 2020
[ upstream commit 070214f ]

Using fmt.Sprintf("%s:%d", host, port) or similar to construct network
address strings can to problems when dealing with IPv6 addresses.
However, net.JoinHostPort formats IPv6 in the correct [host]:port
notation, for example [fe80::a46c:bcff:feed:a481]:8080

For consistency, also use the same function in places where it's obvious
we're dealing with IPv4 addresses.

Also see golang/go#28308

Found using semgrep using the hostport.yml rule from
https://github.com/dgryski/semgrep-go

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Signed-off-by: Paul Chaignon <paul@cilium.io>
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Feb 24, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Feb 24, 2021

I agree that you want to look for fmt.Sprint output feeding into net.Dial, not variable names.
Otherwise it seems reasonable.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2021

It also seems like you'd want to find x+":"+z as a Dial argument.
Overall this seems doable in the same vein as the printf wrapper detector already in vet.

So it sounds like the next step would be to implement a check and see how well it does on the three vet criteria from cmd/vet/README.

Does anyone want to do that?

@rsc rsc moved this from Incoming to Active in Proposals Mar 10, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Mar 10, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Mar 11, 2021

You may want to look other variants of the format string: %v:%v, %s:%v, %v:%d.

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Mar 11, 2021

@rsc I can try making a checker for this.

@rsc rsc changed the title cmd/vet: flag using %s:%d to construct network addresses proposal: cmd/vet: flag using %s:%d to construct network addresses Mar 12, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Mar 12, 2021

I grepped for net.Dial.*(Sprintf|\+) in my now year-old Go corpus and attach the 342 results.

dials.txt.gz

@rsc
Copy link
Contributor

@rsc rsc commented Mar 12, 2021

I filed #44955 as a potential alternative to discuss: just accept %s:%d in Dial instead, at least for tcp and udp.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 24, 2021

Blocked on #44955.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 31, 2021

#44955 is retracted. Back to this.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 31, 2021

I don't see anyone objecting to adding this vet check. Are there any objections?

@rsc
Copy link
Contributor

@rsc rsc commented Apr 7, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals Apr 7, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals Apr 14, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Apr 14, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: cmd/vet: flag using %s:%d to construct network addresses cmd/vet: flag using %s:%d to construct network addresses Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants