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: false positive printf detection for URL-encoded / (%2F) in string literal #29854

Open
kaedys opened this issue Jan 21, 2019 · 6 comments
Open

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

kaedys opened this issue Jan 21, 2019 · 6 comments
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Unplanned

Comments

@kaedys
Copy link

kaedys commented Jan 21, 2019

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
Copy link
Member

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
Copy link
Author

kaedys commented Jan 21, 2019

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
Copy link
Member

You're right, of course.

@mvdan
Copy link
Member

mvdan commented Jan 21, 2019

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?

@mvdan mvdan added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jan 21, 2019
@alandonovan
Copy link
Contributor

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
Copy link
Member

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.

@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@adonovan adonovan added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Apr 23, 2023
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) NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@dominikh @mvdan @adonovan @alandonovan @kaedys @seankhliao