diff --git a/gopls/doc/analyzers.md b/gopls/doc/analyzers.md index 2905a0e5336..acc95d29dc4 100644 --- a/gopls/doc/analyzers.md +++ b/gopls/doc/analyzers.md @@ -290,6 +290,31 @@ Default: on. Package documentation: [framepointer](https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/framepointer) + +## `hostport`: check format of addresses passed to net.Dial + + +This analyzer flags code that produce network address strings using +fmt.Sprintf, as in this example: + + addr := fmt.Sprintf("%s:%d", host, 12345) // "will not work with IPv6" + ... + conn, err := net.Dial("tcp", addr) // "when passed to dial here" + +The analyzer suggests a fix to use the correct approach, a call to +net.JoinHostPort: + + addr := net.JoinHostPort(host, "12345") + ... + conn, err := net.Dial("tcp", addr) + +A similar diagnostic and fix are produced for a format string of "%s:%s". + + +Default: on. + +Package documentation: [hostport](https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/hostport) + ## `httpresponse`: check for mistakes using HTTP responses diff --git a/gopls/doc/release/v0.18.0.md b/gopls/doc/release/v0.18.0.md index 9f7ddd0909b..769ca69f2ea 100644 --- a/gopls/doc/release/v0.18.0.md +++ b/gopls/doc/release/v0.18.0.md @@ -40,6 +40,15 @@ functions and methods are candidates. (For a more precise analysis that may report unused exported functions too, use the `golang.org/x/tools/cmd/deadcode` command.) +## New `hostport` analyzer + +With the growing use of IPv6, forming a "host:port" string using +`fmt.Sprintf("%s:%d")` is no longer appropriate because host names may +contain colons. Gopls now reports places where a string constructed in +this fashion (or with `%s` for the port) is passed to `net.Dial` or a +related function, and offers a fix to use `net.JoinHostPort` +instead. + ## "Implementations" supports generics At long last, the "Go to Implementations" feature now fully supports diff --git a/gopls/internal/analysis/hostport/hostport.go b/gopls/internal/analysis/hostport/hostport.go new file mode 100644 index 00000000000..bf3b761b840 --- /dev/null +++ b/gopls/internal/analysis/hostport/hostport.go @@ -0,0 +1,192 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package hostport defines an analyzer for calls to net.Dial with +// addresses of the form "%s:%d" or "%s:%s", which work only with IPv4. +package hostport + +import ( + "fmt" + "go/ast" + "go/constant" + "go/types" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" + "golang.org/x/tools/go/types/typeutil" + "golang.org/x/tools/gopls/internal/util/safetoken" + "golang.org/x/tools/internal/analysisinternal" + "golang.org/x/tools/internal/astutil/cursor" +) + +const Doc = `check format of addresses passed to net.Dial + +This analyzer flags code that produce network address strings using +fmt.Sprintf, as in this example: + + addr := fmt.Sprintf("%s:%d", host, 12345) // "will not work with IPv6" + ... + conn, err := net.Dial("tcp", addr) // "when passed to dial here" + +The analyzer suggests a fix to use the correct approach, a call to +net.JoinHostPort: + + addr := net.JoinHostPort(host, "12345") + ... + conn, err := net.Dial("tcp", addr) + +A similar diagnostic and fix are produced for a format string of "%s:%s". +` + +var Analyzer = &analysis.Analyzer{ + Name: "hostport", + Doc: Doc, + URL: "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/hostport", + Requires: []*analysis.Analyzer{inspect.Analyzer}, + Run: run, +} + +func run(pass *analysis.Pass) (any, error) { + // Fast path: if the package doesn't import net and fmt, skip + // the traversal. + if !analysisinternal.Imports(pass.Pkg, "net") || + !analysisinternal.Imports(pass.Pkg, "fmt") { + return nil, nil + } + + info := pass.TypesInfo + + // checkAddr reports a diagnostic (and returns true) if e + // is a call of the form fmt.Sprintf("%d:%d", ...). + // The diagnostic includes a fix. + // + // dialCall is non-nil if the Dial call is non-local + // but within the same file. + checkAddr := func(e ast.Expr, dialCall *ast.CallExpr) { + if call, ok := e.(*ast.CallExpr); ok { + obj := typeutil.Callee(info, call) + if analysisinternal.IsFunctionNamed(obj, "fmt", "Sprintf") { + // Examine format string. + formatArg := call.Args[0] + if tv := info.Types[formatArg]; tv.Value != nil { + numericPort := false + format := constant.StringVal(tv.Value) + switch format { + case "%s:%d": + // Have: fmt.Sprintf("%s:%d", host, port) + numericPort = true + + case "%s:%s": + // Have: fmt.Sprintf("%s:%s", host, portStr) + // Keep port string as is. + + default: + return + } + + // Use granular edits to preserve original formatting. + edits := []analysis.TextEdit{ + { + // Replace fmt.Sprintf with net.JoinHostPort. + Pos: call.Fun.Pos(), + End: call.Fun.End(), + NewText: []byte("net.JoinHostPort"), + }, + { + // Delete format string. + Pos: formatArg.Pos(), + End: call.Args[1].Pos(), + }, + } + + // Turn numeric port into a string. + if numericPort { + // port => fmt.Sprintf("%d", port) + // 123 => "123" + port := call.Args[2] + newPort := fmt.Sprintf(`fmt.Sprintf("%%d", %s)`, port) + if port := info.Types[port].Value; port != nil { + if i, ok := constant.Int64Val(port); ok { + newPort = fmt.Sprintf(`"%d"`, i) // numeric constant + } + } + + edits = append(edits, analysis.TextEdit{ + Pos: port.Pos(), + End: port.End(), + NewText: []byte(newPort), + }) + } + + // Refer to Dial call, if not adjacent. + suffix := "" + if dialCall != nil { + suffix = fmt.Sprintf(" (passed to net.Dial at L%d)", + safetoken.StartPosition(pass.Fset, dialCall.Pos()).Line) + } + + pass.Report(analysis.Diagnostic{ + // Highlight the format string. + Pos: formatArg.Pos(), + End: formatArg.End(), + Message: fmt.Sprintf("address format %q does not work with IPv6%s", format, suffix), + SuggestedFixes: []analysis.SuggestedFix{{ + Message: "Replace fmt.Sprintf with net.JoinHostPort", + TextEdits: edits, + }}, + }) + } + } + } + } + + // Check address argument of each call to net.Dial et al. + inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + for curCall := range cursor.Root(inspect).Preorder((*ast.CallExpr)(nil)) { + call := curCall.Node().(*ast.CallExpr) + + obj := typeutil.Callee(info, call) + if analysisinternal.IsFunctionNamed(obj, "net", "Dial", "DialTimeout") || + analysisinternal.IsMethodNamed(obj, "net", "Dialer", "Dial") { + + switch address := call.Args[1].(type) { + case *ast.CallExpr: + // net.Dial("tcp", fmt.Sprintf("%s:%d", ...)) + checkAddr(address, nil) + + case *ast.Ident: + // addr := fmt.Sprintf("%s:%d", ...) + // ... + // net.Dial("tcp", addr) + + // Search for decl of addrVar within common ancestor of addrVar and Dial call. + if addrVar, ok := info.Uses[address].(*types.Var); ok { + pos := addrVar.Pos() + // TODO(adonovan): use Cursor.Ancestors iterator when available. + for _, curAncestor := range curCall.Stack(nil) { + if curIdent, ok := curAncestor.FindPos(pos, pos); ok { + // curIdent is the declaring ast.Ident of addr. + switch parent := curIdent.Parent().Node().(type) { + case *ast.AssignStmt: + if len(parent.Rhs) == 1 { + // Have: addr := fmt.Sprintf("%s:%d", ...) + checkAddr(parent.Rhs[0], call) + } + + case *ast.ValueSpec: + if len(parent.Values) == 1 { + // Have: var addr = fmt.Sprintf("%s:%d", ...) + checkAddr(parent.Values[0], call) + } + } + break + } + } + } + } + } + } + return nil, nil +} diff --git a/gopls/internal/analysis/hostport/hostport_test.go b/gopls/internal/analysis/hostport/hostport_test.go new file mode 100644 index 00000000000..4e57a43e8d4 --- /dev/null +++ b/gopls/internal/analysis/hostport/hostport_test.go @@ -0,0 +1,17 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package hostport_test + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" + "golang.org/x/tools/gopls/internal/analysis/hostport" +) + +func Test(t *testing.T) { + testdata := analysistest.TestData() + analysistest.RunWithSuggestedFixes(t, testdata, hostport.Analyzer, "a") +} diff --git a/gopls/internal/analysis/hostport/main.go b/gopls/internal/analysis/hostport/main.go new file mode 100644 index 00000000000..99f7a09ec39 --- /dev/null +++ b/gopls/internal/analysis/hostport/main.go @@ -0,0 +1,14 @@ +// Copyright 2024 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +//go:build ignore + +package main + +import ( + "golang.org/x/tools/go/analysis/singlechecker" + "golang.org/x/tools/gopls/internal/analysis/hostport" +) + +func main() { singlechecker.Main(hostport.Analyzer) } diff --git a/gopls/internal/analysis/hostport/testdata/src/a/a.go b/gopls/internal/analysis/hostport/testdata/src/a/a.go new file mode 100644 index 00000000000..7d80f80f734 --- /dev/null +++ b/gopls/internal/analysis/hostport/testdata/src/a/a.go @@ -0,0 +1,40 @@ +package a + +import ( + "fmt" + "net" +) + +func direct(host string, port int, portStr string) { + // Dial, directly called with result of Sprintf. + net.Dial("tcp", fmt.Sprintf("%s:%d", host, port)) // want `address format "%s:%d" does not work with IPv6` + + net.Dial("tcp", fmt.Sprintf("%s:%s", host, portStr)) // want `address format "%s:%s" does not work with IPv6` +} + +// port is a constant: +var addr4 = fmt.Sprintf("%s:%d", "localhost", 123) // want `address format "%s:%d" does not work with IPv6 \(passed to net.Dial at L39\)` + +func indirect(host string, port int) { + // Dial, addr is immediately preceding. + { + addr1 := fmt.Sprintf("%s:%d", host, port) // want `address format "%s:%d" does not work with IPv6.*at L22` + net.Dial("tcp", addr1) + } + + // DialTimeout, addr is in ancestor block. + addr2 := fmt.Sprintf("%s:%d", host, port) // want `address format "%s:%d" does not work with IPv6.*at L28` + { + net.DialTimeout("tcp", addr2, 0) + } + + // Dialer.Dial, addr is declared with var. + var dialer net.Dialer + { + var addr3 = fmt.Sprintf("%s:%d", host, port) // want `address format "%s:%d" does not work with IPv6.*at L35` + dialer.Dial("tcp", addr3) + } + + // Dialer.Dial again, addr is declared at package level. + dialer.Dial("tcp", addr4) +} diff --git a/gopls/internal/analysis/hostport/testdata/src/a/a.go.golden b/gopls/internal/analysis/hostport/testdata/src/a/a.go.golden new file mode 100644 index 00000000000..b219224e0aa --- /dev/null +++ b/gopls/internal/analysis/hostport/testdata/src/a/a.go.golden @@ -0,0 +1,40 @@ +package a + +import ( + "fmt" + "net" +) + +func direct(host string, port int, portStr string) { + // Dial, directly called with result of Sprintf. + net.Dial("tcp", net.JoinHostPort(host, fmt.Sprintf("%d", port))) // want `address format "%s:%d" does not work with IPv6` + + net.Dial("tcp", net.JoinHostPort(host, portStr)) // want `address format "%s:%s" does not work with IPv6` +} + +// port is a constant: +var addr4 = net.JoinHostPort("localhost", "123") // want `address format "%s:%d" does not work with IPv6 \(passed to net.Dial at L39\)` + +func indirect(host string, port int) { + // Dial, addr is immediately preceding. + { + addr1 := net.JoinHostPort(host, fmt.Sprintf("%d", port)) // want `address format "%s:%d" does not work with IPv6.*at L22` + net.Dial("tcp", addr1) + } + + // DialTimeout, addr is in ancestor block. + addr2 := net.JoinHostPort(host, fmt.Sprintf("%d", port)) // want `address format "%s:%d" does not work with IPv6.*at L28` + { + net.DialTimeout("tcp", addr2, 0) + } + + // Dialer.Dial, addr is declared with var. + var dialer net.Dialer + { + var addr3 = net.JoinHostPort(host, fmt.Sprintf("%d", port)) // want `address format "%s:%d" does not work with IPv6.*at L35` + dialer.Dial("tcp", addr3) + } + + // Dialer.Dial again, addr is declared at package level. + dialer.Dial("tcp", addr4) +} diff --git a/gopls/internal/doc/api.json b/gopls/internal/doc/api.json index 982ec34909b..b6fcc8f5b19 100644 --- a/gopls/internal/doc/api.json +++ b/gopls/internal/doc/api.json @@ -440,6 +440,11 @@ "Doc": "report assembly that clobbers the frame pointer before saving it", "Default": "true" }, + { + "Name": "\"hostport\"", + "Doc": "check format of addresses passed to net.Dial\n\nThis analyzer flags code that produce network address strings using\nfmt.Sprintf, as in this example:\n\n addr := fmt.Sprintf(\"%s:%d\", host, 12345) // \"will not work with IPv6\"\n ...\n conn, err := net.Dial(\"tcp\", addr) // \"when passed to dial here\"\n\nThe analyzer suggests a fix to use the correct approach, a call to\nnet.JoinHostPort:\n\n addr := net.JoinHostPort(host, \"12345\")\n ...\n conn, err := net.Dial(\"tcp\", addr)\n\nA similar diagnostic and fix are produced for a format string of \"%s:%s\".\n", + "Default": "true" + }, { "Name": "\"httpresponse\"", "Doc": "check for mistakes using HTTP responses\n\nA common mistake when using the net/http package is to defer a function\ncall to close the http.Response Body before checking the error that\ndetermines whether the response is valid:\n\n\tresp, err := http.Head(url)\n\tdefer resp.Body.Close()\n\tif err != nil {\n\t\tlog.Fatal(err)\n\t}\n\t// (defer statement belongs here)\n\nThis checker helps uncover latent nil dereference bugs by reporting a\ndiagnostic for such mistakes.", @@ -1060,6 +1065,12 @@ "URL": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/framepointer", "Default": true }, + { + "Name": "hostport", + "Doc": "check format of addresses passed to net.Dial\n\nThis analyzer flags code that produce network address strings using\nfmt.Sprintf, as in this example:\n\n addr := fmt.Sprintf(\"%s:%d\", host, 12345) // \"will not work with IPv6\"\n ...\n conn, err := net.Dial(\"tcp\", addr) // \"when passed to dial here\"\n\nThe analyzer suggests a fix to use the correct approach, a call to\nnet.JoinHostPort:\n\n addr := net.JoinHostPort(host, \"12345\")\n ...\n conn, err := net.Dial(\"tcp\", addr)\n\nA similar diagnostic and fix are produced for a format string of \"%s:%s\".\n", + "URL": "https://pkg.go.dev/golang.org/x/tools/gopls/internal/analysis/hostport", + "Default": true + }, { "Name": "httpresponse", "Doc": "check for mistakes using HTTP responses\n\nA common mistake when using the net/http package is to defer a function\ncall to close the http.Response Body before checking the error that\ndetermines whether the response is valid:\n\n\tresp, err := http.Head(url)\n\tdefer resp.Body.Close()\n\tif err != nil {\n\t\tlog.Fatal(err)\n\t}\n\t// (defer statement belongs here)\n\nThis checker helps uncover latent nil dereference bugs by reporting a\ndiagnostic for such mistakes.", diff --git a/gopls/internal/settings/analysis.go b/gopls/internal/settings/analysis.go index 7e13c801a85..9663c2289d6 100644 --- a/gopls/internal/settings/analysis.go +++ b/gopls/internal/settings/analysis.go @@ -49,6 +49,7 @@ import ( "golang.org/x/tools/gopls/internal/analysis/deprecated" "golang.org/x/tools/gopls/internal/analysis/embeddirective" "golang.org/x/tools/gopls/internal/analysis/fillreturns" + "golang.org/x/tools/gopls/internal/analysis/hostport" "golang.org/x/tools/gopls/internal/analysis/infertypeargs" "golang.org/x/tools/gopls/internal/analysis/modernize" "golang.org/x/tools/gopls/internal/analysis/nonewvars" @@ -158,6 +159,7 @@ func init() { {analyzer: sortslice.Analyzer, enabled: true}, {analyzer: embeddirective.Analyzer, enabled: true}, {analyzer: waitgroup.Analyzer, enabled: true}, // to appear in cmd/vet@go1.25 + {analyzer: hostport.Analyzer, enabled: true}, // to appear in cmd/vet@go1.25 {analyzer: modernize.Analyzer, enabled: true, severity: protocol.SeverityInformation}, // disabled due to high false positives