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

fmt: TestSprintf has no upper bound check for the PTR index #14712

Closed
martisch opened this issue Mar 8, 2016 · 5 comments
Closed

fmt: TestSprintf has no upper bound check for the PTR index #14712

martisch opened this issue Mar 8, 2016 · 5 comments

Comments

@martisch
Copy link
Contributor

martisch commented Mar 8, 2016

go 1.6 (tip) darwin/amd64

The TestSprintf function in fmt_test.go will panic instead of gracefully reporting an error
if the formatting string is shorter than the index the pointer pattern should appear in.

https://github.com/golang/go/blob/master/src/fmt/fmt_test.go#L995

to reproduce change e.g. the line 733 in fmt_test.go from:

{"p0=%p", new(int), "p0=0xPTR"},

to:

{"%p", new(int), "0x____________PTR"},

--- FAIL: TestSprintf (0.00s)
panic: runtime error: slice bounds out of range [recovered]
panic: runtime error: slice bounds out of range

goroutine 20 [running]:
panic(0x18a6a0, 0x8202f8090)
/Users/martisch/Documents/development/go/go/src/runtime/panic.go:500 +0x189
testing.tRunner.func1(0x820390120)
/Users/martisch/Documents/development/go/go/src/testing/testing.go:467 +0x119
panic(0x18a6a0, 0x8202f8090)
/Users/martisch/Documents/development/go/go/src/runtime/panic.go:458 +0x222
fmt_test.TestSprintf(0x820390120)
/Users/martisch/Documents/development/go/go/src/fmt/fmt_test.go:995 +0x749
testing.tRunner(0x820390120, 0x28e358)
/Users/martisch/Documents/development/go/go/src/testing/testing.go:473 +0x7e
created by testing.RunTests
/Users/martisch/Documents/development/go/go/src/testing/testing.go:581 +0x313
exit status 2
FAIL fmt 0.013s

patch incoming together with new pointer tests.

@ianlancetaylor
Copy link
Contributor

I don't see why this is a bug. It would be a bug if the test failed, but the test doesn't fail.

@martisch
Copy link
Contributor Author

martisch commented Mar 8, 2016

Ok than an improvement?

The test fails with a panic that does not show where the error is but just where to test produced an out of bounds panic in the test function.

Ideally like all other tests it should output:

--- FAIL: TestSprintf (0.00s)
fmt_test.go:1024: Sprintf("%p", 0x2b7128) = "0x2b7128" want "0x_________________PTR"
FAIL
exit status 1
FAIL fmt 0.082s

otherwise we do not know which of the dozen pointer tests actually failed.

@ianlancetaylor
Copy link
Contributor

If you want to improve the test, and test new things, sure, go ahead.

I just don't see why this is an issue. There is no bug here that I can see. For the Go project it's not necessary to associate every code change with an issue.

@martisch
Copy link
Contributor Author

martisch commented Mar 8, 2016

Ok - i just thought if a test does not trigger right e.g. does not produce the error string its supposed to display - it would be a bug. But if throwing a generic panic instead is fine then sure its ok. And since i thought it classifies as a bug it thought it would need to be documented first.

@gopherbot
Copy link

CL https://golang.org/cl/20371 mentions this issue.

@golang golang locked and limited conversation to collaborators Mar 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants