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

Jump to bottom

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

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

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

stapelberg opened this issue Oct 22, 2018 · 14 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest Proposal Proposal-Accepted
Projects
Proposals (old)
Milestone
Unplanned

Comments

@stapelberg
Copy link
Contributor

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)")
@dominikh dominikh mentioned this issue Oct 23, 2018
Open
@mvdan
Copy link
Member

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 FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Oct 23, 2018
@bcmills bcmills added this to the Unplanned milestone Oct 23, 2018
@froodian froodian mentioned this issue Aug 22, 2019
Merged
lisa added a commit to openshift/managed-cluster-validating-webhooks that referenced this issue Aug 21, 2020
@lisa
Use net.JoinHostPort over fmt.Sprintf
cdfb9ff 
See: golang/go#28308, dominikh/go-tools#358 and others.
@lisa lisa mentioned this issue Aug 21, 2020
Merged
tklauser added a commit to cilium/cilium that referenced this issue Aug 26, 2020
@tklauser
Use net.JoinHostPort to construct network address strings
6c5c867 
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>
@tklauser tklauser mentioned this issue Aug 26, 2020
Merged
kkourt pushed a commit to cilium/cilium that referenced this issue Sep 4, 2020
@tklauser @kkourt
Use net.JoinHostPort to construct network address strings
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>
pchaigno pushed a commit to cilium/cilium that referenced this issue Oct 22, 2020
@tklauser @pchaigno
Use net.JoinHostPort to construct network address strings
0635d8a 
[ 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 pushed a commit to cilium/cilium that referenced this issue Oct 27, 2020
@tklauser @pchaigno
Use net.JoinHostPort to construct network address strings
953b2e9 
[ 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 (old) Feb 24, 2021
@rsc
Copy link
Contributor

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 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 (old) Mar 10, 2021
@rsc
Copy link
Contributor

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

@rsc rsc mentioned this issue Mar 10, 2021
Open
@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Mar 10, 2021
@egonelbre
Copy link
Contributor

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

@timothy-king
Copy link
Contributor

@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 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

@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 12, 2021
@rsc rsc mentioned this issue Mar 12, 2021
Closed
@rsc
Copy link
Contributor

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 commented Mar 24, 2021

Blocked on #44955.

@rsc
Copy link
Contributor

rsc commented Mar 31, 2021

#44955 is retracted. Back to this.

@rsc
Copy link
Contributor

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 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 (old) Apr 7, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Apr 14, 2021
@rsc
Copy link
Contributor

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
@atc0005 atc0005 mentioned this issue Mar 8, 2022
Closed
Open
@gopherbot
Copy link

Change https://go.dev/cl/554495 mentions this issue: go/analysis/passes/hostport: report net.Dial("%s:%d") addresses

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest Proposal Proposal-Accepted
Projects
Proposals
Status: Accepted
Proposals (old)
Accepted
Milestone
Unplanned
Development

No branches or pull requests
7 participants
@stapelberg @rsc @egonelbre @timothy-king @mvdan @bcmills @gopherbot