Go: cmd/vet: false positive printf detection for URL-encoded `/` (`%2F`) in string literal

[kaedys]

What version of Go are you using (go version)?

$ go version
go version go1.11 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env

GOARCH="amd64"

GOBIN=""

GOCACHE="/Users/msl21dp/Library/Caches/go-build"

GOEXE=""

GOFLAGS=""

GOHOSTARCH="amd64"

GOHOSTOS="darwin"

GOOS="darwin"

GOPATH="/Users/msl21dp/go"

GOPROXY=""

GORACE=""

GOROOT="/Users/msl21dp/.gvm/gos/go1.11"

GOTMPDIR=""

GOTOOLDIR="/Users/msl21dp/.gvm/gos/go1.11/pkg/tool/darwin_amd64"

GCCGO="gccgo"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="/Users/msl21dp/gomod/homedepot.com/vulcancore/go.mod"

CGO_CFLAGS="-g -O2"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-g -O2"

CGO_FFLAGS="-g -O2"

CGO_LDFLAGS="-g -O2"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/4y/g8py5x0x0rx2qxlpxls23gnw0000gn/T/go-build585874623=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Using the logging library github.com/spf13/jwalterweatherman, I have this line in our logs:

logging.INFO.Println("Logs can be viewed at https://console.cloud.google.com/logs/viewer?project=redacted&logName=projects%2Fredacted%2Flogs%redacted")

This is supposed to print a URL to console that the user can click to go to the logs. Since the URL includes a slash-separated string as a parameter, those slashes have to be URL-encoded, which for a forward slach is %2F.

What did you expect to see?

No vet error

What did you see instead?

vet fires an error:

Logger.Println call has possible formatting directive %2F

Normally this wouldn't be an issue, but as of Go 1.10, go test now runs go vet natively with a hardcoded list of checks, and the only option is to either convert this to a Printf and doubling the percent signs to escape them, or disable go vet during go test, neither of which is a fantastic solution.

Recommendations / possible resolutions:

• Figure out how to filter out situations like this. In this situation, the possible formatting directive is A) uppercase, and B) in a Print call with no arguments to take the place of that directive (which would mean I would have had to have improperly use a formatting directive in a non-f call _and_ neglected to include the value to replace it with in the formatted string).
• Allow specific tests to be disabled in the go test automatic run of go vet, possibly via a flag like -vetflags="-printf=false". This would also allow any of the other vet-specific flags to be passed down to it.

[dominikh]

The real question is why vet thinks that Println is a printf wrapper when it isn't. If it was in fact a printf wrapper, then the report would be correct, and you should use %%2F to escape the %.

[kaedys]

I think it's more calling out that this print call may have been _intended_ to be a Printf call, but was inadvertently coded as Println instead, causing the directive to be ignored and any arguments simply appended. And that's a reasonable and fairly useful check. Just kinda falls down when you're using Println to print out URL-encoded characters that mimic actual formatting directives.

[dominikh]

You're right, of course.

[mvdan]

Perhaps we could teach vet to ignore certain common false positives like %2F for /, %3F for ?, %3D for =, and so on. As long as we limit these to capital letters, I don't think we should worry about adding false negatives.

@alandonovan @josharian thoughts?

[alandonovan]

Yes, checking for %XX (uppercase hex not starting with zero) might be the simplest workaround for all the URL encoding cases you mention. There will still be occasional false positives, of course.

[dominikh]

IMO it shouldn't flag calls that have a single argument, either. It is unlikely that someone uses Println instead of Printf by accident _and_ forgets to provide arguments for the format verbs.