go/analysis/passes/hostport: fix four bugs

All arise from assuming that just because the program is well
typed, that the fmt.Sprintf call is valid, specifically:
1. don't assume sufficient call arguments;
2. don't assume portExpr.String() == Format(portExpr);
3. don't assume that if port is a constant, it's an integer.
4. don't transform a constant port kPort into a string "123"
   since it loses the connection to the symbolic constant.

+ tests of all four

Change-Id: I7c6e448efe30065d1c82ed144382788e16c70acf
Reviewed-on: https://go-review.googlesource.com/c/tools/+/702895
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Alan Donovan <adonovan@google.com>

Author: adonovan

go/analysis/passes/hostport/hostport.go

@@ -10,7 +10,9 @@ import (
 	"fmt"
 	"go/ast"
 	"go/constant"
+	"go/token"
 	"go/types"
+	"strconv"
 
 	"golang.org/x/tools/go/analysis"
 	"golang.org/x/tools/go/analysis/passes/inspect"
@@ -57,13 +59,16 @@ func run(pass *analysis.Pass) (any, error) {
 	}
 
 	// checkAddr reports a diagnostic (and returns true) if e
-	// is a call of the form fmt.Sprintf("%d:%d", ...).
+	// is a call of the form fmt.Sprintf("%s:%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 && typeutil.Callee(info, call) == fmtSprintf {
+		if call, ok := e.(*ast.CallExpr); ok &&
+			len(call.Args) == 3 &&
+			typeutil.Callee(info, call) == fmtSprintf {
+
 			// Examine format string.
 			formatArg := call.Args[0]
 			if tv := info.Types[formatArg]; tv.Value != nil {
@@ -99,21 +104,41 @@ func run(pass *analysis.Pass) (any, error) {
 
 				// 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
+
+					// Is port an integer literal?
+					//
+					// (Don't allow arbitrary constants k otherwise the
+					// transformation k => fmt.Sprintf("%d", "123")
+					// loses the symbolic connection to k.)
+					var kPort int64 = -1
+					if lit, ok := port.(*ast.BasicLit); ok && lit.Kind == token.INT {
+						if v, err := strconv.ParseInt(lit.Value, 0, 64); err == nil {
+							kPort = v
 						}
 					}
-
-					edits = append(edits, analysis.TextEdit{
-						Pos:     port.Pos(),
-						End:     port.End(),
-						NewText: []byte(newPort),
-					})
+					if kPort >= 0 {
+						// literal: 0x7B  => "123"
+						edits = append(edits, analysis.TextEdit{
+							Pos:     port.Pos(),
+							End:     port.End(),
+							NewText: fmt.Appendf(nil, `"%d"`, kPort), // (decimal)
+						})
+					} else {
+						// non-literal: port => fmt.Sprintf("%d", port)
+						edits = append(edits, []analysis.TextEdit{
+							{
+								Pos:     port.Pos(),
+								End:     port.Pos(),
+								NewText: []byte(`fmt.Sprintf("%d", `),
+							},
+							{
+								Pos:     port.End(),
+								End:     port.End(),
+								NewText: []byte(`)`),
+							},
+						}...)
+					}
 				}
 
 				// Refer to Dial call, if not adjacent.

go/analysis/passes/hostport/testdata/src/a/a.go

@@ -12,7 +12,7 @@ func direct(host string, port int, portStr string) {
 	net.Dial("tcp", fmt.Sprintf("%s:%s", host, portStr)) // want `address format "%s:%s" does not work with IPv6`
 }
 
-// port is a constant:
+// port is a literal:
 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) {
@@ -38,3 +38,17 @@ func indirect(host string, port int) {
 	// Dialer.Dial again, addr is declared at package level.
 	dialer.Dial("tcp", addr4)
 }
+
+// Regression tests for crashes in well-typed code that nonetheless mis-uses Sprintf:
+// too few arguments, or port is not an integer.
+var (
+	_, _ = net.Dial("tcp", fmt.Sprintf("%s:%d"))
+	_, _ = net.Dial("tcp", fmt.Sprintf("%s:%d", "host"))
+	_, _ = net.Dial("tcp", fmt.Sprintf("%s:%d", "host", "port")) // want `address format "%s:%d" does not work with IPv6`
+)
+
+func _() {
+	// port is a non-constant literal
+	const port = 0x7B
+	_, _ = net.Dial("tcp", fmt.Sprintf("%s:%d", "localhost", port)) // want `address format "%s:%d" does not work with IPv6`
+}

go/analysis/passes/hostport/testdata/src/a/a.go.golden

@@ -12,7 +12,7 @@ func direct(host string, port int, portStr string) {
 	net.Dial("tcp", net.JoinHostPort(host, portStr)) // want `address format "%s:%s" does not work with IPv6`
 }
 
-// port is a constant:
+// port is a literal:
 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) {
@@ -38,3 +38,17 @@ func indirect(host string, port int) {
 	// Dialer.Dial again, addr is declared at package level.
 	dialer.Dial("tcp", addr4)
 }
+
+// Regression tests for crashes in well-typed code that nonetheless mis-uses Sprintf:
+// too few arguments, or port is not an integer.
+var (
+	_, _ = net.Dial("tcp", fmt.Sprintf("%s:%d"))
+	_, _ = net.Dial("tcp", fmt.Sprintf("%s:%d", "host"))
+	_, _ = net.Dial("tcp", net.JoinHostPort("host", fmt.Sprintf("%d", "port"))) // want `address format "%s:%d" does not work with IPv6`
+)
+
+func _() {
+	// port is a non-constant literal
+	const port = 0x7B
+	_, _ = net.Dial("tcp", net.JoinHostPort("localhost", fmt.Sprintf("%d", port))) // want `address format "%s:%d" does not work with IPv6`
+}