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

cmd/vet: doesn't handle formatting strings in variables #44063

Open
raelyz opened this issue Feb 2, 2021 · 7 comments
Open

cmd/vet: doesn't handle formatting strings in variables #44063

raelyz opened this issue Feb 2, 2021 · 7 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

Comments

@raelyz
Copy link

raelyz commented Feb 2, 2021

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

$ go version
go version go1.15.6

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
C:\Projects\Go\src\testing>go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Ray\AppData\Local\go-build
set GOENV=C:\Users\Ray\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Projects\GoLib\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Projects\GoLib;C:\Projects\Go;
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=c:\go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=c:\go\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\Ray\AppData\Local\Temp\go-build204160270=/tmp/go-build -gno-record-gcc-switches

What did you do?

type invalidSide struct {
	erroneousSide float64
	errorMessage  string
}

func (invalidSide *invalidSide) Error() string {
	format := "%v given for one of the sides. values must be more than 0."
	return fmt.Sprintf(format, invalidSide)
	return fmt.Sprintf("%v given for one of the sides. values must be more than 0.", invalidSide)
}

Here is a sample of the code that resulted in a small issue I noticed.

What did you expect to see?

The first return statement should be flagged out by Go-Vet as it results in recursive Error calls.

What did you see instead?

The file saved without issue and crashes during run time. The 2nd return statement is able to be detected by Go-Vet perfectly fine.

@seankhliao seankhliao changed the title Go-Vet not detecting certain instances of invalid arguments with Sprintf cmd/vet: doesn't handle formatting strings in variables Feb 2, 2021
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 2, 2021
@seankhliao
Copy link
Member

observation: it works if the format string is constant:

func (invalidSide *invalidSide) Error() string {
	const format = "%v given for one of the sides. values must be more than 0."
	return fmt.Sprintf(format, invalidSide)
}

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Feb 2, 2021
@timothy-king
Copy link
Contributor

It is possible to support this exact case in vet. A string constant flowing through a non-package scope string variable that is set exactly once into the format position of an fmt.*printf function. Handling more than this (handling control flow, being set conditionally, flow through globals, etc.) would require a fairly different checker.

@raelyz how did you end up encountering this? Is it possible to see the original? Also is the above example minimized? I am a bit worried that if we extend vet to handle this, it would not actually have helped you.

@raelyz
Copy link
Author

raelyz commented Feb 3, 2021

Hi Timothy, I'm currently learning Go in a course and the instructor demonstrated error handling with that particular example except for the fact she used invalidSide.errorMessage as the second argument to Sprintf.

A classmate of mine used invalidSide instead with the entire string as the first argument and vet prompted her to fix the issue due to the recursive calls on Error(). She changed it to a variable as demonstrated in our lecture slides and the error disappeared which was rather puzzling. This lead me to dig deeper to this as I couldn't find much documentation on the best practices per say in utilizing Sprintf.

I understand vet utilizes heuristics to check and as a result it may not be comprehensive. Mainly raising the issue for consideration in the event others may have encountered similar instances but since no errors were flagged by vet they couldn't pinpoint the issue. I'm still fairly new to programming in general so I would leave my inputs as that and maybe ya'll could make the call on this issue. Thanks!

@timothy-king
Copy link
Contributor

Thank you for the additional context. It sounds like this might be frequent enough for vet to raise an alert.

Some potential extensions that could have done something:

  1. Suggest replacing the variable with const. (@seankhliao 's observation )
  2. Suggest replacing the variable by inlining the constant.
  3. Improve the printf checker to allow the format argument to also be an identifiers when it can only be a single constant value. Should be able to figure this out from the TypeInfo

@raelyz
Copy link
Author

raelyz commented Feb 3, 2021

Totally agree, 1 and 2 are current best practices per say to prevent such errors from happening in run time.

Just a possible scenario but in the event of 3, hypothetically if someone is trying to print dynamically with different verbs in various cases, this may potentially break their code. The tradeoff here would then be the relative occurrence of such a practice vs the safety or potential confusion to people new to go should they encounter something similar to what I've seen.

Would a plausible hotfix in the short run be to highlight it within the printf package to either 1 or 2 when utilizing printf?

@timothy-king
Copy link
Contributor

Just a possible scenario but in the event of 3, hypothetically if someone is trying to print dynamically with different verbs in various cases, this may potentially break their code.

My understanding of 3 is that it would not flag this case as it only would look for values that syntactically can only be a single constant value. This is effectively just variables that are assigned to a constant when declared. (This could be done more thoroughly with a constant propagation domain, but this would likely be significantly slower for unclear additional benefit.)

Would a plausible hotfix in the short run be to highlight it within the printf package to either 1 or 2 when utilizing printf?

There is no obvious "bug" preventing someone using go, getting an incorrect answer, or a security problem. This is a feature request for improved capabilities for vet. I do not see a "hotfix" as necessary.

@timothy-king timothy-king added this to the Backlog milestone Feb 4, 2021
@timothy-king timothy-king added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Feb 4, 2021
@adonovan
Copy link
Member

I think this issue is waiting for evidence that the problem is frequent enough to justify the extra checker effort to notice "effectively constant" format strings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

No branches or pull requests

4 participants